Merge pull request #30453 from bdach/everything-is-terrible

Fix several issues with beatmap online ID management

osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs

@@ -241,8 +241,8 @@ namespace osu.Game.Tests.Beatmaps
 
             metadataLookup.Update(beatmapSet, preferOnlineFetch);
 
-            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
-            Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
+            Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
         }
 
         [Test]
@@ -273,34 +273,6 @@ namespace osu.Game.Tests.Beatmaps
             Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
         }
 
-        [Test]
-        public void TestMetadataLookupForBeatmapWithoutPopulatedIDAndIncorrectHash([Values] bool preferOnlineFetch)
-        {
-            var lookupResult = new OnlineBeatmapMetadata
-            {
-                BeatmapID = 654321,
-                BeatmapStatus = BeatmapOnlineStatus.Ranked,
-                MD5Hash = @"cafebabe",
-            };
-
-            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
-            targetMock.Setup(src => src.Available).Returns(true);
-            targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
-                      .Returns(true);
-
-            var beatmap = new BeatmapInfo
-            {
-                MD5Hash = @"deadbeef"
-            };
-            var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
-            beatmap.BeatmapSet = beatmapSet;
-
-            metadataLookup.Update(beatmapSet, preferOnlineFetch);
-
-            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
-            Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
-        }
-
         [Test]
         public void TestReturnedMetadataHasDifferentHash([Values] bool preferOnlineFetch)
         {
@@ -383,58 +355,5 @@ namespace osu.Game.Tests.Beatmaps
 
             Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
         }
-
-        [Test]
-        public void TestPartiallyMaliciousSet([Values] bool preferOnlineFetch)
-        {
-            var firstResult = new OnlineBeatmapMetadata
-            {
-                BeatmapID = 654321,
-                BeatmapStatus = BeatmapOnlineStatus.Ranked,
-                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
-                MD5Hash = @"cafebabe"
-            };
-            var secondResult = new OnlineBeatmapMetadata
-            {
-                BeatmapStatus = BeatmapOnlineStatus.Ranked,
-                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
-                MD5Hash = @"dededede"
-            };
-
-            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
-            targetMock.Setup(src => src.Available).Returns(true);
-            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 654321), out firstResult))
-                      .Returns(true);
-            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 666666), out secondResult))
-                      .Returns(true);
-
-            var firstBeatmap = new BeatmapInfo
-            {
-                OnlineID = 654321,
-                MD5Hash = @"cafebabe",
-            };
-            var secondBeatmap = new BeatmapInfo
-            {
-                OnlineID = 666666,
-                MD5Hash = @"deadbeef"
-            };
-            var beatmapSet = new BeatmapSetInfo(new[]
-            {
-                firstBeatmap,
-                secondBeatmap
-            });
-            firstBeatmap.BeatmapSet = beatmapSet;
-            secondBeatmap.BeatmapSet = beatmapSet;
-
-            metadataLookup.Update(beatmapSet, preferOnlineFetch);
-
-            Assert.That(firstBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
-            Assert.That(firstBeatmap.OnlineID, Is.EqualTo(654321));
-
-            Assert.That(secondBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
-            Assert.That(secondBeatmap.OnlineID, Is.EqualTo(-1));
-
-            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
-        }
     }
 }

osu.Game/Beatmaps/APIBeatmapMetadataSource.cs

@@ -33,7 +33,7 @@ namespace osu.Game.Beatmaps
 
             Debug.Assert(beatmapInfo.BeatmapSet != null);
 
-            var req = new GetBeatmapRequest(beatmapInfo);
+            var req = new GetBeatmapRequest(md5Hash: beatmapInfo.MD5Hash, filename: beatmapInfo.Path);
 
             try
             {

osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs

@@ -5,7 +5,6 @@ using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
-using osu.Framework.Logging;
 using osu.Framework.Platform;
 using osu.Game.Online.API;
 
@@ -44,10 +43,19 @@ namespace osu.Game.Beatmaps
 
             foreach (var beatmapInfo in beatmapSet.Beatmaps)
             {
+                // note that these lookups DO NOT ACTUALLY FULLY GUARANTEE that the beatmap is what it claims it is,
+                // i.e. the correctness of this lookup should be treated as APPROXIMATE AT WORST.
+                // this is because the beatmap filename is used as a fallback in some scenarios where the MD5 of the beatmap may mismatch.
+                // this is considered to be an acceptable casualty so that things can continue to work as expected for users in some rare scenarios
+                // (stale beatmap files in beatmap packs, beatmap mirror desyncs).
+                // however, all this means that other places such as score submission ARE EXPECTED TO VERIFY THE MD5 OF THE BEATMAP AGAINST THE ONLINE ONE EXPLICITLY AGAIN.
+                //
+                // additionally note that the online ID stored to the map is EXPLICITLY NOT USED because some users in a silly attempt to "fix" things for themselves on stable
+                // would reuse online IDs of already submitted beatmaps, which means that information is pretty much expected to be bogus in a nonzero number of beatmapsets.
                 if (!tryLookup(beatmapInfo, preferOnlineFetch, out var res))
                     continue;
 
-                if (res == null || shouldDiscardLookupResult(res, beatmapInfo))
+                if (res == null)
                 {
                     beatmapInfo.ResetOnlineInfo();
                     lookupResults.Add(null); // mark lookup failure
@@ -83,23 +91,6 @@ namespace osu.Game.Beatmaps
             }
         }
 
-        private bool shouldDiscardLookupResult(OnlineBeatmapMetadata result, BeatmapInfo beatmapInfo)
-        {
-            if (beatmapInfo.OnlineID > 0 && result.BeatmapID != beatmapInfo.OnlineID)
-            {
-                Logger.Log($"Discarding metadata lookup result due to mismatching online ID (expected: {beatmapInfo.OnlineID} actual: {result.BeatmapID})", LoggingTarget.Database);
-                return true;
-            }
-
-            if (beatmapInfo.OnlineID == -1 && result.MD5Hash != beatmapInfo.MD5Hash)
-            {
-                Logger.Log($"Discarding metadata lookup result due to mismatching hash (expected: {beatmapInfo.MD5Hash} actual: {result.MD5Hash})", LoggingTarget.Database);
-                return true;
-            }
-
-            return false;
-        }
-
         /// <summary>
         /// Attempts to retrieve the <see cref="OnlineBeatmapMetadata"/> for the given <paramref name="beatmapInfo"/>.
         /// </summary>

osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs

@@ -90,8 +90,7 @@ namespace osu.Game.Beatmaps
             }
 
             if (string.IsNullOrEmpty(beatmapInfo.MD5Hash)
-                && string.IsNullOrEmpty(beatmapInfo.Path)
+                && string.IsNullOrEmpty(beatmapInfo.Path))
-                && beatmapInfo.OnlineID <= 0)
             {
                 onlineMetadata = null;
                 return false;
@@ -240,10 +239,9 @@ namespace osu.Game.Beatmaps
             using var cmd = db.CreateCommand();
 
             cmd.CommandText =
-                @"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash OR beatmap_id = @OnlineID OR filename = @Path";
+                @"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash OR filename = @Path";
 
             cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
-            cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
             cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
 
             using var reader = cmd.ExecuteReader();
@@ -281,11 +279,10 @@ namespace osu.Game.Beatmaps
                 SELECT `b`.`beatmapset_id`, `b`.`beatmap_id`, `b`.`approved`, `b`.`user_id`, `b`.`checksum`, `b`.`last_update`, `s`.`submit_date`, `s`.`approved_date`
                 FROM `osu_beatmaps` AS `b`
                 JOIN `osu_beatmapsets` AS `s` ON `s`.`beatmapset_id` = `b`.`beatmapset_id`
-                WHERE `b`.`checksum` = @MD5Hash OR `b`.`beatmap_id` = @OnlineID OR `b`.`filename` = @Path
+                WHERE `b`.`checksum` = @MD5Hash OR `b`.`filename` = @Path
                 """;
 
             cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
-            cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
             cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
 
             using var reader = cmd.ExecuteReader();

osu.Game/Database/RealmArchiveModelImporter.cs

@@ -528,7 +528,7 @@ namespace osu.Game.Database
         /// <param name="model">The new model proposed for import.</param>
         /// <param name="realm">The current realm context.</param>
         /// <returns>An existing model which matches the criteria to skip importing, else null.</returns>
-        protected TModel? CheckForExisting(TModel model, Realm realm) => string.IsNullOrEmpty(model.Hash) ? null : realm.All<TModel>().FirstOrDefault(b => b.Hash == model.Hash);
+        protected TModel? CheckForExisting(TModel model, Realm realm) => string.IsNullOrEmpty(model.Hash) ? null : realm.All<TModel>().OrderBy(b => b.DeletePending).FirstOrDefault(b => b.Hash == model.Hash);
 
         /// <summary>
         /// Whether import can be skipped after finding an existing import early in the process.

osu.Game/Online/API/Requests/GetBeatmapRequest.cs

@@ -1,6 +1,7 @@
 // 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.Globalization;
 using osu.Framework.IO.Network;
 using osu.Game.Beatmaps;
 using osu.Game.Online.API.Requests.Responses;
@@ -9,23 +10,30 @@ namespace osu.Game.Online.API.Requests
 {
     public class GetBeatmapRequest : APIRequest<APIBeatmap>
     {
-        public readonly IBeatmapInfo BeatmapInfo;
+        public readonly int OnlineID;
-        public readonly string Filename;
+        public readonly string? MD5Hash;
+        public readonly string? Filename;
 
         public GetBeatmapRequest(IBeatmapInfo beatmapInfo)
+            : this(onlineId: beatmapInfo.OnlineID, md5Hash: beatmapInfo.MD5Hash, filename: (beatmapInfo as BeatmapInfo)?.Path)
         {
-            BeatmapInfo = beatmapInfo;
+        }
-            Filename = (beatmapInfo as BeatmapInfo)?.Path ?? string.Empty;
+
+        public GetBeatmapRequest(int onlineId = -1, string? md5Hash = null, string? filename = null)
+        {
+            OnlineID = onlineId;
+            MD5Hash = md5Hash;
+            Filename = filename;
         }
 
         protected override WebRequest CreateWebRequest()
         {
             var request = base.CreateWebRequest();
 
-            if (BeatmapInfo.OnlineID > 0)
+            if (OnlineID > 0)
-                request.AddParameter(@"id", BeatmapInfo.OnlineID.ToString());
+                request.AddParameter(@"id", OnlineID.ToString(CultureInfo.InvariantCulture));
-            if (!string.IsNullOrEmpty(BeatmapInfo.MD5Hash))
+            if (!string.IsNullOrEmpty(MD5Hash))
-                request.AddParameter(@"checksum", BeatmapInfo.MD5Hash);
+                request.AddParameter(@"checksum", MD5Hash);
             if (!string.IsNullOrEmpty(Filename))
                 request.AddParameter(@"filename", Filename);
 

osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs

@@ -188,7 +188,7 @@ namespace osu.Game.Tests.Visual.OnlinePlay
 
                 case GetBeatmapRequest getBeatmapRequest:
                 {
-                    getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.BeatmapInfo.OnlineID).Single());
+                    getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.OnlineID).Single());
                     return true;
                 }