Merge pull request #4453 from peppy/ensure-import-sanity

Ensure import continues if an existing item's online IDs are in a bad state

osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs

@@ -207,6 +207,41 @@ namespace osu.Game.Tests.Beatmaps.IO
             }
         }
 
+        [TestCase(true)]
+        [TestCase(false)]
+        public void TestImportThenDeleteThenImportWithOnlineIDMismatch(bool set)
+        {
+            //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"TestImportThenDeleteThenImport-{set}"))
+            {
+                try
+                {
+                    var osu = loadOsu(host);
+
+                    var imported = LoadOszIntoOsu(osu);
+
+                    if (set)
+                        imported.OnlineBeatmapSetID = 1234;
+                    else
+                        imported.Beatmaps.First().OnlineBeatmapID = 1234;
+
+                    osu.Dependencies.Get<BeatmapManager>().Update(imported);
+
+                    deleteBeatmapSet(imported, osu);
+
+                    var importedSecondTime = LoadOszIntoOsu(osu);
+
+                    // check the newly "imported" beatmap has been reimported due to mismatch (even though hashes matched)
+                    Assert.IsTrue(imported.ID != importedSecondTime.ID);
+                    Assert.IsTrue(imported.Beatmaps.First().ID != importedSecondTime.Beatmaps.First().ID);
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
         [Test]
         [NonParallelizable]
         [Ignore("Binding IPC on Appveyor isn't working (port in use). Need to figure out why")]

osu.Game/Beatmaps/BeatmapManager.cs

@@ -102,11 +102,14 @@ namespace osu.Game.Beatmaps
                 b.BeatmapSet = beatmapSet;
             }
 
-            validateOnlineIds(beatmapSet.Beatmaps);
+            validateOnlineIds(beatmapSet);
 
             foreach (BeatmapInfo b in beatmapSet.Beatmaps)
                 fetchAndPopulateOnlineValues(b, beatmapSet.Beatmaps);
+        }
 
+        protected override void PreImport(BeatmapSetInfo beatmapSet)
+        {
             // check if a set already exists with the same online id, delete if it does.
             if (beatmapSet.OnlineBeatmapSetID != null)
             {
@@ -120,14 +123,30 @@ namespace osu.Game.Beatmaps
             }
         }
 
-        private void validateOnlineIds(List<BeatmapInfo> beatmaps)
+        private void validateOnlineIds(BeatmapSetInfo beatmapSet)
         {
-            var beatmapIds = beatmaps.Where(b => b.OnlineBeatmapID.HasValue).Select(b => b.OnlineBeatmapID).ToList();
+            var beatmapIds = beatmapSet.Beatmaps.Where(b => b.OnlineBeatmapID.HasValue).Select(b => b.OnlineBeatmapID).ToList();
 
-            // ensure all IDs are unique in this set and none match existing IDs in the local beatmap store.
+            // ensure all IDs are unique
-            if (beatmapIds.GroupBy(b => b).Any(g => g.Count() > 1) || QueryBeatmaps(b => beatmapIds.Contains(b.OnlineBeatmapID)).Any())
+            if (beatmapIds.GroupBy(b => b).Any(g => g.Count() > 1))
-                // remove all online IDs if any problems were found.
+            {
-                beatmaps.ForEach(b => b.OnlineBeatmapID = null);
+                resetIds();
+                return;
+            }
+
+            // find any existing beatmaps in the database that have matching online ids
+            var existingBeatmaps = QueryBeatmaps(b => beatmapIds.Contains(b.OnlineBeatmapID)).ToList();
+
+            if (existingBeatmaps.Count > 0)
+            {
+                // reset the import ids (to force a re-fetch) *unless* they match the candidate CheckForExisting set.
+                // we can ignore the case where the new ids are contained by the CheckForExisting set as it will either be used (import skipped) or deleted.
+                var existing = CheckForExisting(beatmapSet);
+                if (existing == null || existingBeatmaps.Any(b => !existing.Beatmaps.Contains(b)))
+                    resetIds();
+            }
+
+            void resetIds() => beatmapSet.Beatmaps.ForEach(b => b.OnlineBeatmapID = null);
         }
 
         /// <summary>
@@ -254,6 +273,18 @@ 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 CanUndelete(BeatmapSetInfo existing, BeatmapSetInfo import)
+        {
+            if (!base.CanUndelete(existing, import))
+                return false;
+
+            var existingIds = existing.Beatmaps.Select(b => b.OnlineBeatmapID).OrderBy(i => i);
+            var importIds = import.Beatmaps.Select(b => b.OnlineBeatmapID).OrderBy(i => i);
+
+            // force re-import if we are not in a sane state.
+            return existing.OnlineBeatmapSetID == import.OnlineBeatmapSetID && existingIds.SequenceEqual(importIds);
+        }
+
         /// <summary>
         /// Returns a list of all usable <see cref="BeatmapSetInfo"/>s.
         /// </summary>

osu.Game/Database/ArchiveModelManager.cs

@@ -300,20 +300,30 @@ namespace osu.Game.Database
                     {
                         if (!write.IsTransactionLeader) throw new InvalidOperationException($"Ensure there is no parent transaction so errors can correctly be handled by {this}");
 
+                        if (archive != null)
+                            item.Files = createFileInfos(archive, Files);
+
+                        Populate(item, archive);
+
                         var existing = CheckForExisting(item);
 
                         if (existing != null)
+                        {
+                            if (CanUndelete(existing, item))
                             {
                                 Undelete(existing);
                                 Logger.Log($"Found existing {typeof(TModel)} for {item} (ID {existing.ID}). Skipping import.", LoggingTarget.Database);
                                 handleEvent(() => ItemAdded?.Invoke(existing, true));
                                 return existing;
                             }
+                            else
+                            {
+                                Delete(existing);
+                                ModelStore.PurgeDeletable(s => s.ID == existing.ID);
+                            }
+                        }
 
-                        if (archive != null)
+                        PreImport(item);
-                            item.Files = createFileInfos(archive, Files);
-
-                        Populate(item, archive);
 
                         // import to store
                         ModelStore.Add(item);
@@ -542,12 +552,29 @@ namespace osu.Game.Database
         {
         }
 
+        /// <summary>
+        /// Perform any final actions before the import to database executes.
+        /// </summary>
+        /// <param name="model">The model prepared for import.</param>
+        protected virtual void PreImport(TModel model)
+        {
+        }
+
         /// <summary>
         /// Check whether an existing model already exists for a new import item.
         /// </summary>
-        /// <param name="model">The new model proposed for import. Note that <see cref="Populate"/> has not yet been run on this model.</param>
+        /// <param name="model">The new model proposed for import.
         /// <returns>An existing model which matches the criteria to skip importing, else null.</returns>
-        protected virtual TModel CheckForExisting(TModel model) => model.Hash == null ? null : ModelStore.ConsumableItems.FirstOrDefault(b => b.Hash == model.Hash);
+        protected TModel CheckForExisting(TModel model) => model.Hash == null ? null : ModelStore.ConsumableItems.FirstOrDefault(b => b.Hash == model.Hash);
+
+        /// <summary>
+        /// After an existing <see cref="TModel"/> is found during an import process, the default behaviour is to restore the existing
+        /// item and skip the import. This method allows changing that behaviour.
+        /// </summary>
+        /// <param name="existing">The existing model.</param>
+        /// <param name="import">The newly imported model.</param>
+        /// <returns>Whether the existing model should be restored and used. Returning false will delete the existing a force a re-import.</returns>
+        protected virtual bool CanUndelete(TModel existing, TModel import) => true;
 
         private DbSet<TModel> queryModel() => ContextFactory.Get().Set<TModel>();