From 2b0fd3558f2f7f1c6a79dbba62ab80f40d656ca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 30 Oct 2024 08:44:23 +0100 Subject: [PATCH] Remove more no-longer-required checks The scenario that remaining guard was trying to protect against is obviated by and no longer possible after 776fabd77c5a6225e69a0b14e79b9e097692320c. --- .../BeatmapUpdaterMetadataLookupTest.cs | 28 ------------------- .../Beatmaps/BeatmapUpdaterMetadataLookup.cs | 24 ++++------------ 2 files changed, 5 insertions(+), 47 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs index 2e1bb3a1c6..82e54875ef 100644 --- a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs +++ b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs @@ -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(), 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) { diff --git a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs b/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs index 42d8e07432..dd17ee784b 100644 --- a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs +++ b/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,14 @@ namespace osu.Game.Beatmaps foreach (var beatmapInfo in beatmapSet.Beatmaps) { + // note that it is KEY that the lookup here only uses MD5 inside + // (see implementations of underlying `IOnlineBeatmapMetadataSource`s). + // anything else like online ID or filename can be manipulated, thus being inherently unreliable, + // thus being unusable here for any purpose. 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 +86,6 @@ namespace osu.Game.Beatmaps } } - private bool shouldDiscardLookupResult(OnlineBeatmapMetadata result, BeatmapInfo beatmapInfo) - { - // previously this used to check whether the `OnlineID` of the looked-up beatmap matches the local `OnlineID`. - // unfortunately it appears that historically stable mappers would apply crude hacks to fix unspecified "issues" with submission - // which would amount to reusing online IDs of other beatmaps. - // this means that the online ID in the `.osu` file is not reliable, and this cannot be fixed server-side - // because updating the maps retroactively would break stable (by losing all of users' local scores). - - 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; - } - /// /// Attempts to retrieve the for the given . ///