From 90b87cbb9eaba15ade0585a881ded8978192d871 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Mon, 28 Jun 2021 10:11:27 +0900
Subject: [PATCH] Add back unidirectional online id check

---
 osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs | 10 +++-------
 osu.Game/Beatmaps/BeatmapManager.cs             |  8 ++++++++
 osu.Game/Database/ArchiveModelManager.cs        | 12 +++++++++++-
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs
index 990e75df81..e87b82cd2a 100644
--- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs
+++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs
@@ -441,9 +441,7 @@ namespace osu.Game.Tests.Beatmaps.IO
             }
         }
 
-        [TestCase(true)]
-        [TestCase(false)]
-        public async Task TestImportThenDeleteThenImportWithOnlineIDMismatch(bool set)
+        public async Task TestImportThenDeleteThenImportWithOnlineIDsMissing()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
             using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"{nameof(ImportBeatmapTest)}-{set}"))
@@ -454,10 +452,8 @@ namespace osu.Game.Tests.Beatmaps.IO
 
                     var imported = await LoadOszIntoOsu(osu);
 
-                    if (set)
-                        imported.OnlineBeatmapSetID = 1234;
-                    else
-                        imported.Beatmaps.First().OnlineBeatmapID = 1234;
+                    foreach (var b in imported.Beatmaps)
+                        b.OnlineBeatmapID = null;
 
                     osu.Dependencies.Get<BeatmapManager>().Update(imported);
 
diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs
index 00af06703d..f854a5fecb 100644
--- a/osu.Game/Beatmaps/BeatmapManager.cs
+++ b/osu.Game/Beatmaps/BeatmapManager.cs
@@ -319,6 +319,14 @@ namespace osu.Game.Beatmaps
         /// <returns>The first result for the provided query, or null if no results were found.</returns>
         public BeatmapSetInfo QueryBeatmapSet(Expression<Func<BeatmapSetInfo, bool>> query) => beatmaps.ConsumableItems.AsNoTracking().FirstOrDefault(query);
 
+        protected override bool CanSkipImport(BeatmapSetInfo existing, BeatmapSetInfo import)
+        {
+            if (!base.CanReuseExisting(existing, import))
+                return false;
+
+            return existing.Beatmaps.Any(b => b.OnlineBeatmapID != null);
+        }
+
         protected override bool CanReuseExisting(BeatmapSetInfo existing, BeatmapSetInfo import)
         {
             if (!base.CanReuseExisting(existing, import))
diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs
index 84473f57fe..6d8b671fd8 100644
--- a/osu.Game/Database/ArchiveModelManager.cs
+++ b/osu.Game/Database/ArchiveModelManager.cs
@@ -372,7 +372,8 @@ namespace osu.Game.Database
                     //
                     // note that this should really be checking filesizes on disk (of existing files) for some degree of sanity.
                     // or alternatively doing a faster hash check. either of these require database changes and reprocessing of existing files.
-                    if (getFilenames(existing.Files).SequenceEqual(getShortenedFilenames(archive).Select(p => p.shortened).OrderBy(f => f)))
+                    if (CanSkipImport(existing, item) &&
+                        getFilenames(existing.Files).SequenceEqual(getShortenedFilenames(archive).Select(p => p.shortened).OrderBy(f => f)))
                     {
                         LogForModel(item, $"Found existing (optimised) {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import.");
                         Undelete(existing);
@@ -801,6 +802,15 @@ namespace osu.Game.Database
         /// <returns>An existing model which matches the criteria to skip importing, else null.</returns>
         protected TModel CheckForExisting(TModel model) => model.Hash == null ? null : ModelStore.ConsumableItems.FirstOrDefault(b => b.Hash == model.Hash);
 
+        /// <summary>
+        /// Whether inport can be skipped after finding an existing import early in the process.
+        /// Only valid when <see cref="ComputeHash"/> is not overridden.
+        /// </summary>
+        /// <param name="existing">The existing model.</param>
+        /// <param name="import">The newly imported model.</param>
+        /// <returns>Whether to skip this import completely.</returns>
+        protected virtual bool CanSkipImport(TModel existing, TModel import) => true;
+
         /// <summary>
         /// After an existing <typeparamref name="TModel"/> is found during an import process, the default behaviour is to use/restore the existing
         /// item and skip the import. This method allows changing that behaviour.