From bbbc0a863ff9b5f33b6720423569f58b0a6fc85d Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Mon, 25 Jul 2022 15:21:24 +0900
Subject: [PATCH 1/2] Add test coverage of `WorkingBeatmap` retrieval from
 `BeatmapManager`

---
 .../Beatmaps/WorkingBeatmapManagerTest.cs     | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 osu.Game.Tests/Beatmaps/WorkingBeatmapManagerTest.cs

diff --git a/osu.Game.Tests/Beatmaps/WorkingBeatmapManagerTest.cs b/osu.Game.Tests/Beatmaps/WorkingBeatmapManagerTest.cs
new file mode 100644
index 0000000000..0348e47d4a
--- /dev/null
+++ b/osu.Game.Tests/Beatmaps/WorkingBeatmapManagerTest.cs
@@ -0,0 +1,100 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Audio;
+using osu.Framework.Extensions;
+using osu.Framework.Platform;
+using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Database;
+using osu.Game.Rulesets;
+using osu.Game.Tests.Resources;
+using osu.Game.Tests.Visual;
+
+namespace osu.Game.Tests.Beatmaps
+{
+    [HeadlessTest]
+    public class WorkingBeatmapManagerTest : OsuTestScene
+    {
+        private BeatmapManager beatmaps = null!;
+
+        private BeatmapSetInfo importedSet = null!;
+
+        [BackgroundDependencyLoader]
+        private void load(GameHost host, AudioManager audio, RulesetStore rulesets)
+        {
+            Dependencies.Cache(beatmaps = new BeatmapManager(LocalStorage, Realm, rulesets, null, audio, Resources, host, Beatmap.Default));
+        }
+
+        [SetUpSteps]
+        public void SetUpSteps()
+        {
+            AddStep("import beatmap", () =>
+            {
+                beatmaps.Import(TestResources.GetQuickTestBeatmapForImport()).WaitSafely();
+                importedSet = beatmaps.GetAllUsableBeatmapSets().First();
+            });
+        }
+
+        [Test]
+        public void TestGetWorkingBeatmap() => AddStep("run test", () =>
+        {
+            Assert.That(beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First()), Is.Not.Null);
+        });
+
+        [Test]
+        public void TestCachedRetrievalNoFiles() => AddStep("run test", () =>
+        {
+            var beatmap = importedSet.Beatmaps.First();
+
+            Assert.That(beatmap.BeatmapSet?.Files, Is.Empty);
+
+            var first = beatmaps.GetWorkingBeatmap(beatmap);
+            var second = beatmaps.GetWorkingBeatmap(beatmap);
+
+            Assert.That(first, Is.SameAs(second));
+            Assert.That(first.BeatmapInfo.BeatmapSet?.Files, Has.Count.GreaterThan(0));
+        });
+
+        [Test]
+        public void TestCachedRetrievalWithFiles() => AddStep("run test", () =>
+        {
+            var beatmap = Realm.Run(r => r.Find<BeatmapInfo>(importedSet.Beatmaps.First().ID).Detach());
+
+            Assert.That(beatmap.BeatmapSet?.Files, Has.Count.GreaterThan(0));
+
+            var first = beatmaps.GetWorkingBeatmap(beatmap);
+            var second = beatmaps.GetWorkingBeatmap(beatmap);
+
+            Assert.That(first, Is.SameAs(second));
+            Assert.That(first.BeatmapInfo.BeatmapSet?.Files, Has.Count.GreaterThan(0));
+        });
+
+        [Test]
+        public void TestForcedRefetchRetrievalNoFiles() => AddStep("run test", () =>
+        {
+            var beatmap = importedSet.Beatmaps.First();
+
+            Assert.That(beatmap.BeatmapSet?.Files, Is.Empty);
+
+            var first = beatmaps.GetWorkingBeatmap(beatmap);
+            var second = beatmaps.GetWorkingBeatmap(beatmap, true);
+            Assert.That(first, Is.Not.SameAs(second));
+        });
+
+        [Test]
+        public void TestForcedRefetchRetrievalWithFiles() => AddStep("run test", () =>
+        {
+            var beatmap = Realm.Run(r => r.Find<BeatmapInfo>(importedSet.Beatmaps.First().ID).Detach());
+
+            Assert.That(beatmap.BeatmapSet?.Files, Has.Count.GreaterThan(0));
+
+            var first = beatmaps.GetWorkingBeatmap(beatmap);
+            var second = beatmaps.GetWorkingBeatmap(beatmap, true);
+            Assert.That(first, Is.Not.SameAs(second));
+        });
+    }
+}

From 2ec90e37bb9e005dea4f99691758793a549ecd94 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Mon, 25 Jul 2022 14:59:11 +0900
Subject: [PATCH 2/2] Fix calls to `GetWorkingBeatmap` invalidating cache too
 often

With recent changes, the pathway between refetching (on request) and
refetching (on requirement due to unpopulated files) was combined.
Unfortunately this pathway also added a forced invalidation, which
should not have been applied to the second case.

Closes https://github.com/ppy/osu/issues/19365.
---
 osu.Game/Beatmaps/BeatmapManager.cs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs
index 30456afd2f..7717c9cc87 100644
--- a/osu.Game/Beatmaps/BeatmapManager.cs
+++ b/osu.Game/Beatmaps/BeatmapManager.cs
@@ -439,12 +439,15 @@ namespace osu.Game.Beatmaps
         {
             if (beatmapInfo != null)
             {
-                // Detached sets don't come with files.
-                // If we seem to be missing files, now is a good time to re-fetch.
-                if (refetch || beatmapInfo.IsManaged || beatmapInfo.BeatmapSet?.Files.Count == 0)
-                {
+                if (refetch)
                     workingBeatmapCache.Invalidate(beatmapInfo);
 
+                // Detached beatmapsets don't come with files as an optimisation (see `RealmObjectExtensions.beatmap_set_mapper`).
+                // If we seem to be missing files, now is a good time to re-fetch.
+                bool missingFiles = beatmapInfo.BeatmapSet?.Files.Count == 0;
+
+                if (refetch || beatmapInfo.IsManaged || missingFiles)
+                {
                     Guid id = beatmapInfo.ID;
                     beatmapInfo = Realm.Run(r => r.Find<BeatmapInfo>(id)?.Detach()) ?? beatmapInfo;
                 }