h1or4dme8/osu-lazer
1
0
Fork 0
mirror of https://github.com/ppy/osu.git synced 2024-12-14 10:12:54 +08:00
Code Issues Releases Wiki Activity

Merge pull request #19029 from peppy/fix-skin-import-duplicate

Browse Source 
Fix skins potentially being duplicated on batch import
This commit is contained in:
Dan Balasescu 2022-07-08 14:18:14 +09:00 committed by GitHub
commit 0bc332e90b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 18 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
osu.Game.Tests/Skins/IO/ImportSkinTest.cs
View File

@ -83,20 +83,20 @@ namespace osu.Game.Tests.Skins.IO
#region Cases where imports should match existing #region Cases where imports should match existing
[Test] [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 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")); var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("test skin", "skinner"), "skin.osk"), batchImport);
assertImportedOnce(import1, import2); assertImportedOnce(import1, import2);
}); });
[Test] [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. // 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 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")); var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni(string.Empty, string.Empty), "download.osk"), batchImport);
assertImportedOnce(import1, import2); assertImportedOnce(import1, import2);
}); });
@ -134,10 +134,10 @@ namespace osu.Game.Tests.Skins.IO
}); });
[Test] [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 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")); var import2 = await loadSkinIntoOsu(osu, new ImportTask(createOskWithIni("name 1", "author 1"), "my custom skin 1"), batchImport);
assertImportedOnce(import1, import2); assertImportedOnce(import1, import2);
assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu); assertCorrectMetadata(import1, "name 1 [my custom skin 1]", "author 1", osu);
@ -357,10 +357,10 @@ namespace osu.Game.Tests.Skins.IO
} }
} }
private async Task<Live<SkinInfo>> loadSkinIntoOsu(OsuGameBase osu, ImportTask import) private async Task<Live<SkinInfo>> loadSkinIntoOsu(OsuGameBase osu, ImportTask import, bool batchImport = false)
{ {
var skinManager = osu.Dependencies.Get<SkinManager>(); var skinManager = osu.Dependencies.Get<SkinManager>();
return await skinManager.Import(import); return await skinManager.Import(import, batchImport);
} }
} }
} }

12
osu.Game/Database/RealmArchiveModelImporter.cs
View File

@ -258,15 +258,13 @@ namespace osu.Game.Database
{ {
cancellationToken.ThrowIfCancellationRequested(); cancellationToken.ThrowIfCancellationRequested();
bool checkedExisting = false; TModel? existing;
TModel? existing = null;
if (batchImport && archive != null) if (batchImport && archive != null)
{ {
// this is a fast bail condition to improve large import performance. // this is a fast bail condition to improve large import performance.
item.Hash = computeHashFast(archive); item.Hash = computeHashFast(archive);
checkedExisting = true;
existing = CheckForExisting(item, realm); existing = CheckForExisting(item, realm);
if (existing != null) if (existing != null)
@ -311,8 +309,12 @@ namespace osu.Game.Database
// TODO: we may want to run this outside of the transaction. // TODO: we may want to run this outside of the transaction.
Populate(item, archive, realm, cancellationToken); Populate(item, archive, realm, cancellationToken);
if (!checkedExisting) // Populate() may have adjusted file content (see SkinImporter.updateSkinIniMetadata), so regardless of whether a fast check was done earlier, let's
existing = CheckForExisting(item, realm); // 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) if (existing != null)
{ {