From c155ab83399a51bdab44a48d5805463c8520318a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Jun 2020 18:03:10 +0900 Subject: [PATCH 1/5] Check filenames and timestamps before reusing an already imported model --- osu.Game/Beatmaps/BeatmapManager.cs | 4 ++-- osu.Game/Database/ArchiveModelManager.cs | 22 +++++++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index f626b45e42..e7cef13c68 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -258,9 +258,9 @@ namespace osu.Game.Beatmaps /// The first result for the provided query, or null if no results were found. public BeatmapSetInfo QueryBeatmapSet(Expression> query) => beatmaps.ConsumableItems.AsNoTracking().FirstOrDefault(query); - protected override bool CanUndelete(BeatmapSetInfo existing, BeatmapSetInfo import) + protected override bool CanReuseExisting(BeatmapSetInfo existing, BeatmapSetInfo import) { - if (!base.CanUndelete(existing, import)) + if (!base.CanReuseExisting(existing, import)) return false; var existingIds = existing.Beatmaps.Select(b => b.OnlineBeatmapID).OrderBy(i => i); diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index ae55a7b14a..4d7d3e96e6 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -332,7 +332,7 @@ namespace osu.Game.Database if (existing != null) { - if (CanUndelete(existing, item)) + if (CanReuseExisting(existing, item)) { Undelete(existing); LogForModel(item, $"Found existing {HumanisedModelName} for {item} (ID {existing.ID}) – skipping import."); @@ -660,13 +660,29 @@ namespace osu.Game.Database protected TModel CheckForExisting(TModel model) => model.Hash == null ? null : ModelStore.ConsumableItems.FirstOrDefault(b => b.Hash == model.Hash); /// - /// After an existing is found during an import process, the default behaviour is to restore the existing + /// After an existing 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. /// /// The existing model. /// The newly imported model. /// Whether the existing model should be restored and used. Returning false will delete the existing and force a re-import. - protected virtual bool CanUndelete(TModel existing, TModel import) => true; + protected virtual bool CanReuseExisting(TModel existing, TModel import) => + getFilenames(existing.Files).SequenceEqual(getFilenames(import.Files)) && + // poor-man's (cheap) equality comparison, avoiding hashing unnecessarily. + // can switch to full hash checks on a per-case basis (or for all) if we decide this is not a performance issue. + getTimestamps(existing.Files).SequenceEqual(getTimestamps(import.Files)); + + private IEnumerable getFilenames(List files) + { + foreach (var f in files.OrderBy(f => f.Filename)) + yield return f.Filename; + } + + private IEnumerable getTimestamps(List files) + { + foreach (var f in files.OrderBy(f => f.Filename)) + yield return File.GetLastWriteTimeUtc(Files.Storage.GetFullPath(f.FileInfo.StoragePath)).ToFileTime(); + } private DbSet queryModel() => ContextFactory.Get().Set(); From 012933545eccd4510dc3c0a70b040610dad0bf8d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Jun 2020 18:30:27 +0900 Subject: [PATCH 2/5] Add test coverage --- .../Beatmaps/IO/ImportBeatmapTest.cs | 161 ++++++++++++++++++ osu.Game/Database/ArchiveModelManager.cs | 2 +- 2 files changed, 162 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 5eb11a3264..12c9c92e90 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -12,6 +12,7 @@ using NUnit.Framework; using osu.Framework.Platform; using osu.Game.IPC; using osu.Framework.Allocation; +using osu.Framework.Extensions; using osu.Framework.Logging; using osu.Game.Beatmaps; using osu.Game.Beatmaps.Formats; @@ -22,6 +23,7 @@ using SharpCompress.Archives; using SharpCompress.Archives.Zip; using SharpCompress.Common; using SharpCompress.Writers.Zip; +using FileInfo = System.IO.FileInfo; namespace osu.Game.Tests.Beatmaps.IO { @@ -93,6 +95,165 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public async Task TestImportThenImportWithReZip() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithNewerTimestamp))) + { + try + { + var osu = loadOsu(host); + + var temp = TestResources.GetTestBeatmapForImport(); + + string extractedFolder = $"{temp}_extracted"; + Directory.CreateDirectory(extractedFolder); + + try + { + var imported = await LoadOszIntoOsu(osu); + + string hashBefore = hashFile(temp); + + using (var zip = ZipArchive.Open(temp)) + zip.WriteToDirectory(extractedFolder); + + using (var zip = ZipArchive.Create()) + { + zip.AddAllFromDirectory(extractedFolder); + zip.SaveTo(temp, new ZipWriterOptions(CompressionType.Deflate)); + } + + // zip files differ because different compression or encoder. + Assert.AreNotEqual(hashBefore, hashFile(temp)); + + var importedSecondTime = await osu.Dependencies.Get().Import(temp); + + ensureLoaded(osu); + + // but contents doesn't, so existing should still be used. + Assert.IsTrue(imported.ID == importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID == importedSecondTime.Beatmaps.First().ID); + } + finally + { + Directory.Delete(extractedFolder, true); + } + } + finally + { + host.Exit(); + } + } + } + + private string hashFile(string filename) + { + using (var s = File.OpenRead(filename)) + return s.ComputeMD5Hash(); + } + + [Test] + public async Task TestImportThenImportWithNewerTimestamp() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithNewerTimestamp))) + { + try + { + var osu = loadOsu(host); + + var temp = TestResources.GetTestBeatmapForImport(); + + string extractedFolder = $"{temp}_extracted"; + Directory.CreateDirectory(extractedFolder); + + try + { + var imported = await LoadOszIntoOsu(osu); + + using (var zip = ZipArchive.Open(temp)) + zip.WriteToDirectory(extractedFolder); + + // change timestamp + new FileInfo(Directory.GetFiles(extractedFolder).First()).LastWriteTime = DateTime.Now; + + using (var zip = ZipArchive.Create()) + { + zip.AddAllFromDirectory(extractedFolder); + zip.SaveTo(temp, new ZipWriterOptions(CompressionType.Deflate)); + } + + var importedSecondTime = await osu.Dependencies.Get().Import(temp); + + ensureLoaded(osu); + + // check the newly "imported" beatmap is not the original. + Assert.IsTrue(imported.ID != importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID != importedSecondTime.Beatmaps.First().ID); + } + finally + { + Directory.Delete(extractedFolder, true); + } + } + finally + { + host.Exit(); + } + } + } + + [Test] + public async Task TestImportThenImportWithDifferentFilename() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithDifferentFilename))) + { + try + { + var osu = loadOsu(host); + + var temp = TestResources.GetTestBeatmapForImport(); + + string extractedFolder = $"{temp}_extracted"; + Directory.CreateDirectory(extractedFolder); + + try + { + var imported = await LoadOszIntoOsu(osu); + + using (var zip = ZipArchive.Open(temp)) + zip.WriteToDirectory(extractedFolder); + + // change filename + var firstFile = new FileInfo(Directory.GetFiles(extractedFolder).First()); + firstFile.MoveTo(Path.Combine(firstFile.DirectoryName, $"{firstFile.Name}-changed{firstFile.Extension}")); + + using (var zip = ZipArchive.Create()) + { + zip.AddAllFromDirectory(extractedFolder); + zip.SaveTo(temp, new ZipWriterOptions(CompressionType.Deflate)); + } + + var importedSecondTime = await osu.Dependencies.Get().Import(temp); + + ensureLoaded(osu); + + // check the newly "imported" beatmap is not the original. + Assert.IsTrue(imported.ID != importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID != importedSecondTime.Beatmaps.First().ID); + } + finally + { + Directory.Delete(extractedFolder, true); + } + } + finally + { + host.Exit(); + } + } + } + [Test] public async Task TestImportCorruptThenImport() { diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 4d7d3e96e6..5ca9423de2 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -276,7 +276,7 @@ namespace osu.Game.Database // for now, concatenate all .osu files in the set to create a unique hash. MemoryStream hashable = new MemoryStream(); - foreach (TFileModel file in item.Files.Where(f => HashableFileTypes.Any(f.Filename.EndsWith))) + foreach (TFileModel file in item.Files.Where(f => HashableFileTypes.Any(f.Filename.EndsWith)).OrderBy(f => f.Filename)) { using (Stream s = Files.Store.GetStream(file.FileInfo.StoragePath)) s.CopyTo(hashable); From 25160dc220d9f2f0bde4125f0bafd7446e3ad354 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Jun 2020 19:15:52 +0900 Subject: [PATCH 3/5] Fix test name --- osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 12c9c92e90..12f06059f7 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -98,7 +98,7 @@ namespace osu.Game.Tests.Beatmaps.IO [Test] public async Task TestImportThenImportWithReZip() { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithNewerTimestamp))) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithReZip))) { try { From 5ed3cd205f068d572caddc0ae01aa7ab8a580a6a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Jun 2020 22:35:01 +0900 Subject: [PATCH 4/5] Simplify reuse check using FileInfo IDs --- .../Beatmaps/IO/ImportBeatmapTest.cs | 9 +++++---- osu.Game/Database/ArchiveModelManager.cs | 20 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 12f06059f7..9b34eece5f 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -154,9 +154,9 @@ namespace osu.Game.Tests.Beatmaps.IO } [Test] - public async Task TestImportThenImportWithNewerTimestamp() + public async Task TestImportThenImportWithChangedFile() { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithNewerTimestamp))) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportWithChangedFile))) { try { @@ -174,8 +174,9 @@ namespace osu.Game.Tests.Beatmaps.IO using (var zip = ZipArchive.Open(temp)) zip.WriteToDirectory(extractedFolder); - // change timestamp - new FileInfo(Directory.GetFiles(extractedFolder).First()).LastWriteTime = DateTime.Now; + // arbitrary write to non-hashed file + using (var sw = new FileInfo(Directory.GetFiles(extractedFolder, "*.mp3").First()).AppendText()) + sw.WriteLine("text"); using (var zip = ZipArchive.Create()) { diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index 5ca9423de2..0fe8dd1268 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -667,10 +667,16 @@ namespace osu.Game.Database /// The newly imported model. /// Whether the existing model should be restored and used. Returning false will delete the existing and force a re-import. protected virtual bool CanReuseExisting(TModel existing, TModel import) => - getFilenames(existing.Files).SequenceEqual(getFilenames(import.Files)) && - // poor-man's (cheap) equality comparison, avoiding hashing unnecessarily. - // can switch to full hash checks on a per-case basis (or for all) if we decide this is not a performance issue. - getTimestamps(existing.Files).SequenceEqual(getTimestamps(import.Files)); + // for the best or worst, we copy and import files of a new import before checking whether + // it is a duplicate. so to check if anything has changed, we can just compare all FileInfo IDs. + getIDs(existing.Files).SequenceEqual(getIDs(import.Files)) && + getFilenames(existing.Files).SequenceEqual(getFilenames(import.Files)); + + private IEnumerable getIDs(List files) + { + foreach (var f in files.OrderBy(f => f.Filename)) + yield return f.FileInfo.ID; + } private IEnumerable getFilenames(List files) { @@ -678,12 +684,6 @@ namespace osu.Game.Database yield return f.Filename; } - private IEnumerable getTimestamps(List files) - { - foreach (var f in files.OrderBy(f => f.Filename)) - yield return File.GetLastWriteTimeUtc(Files.Storage.GetFullPath(f.FileInfo.StoragePath)).ToFileTime(); - } - private DbSet queryModel() => ContextFactory.Get().Set(); protected virtual string HumanisedModelName => $"{typeof(TModel).Name.Replace("Info", "").ToLower()}"; From 66ec2afe5cdcd9eb77adf5015966e5dcb652c1d1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Jun 2020 23:38:40 +0900 Subject: [PATCH 5/5] Remove broken import test --- .../Beatmaps/IO/ImportBeatmapTest.cs | 33 +------------------ 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 9b34eece5f..546bf758c1 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -374,37 +374,6 @@ namespace osu.Game.Tests.Beatmaps.IO } } - [Test] - public async Task TestImportThenImportDifferentHash() - { - // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportDifferentHash))) - { - try - { - var osu = loadOsu(host); - var manager = osu.Dependencies.Get(); - - var imported = await LoadOszIntoOsu(osu); - - imported.Hash += "-changed"; - manager.Update(imported); - - var importedSecondTime = await LoadOszIntoOsu(osu); - - Assert.IsTrue(imported.ID != importedSecondTime.ID); - Assert.IsTrue(imported.Beatmaps.First().ID < importedSecondTime.Beatmaps.First().ID); - - // only one beatmap will exist as the online set ID matched, causing purging of the first import. - checkBeatmapSetCount(osu, 1); - } - finally - { - host.Exit(); - } - } - } - [Test] public async Task TestImportThenDeleteThenImport() {