Merge pull request #15590 from peppy/fix-failed-imports-thinking-correct

Fix failed imports being incorrectly considered as successfully importing for notification purposes

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

@@ -18,6 +18,7 @@ using osu.Game.Beatmaps;
 using osu.Game.Database;
 using osu.Game.IO;
 using osu.Game.Online.API.Requests.Responses;
+using osu.Game.Overlays.Notifications;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Objects;
 using osu.Game.Scoring;
@@ -387,6 +388,41 @@ namespace osu.Game.Tests.Beatmaps.IO
             }
         }
 
+        [Test]
+        public async Task TestModelCreationFailureDoesntReturn()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
+            {
+                try
+                {
+                    var osu = LoadOsuIntoHost(host);
+                    var importer = osu.Dependencies.Get<BeatmapManager>();
+
+                    var progressNotification = new ImportProgressNotification();
+
+                    var zipStream = new MemoryStream();
+
+                    using (var zip = ZipArchive.Create())
+                        zip.SaveTo(zipStream, new ZipWriterOptions(CompressionType.Deflate));
+
+                    var imported = await importer.Import(
+                        progressNotification,
+                        new ImportTask(zipStream, string.Empty)
+                    );
+
+                    checkBeatmapSetCount(osu, 0);
+                    checkBeatmapCount(osu, 0);
+
+                    Assert.IsEmpty(imported);
+                    Assert.AreEqual(ProgressNotificationState.Cancelled, progressNotification.State);
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
         [Test]
         public async Task TestRollbackOnFailure()
         {

osu.Game.Tests/Database/BeatmapImporterTests.cs

@@ -16,6 +16,7 @@ using osu.Game.Beatmaps;
 using osu.Game.Database;
 using osu.Game.IO.Archives;
 using osu.Game.Models;
+using osu.Game.Overlays.Notifications;
 using osu.Game.Stores;
 using osu.Game.Tests.Resources;
 using Realms;
@@ -367,6 +368,34 @@ namespace osu.Game.Tests.Database
             });
         }
 
+        [Test]
+        public void TestModelCreationFailureDoesntReturn()
+        {
+            RunTestWithRealmAsync(async (realmFactory, storage) =>
+            {
+                using var importer = new BeatmapImporter(realmFactory, storage);
+                using var store = new RealmRulesetStore(realmFactory, storage);
+
+                var progressNotification = new ImportProgressNotification();
+
+                var zipStream = new MemoryStream();
+
+                using (var zip = ZipArchive.Create())
+                    zip.SaveTo(zipStream, new ZipWriterOptions(CompressionType.Deflate));
+
+                var imported = await importer.Import(
+                    progressNotification,
+                    new ImportTask(zipStream, string.Empty)
+                );
+
+                checkBeatmapSetCount(realmFactory.Context, 0);
+                checkBeatmapCount(realmFactory.Context, 0);
+
+                Assert.IsEmpty(imported);
+                Assert.AreEqual(ProgressNotificationState.Cancelled, progressNotification.State);
+            });
+        }
+
         [Test]
         public void TestRollbackOnFailure()
         {

osu.Game/Database/ArchiveModelManager.cs

@@ -264,7 +264,7 @@ namespace osu.Game.Database
                 model = CreateModel(archive);
 
                 if (model == null)
-                    return Task.FromResult<ILive<TModel>>(new EntityFrameworkLive<TModel>(null));
+                    return Task.FromResult<ILive<TModel>>(null);
             }
             catch (TaskCanceledException)
             {

osu.Game/Database/IModelImporter.cs

@@ -7,6 +7,8 @@ using System.Threading.Tasks;
 using osu.Game.IO.Archives;
 using osu.Game.Overlays.Notifications;
 
+#nullable enable
+
 namespace osu.Game.Database
 {
     /// <summary>
@@ -26,7 +28,7 @@ namespace osu.Game.Database
         /// <param name="lowPriority">Whether this is a low priority import.</param>
         /// <param name="cancellationToken">An optional cancellation token.</param>
         /// <returns>The imported model, if successful.</returns>
-        Task<ILive<TModel>> Import(ImportTask task, bool lowPriority = false, CancellationToken cancellationToken = default);
+        Task<ILive<TModel>?> Import(ImportTask task, bool lowPriority = false, CancellationToken cancellationToken = default);
 
         /// <summary>
         /// Silently import an item from an <see cref="ArchiveReader"/>.
@@ -34,7 +36,7 @@ namespace osu.Game.Database
         /// <param name="archive">The archive to be imported.</param>
         /// <param name="lowPriority">Whether this is a low priority import.</param>
         /// <param name="cancellationToken">An optional cancellation token.</param>
-        Task<ILive<TModel>> Import(ArchiveReader archive, bool lowPriority = false, CancellationToken cancellationToken = default);
+        Task<ILive<TModel>?> Import(ArchiveReader archive, bool lowPriority = false, CancellationToken cancellationToken = default);
 
         /// <summary>
         /// Silently import an item from a <typeparamref name="TModel"/>.
@@ -43,7 +45,7 @@ namespace osu.Game.Database
         /// <param name="archive">An optional archive to use for model population.</param>
         /// <param name="lowPriority">Whether this is a low priority import.</param>
         /// <param name="cancellationToken">An optional cancellation token.</param>
-        Task<ILive<TModel>> Import(TModel item, ArchiveReader archive = null, bool lowPriority = false, CancellationToken cancellationToken = default);
+        Task<ILive<TModel>?> Import(TModel item, ArchiveReader? archive = null, bool lowPriority = false, CancellationToken cancellationToken = default);
 
         /// <summary>
         /// A user displayable name for the model type associated with this manager.