From cf1da1dd18bc7d9e3824acae4c94b53f45eebd4a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 7 Jul 2022 22:38:54 +0900 Subject: [PATCH 1/2] Fix skins potentially being duplicated on batch import Resolves https://github.com/ppy/osu/discussions/19024#discussioncomment-3099200 --- osu.Game/Database/RealmArchiveModelImporter.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index 76f6db1384..ce3d0652b3 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -258,15 +258,13 @@ namespace osu.Game.Database { cancellationToken.ThrowIfCancellationRequested(); - bool checkedExisting = false; - TModel? existing = null; + TModel? existing; if (batchImport && archive != null) { // this is a fast bail condition to improve large import performance. item.Hash = computeHashFast(archive); - checkedExisting = true; existing = CheckForExisting(item, realm); if (existing != null) @@ -311,8 +309,12 @@ namespace osu.Game.Database // TODO: we may want to run this outside of the transaction. Populate(item, archive, realm, cancellationToken); - if (!checkedExisting) - existing = CheckForExisting(item, realm); + // Populate() may have adjusted file content (see SkinImporter.updateSkinIniMetadata), so regardless of whether a fast check was done earlier, let's + // check for existing items a second time. + // + // If this is ever a performance issue, the fast-check hash can be compared and trigger a skip of this second check if it still matches. + // I don't think it is a huge deal doing a second indexed check, though. + existing = CheckForExisting(item, realm); if (existing != null) { From cd4755fbd9151bde922444827d63550cdfe7b9bd Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 7 Jul 2022 18:06:32 +0300 Subject: [PATCH 2/2] Add test coverage for batch-import path --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 8b7fcae1a9..c3c10215a5 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -83,20 +83,20 @@ namespace osu.Game.Tests.Skins.IO #region Cases where imports should match existing [Test] - public Task TestImportTwiceWithSameMetadataAndFilename() => runSkinTest(async osu => + public Task TestImportTwiceWithSameMetadataAndFilename([Values] bool batchImport) => runSkinTest(async osu => { - var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk"), batchImport); assertImportedOnce(import1, import2); }); [Test] - public Task TestImportTwiceWithNoMetadataSameDownloadFilename() => runSkinTest(async osu => + public Task TestImportTwiceWithNoMetadataSameDownloadFilename([Values] bool batchImport) => runSkinTest(async osu => { // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety. - var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk"), batchImport); assertImportedOnce(import1, import2); }); @@ -134,10 +134,10 @@ namespace osu.Game.Tests.Skins.IO }); [Test] - public Task TestSameMetadataNameSameFolderName() => runSkinTest(async osu => + public Task TestSameMetadataNameSameFolderName([Values] bool batchImport) => runSkinTest(async osu => { - var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1")); - var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1")); + var import1 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1"), batchImport); + var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1"), batchImport); assertImportedOnce(import1, import2); assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu); @@ -357,10 +357,10 @@ namespace osu.Game.Tests.Skins.IO } } - private async Task> loadSkinIntoOsu(OsuGameBase osu, ImportTask import) + private async Task> loadSkinIntoOsu(OsuGameBase osu, ImportTask import, bool batchImport = false) { var skinManager = osu.Dependencies.Get(); - return await skinManager.Import(import); + return await skinManager.Import(import, batchImport); } } }