Merge pull request #10133 from peppy/better-skin-hashing

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

@@ -34,7 +34,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportWhenClosed()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -51,7 +51,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportThenDelete()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -72,7 +72,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportThenImport()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -98,7 +98,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportThenImportWithReZip()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -156,7 +156,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportThenImportWithChangedFile()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -207,7 +207,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportThenImportWithDifferentFilename()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -259,7 +259,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportCorruptThenImport()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -301,7 +301,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestRollbackOnFailure()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -378,7 +378,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportThenDeleteThenImport()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -406,7 +406,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task 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(set.ToString()))
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"{nameof(ImportBeatmapTest)}-{set}"))
             {
                 try
                 {
@@ -440,7 +440,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         public async Task TestImportWithDuplicateBeatmapIDs()
         {
             // unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here.
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -496,8 +496,8 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Ignore("Binding IPC on Appveyor isn't working (port in use). Need to figure out why")]
         public void TestImportOverIPC()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost("host", true))
-            using (HeadlessGameHost client = new CleanRunHeadlessGameHost("client", true))
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"{nameof(ImportBeatmapTest)}-host", true))
+            using (HeadlessGameHost client = new CleanRunHeadlessGameHost($"{nameof(ImportBeatmapTest)}-client", true))
             {
                 try
                 {
@@ -526,7 +526,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportWhenFileOpen()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -548,7 +548,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportWithDuplicateHashes()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -590,7 +590,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportNestedStructure()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -635,7 +635,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestImportWithIgnoredDirectoryInArchive()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -689,7 +689,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestUpdateBeatmapInfo()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -719,7 +719,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public async Task TestUpdateBeatmapFile()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -761,7 +761,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public void TestCreateNewEmptyBeatmap()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {
@@ -788,7 +788,7 @@ namespace osu.Game.Tests.Beatmaps.IO
         [Test]
         public void TestCreateNewBeatmapWithObject()
         {
-            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportBeatmapTest)))
             {
                 try
                 {

osu.Game.Tests/Skins/IO/ImportSkinTest.cs

@@ -0,0 +1,177 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using System.IO;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Platform;
+using osu.Game.Beatmaps;
+using osu.Game.IO.Archives;
+using osu.Game.Skinning;
+using osu.Game.Tests.Resources;
+using SharpCompress.Archives.Zip;
+
+namespace osu.Game.Tests.Skins.IO
+{
+    public class ImportSkinTest
+    {
+        [Test]
+        public async Task TestBasicImport()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest)))
+            {
+                try
+                {
+                    var osu = await loadOsu(host);
+
+                    var imported = await loadIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk"));
+
+                    Assert.That(imported.Name, Is.EqualTo("test skin"));
+                    Assert.That(imported.Creator, Is.EqualTo("skinner"));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
+        [Test]
+        public async Task TestImportTwiceWithSameMetadata()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest)))
+            {
+                try
+                {
+                    var osu = await loadOsu(host);
+
+                    var imported = await loadIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin.osk"));
+                    var imported2 = await loadIntoOsu(osu, new ZipArchiveReader(createOsk("test skin", "skinner"), "skin2.osk"));
+
+                    Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Count, Is.EqualTo(1));
+
+                    // the first should be overwritten by the second import.
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().First().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
+        [Test]
+        public async Task TestImportTwiceWithNoMetadata()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest)))
+            {
+                try
+                {
+                    var osu = await loadOsu(host);
+
+                    // 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 imported = await loadIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk"));
+                    var imported2 = await loadIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk"));
+
+                    Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Count, Is.EqualTo(2));
+
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().First().Files.First().FileInfoID, Is.EqualTo(imported.Files.First().FileInfoID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Last().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
+        [Test]
+        public async Task TestImportTwiceWithDifferentMetadata()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(ImportSkinTest)))
+            {
+                try
+                {
+                    var osu = await loadOsu(host);
+
+                    var imported = await loadIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2", "skinner"), "skin.osk"));
+                    var imported2 = await loadIntoOsu(osu, new ZipArchiveReader(createOsk("test skin v2.1", "skinner"), "skin2.osk"));
+
+                    Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Count, Is.EqualTo(2));
+
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().First().Files.First().FileInfoID, Is.EqualTo(imported.Files.First().FileInfoID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Last().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
+        private MemoryStream createOsk(string name, string author)
+        {
+            var zipStream = new MemoryStream();
+            using var zip = ZipArchive.Create();
+            zip.AddEntry("skin.ini", generateSkinIni(name, author));
+            zip.SaveTo(zipStream);
+            return zipStream;
+        }
+
+        private MemoryStream generateSkinIni(string name, string author)
+        {
+            var stream = new MemoryStream();
+            var writer = new StreamWriter(stream);
+
+            writer.WriteLine("[General]");
+            writer.WriteLine($"Name: {name}");
+            writer.WriteLine($"Author: {author}");
+            writer.WriteLine();
+            writer.WriteLine($"# unique {Guid.NewGuid()}");
+
+            writer.Flush();
+
+            return stream;
+        }
+
+        private async Task<SkinInfo> loadIntoOsu(OsuGameBase osu, ArchiveReader archive = null)
+        {
+            var skinManager = osu.Dependencies.Get<SkinManager>();
+            return await skinManager.Import(archive);
+        }
+
+        private async Task<OsuGameBase> loadOsu(GameHost host)
+        {
+            var osu = new OsuGameBase();
+
+#pragma warning disable 4014
+            Task.Run(() => host.Run(osu));
+#pragma warning restore 4014
+
+            waitForOrAssert(() => osu.IsLoaded, @"osu! failed to start in a reasonable amount of time");
+
+            var beatmapFile = TestResources.GetTestBeatmapForImport();
+            var beatmapManager = osu.Dependencies.Get<BeatmapManager>();
+            await beatmapManager.Import(beatmapFile);
+
+            return osu;
+        }
+
+        private void waitForOrAssert(Func<bool> result, string failureMessage, int timeout = 60000)
+        {
+            Task task = Task.Run(() =>
+            {
+                while (!result()) Thread.Sleep(200);
+            });
+
+            Assert.IsTrue(task.Wait(timeout), failureMessage);
+        }
+    }
+}

osu.Game/Database/ArchiveModelManager.cs

@@ -253,6 +253,9 @@ namespace osu.Game.Database
         /// Generally should include all file types which determine the file's uniqueness.
         /// Large files should be avoided if possible.
         /// </summary>
+        /// <remarks>
+        /// This is only used by the default hash implementation. If <see cref="ComputeHash"/> is overridden, it will not be used.
+        /// </remarks>
         protected abstract string[] HashableFileTypes { get; }
 
         internal static void LogForModel(TModel model, string message, Exception e = null)
@@ -271,7 +274,7 @@ namespace osu.Game.Database
         /// <remarks>
         ///  In the case of no matching files, a hash will be generated from the passed archive's <see cref="ArchiveReader.Name"/>.
         /// </remarks>
-        private string computeHash(TModel item, ArchiveReader reader = null)
+        protected virtual string ComputeHash(TModel item, ArchiveReader reader = null)
         {
             // for now, concatenate all .osu files in the set to create a unique hash.
             MemoryStream hashable = new MemoryStream();
@@ -318,7 +321,7 @@ namespace osu.Game.Database
                 LogForModel(item, "Beginning import...");
 
                 item.Files = archive != null ? createFileInfos(archive, Files) : new List<TFileModel>();
-                item.Hash = computeHash(item, archive);
+                item.Hash = ComputeHash(item, archive);
 
                 await Populate(item, archive, cancellationToken);
 
@@ -437,7 +440,7 @@ namespace osu.Game.Database
         {
             using (ContextFactory.GetForWrite())
             {
-                item.Hash = computeHash(item);
+                item.Hash = ComputeHash(item);
                 ModelStore.Update(item);
             }
         }

osu.Game/Skinning/SkinManager.cs

@@ -12,6 +12,7 @@ using Microsoft.EntityFrameworkCore;
 using osu.Framework.Audio;
 using osu.Framework.Audio.Sample;
 using osu.Framework.Bindables;
+using osu.Framework.Extensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.OpenGL.Textures;
 using osu.Framework.Graphics.Textures;
@@ -86,21 +87,46 @@ namespace osu.Game.Skinning
 
         protected override SkinInfo CreateModel(ArchiveReader archive) => new SkinInfo { Name = archive.Name };
 
+        private const string unknown_creator_string = "Unknown";
+
+        protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null)
+        {
+            // we need to populate early to create a hash based off skin.ini contents
+            if (item.Name?.Contains(".osk") == true)
+                populateMetadata(item);
+
+            if (item.Creator != null && item.Creator != unknown_creator_string)
+            {
+                // this is the optimal way to hash legacy skins, but will need to be reconsidered when we move forward with skin implementation.
+                // likely, the skin should expose a real version (ie. the version of the skin, not the skin.ini version it's targeting).
+                return item.ToString().ComputeSHA2Hash();
+            }
+
+            // if there was no creator, the ToString above would give the filename, which alone isn't really enough to base any decisions on.
+            return base.ComputeHash(item, reader);
+        }
+
         protected override async Task Populate(SkinInfo model, ArchiveReader archive, CancellationToken cancellationToken = default)
         {
             await base.Populate(model, archive, cancellationToken);
 
-            Skin reference = GetSkin(model);
+            if (model.Name?.Contains(".osk") == true)
+                populateMetadata(model);
+        }
+
+        private void populateMetadata(SkinInfo item)
+        {
+            Skin reference = GetSkin(item);
 
             if (!string.IsNullOrEmpty(reference.Configuration.SkinInfo.Name))
             {
-                model.Name = reference.Configuration.SkinInfo.Name;
-                model.Creator = reference.Configuration.SkinInfo.Creator;
+                item.Name = reference.Configuration.SkinInfo.Name;
+                item.Creator = reference.Configuration.SkinInfo.Creator;
             }
             else
             {
-                model.Name = model.Name.Replace(".osk", "");
-                model.Creator ??= "Unknown";
+                item.Name = item.Name.Replace(".osk", "");
+                item.Creator ??= unknown_creator_string;
             }
         }