diff --git a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs index 84195f1e7c..11c4c54ea6 100644 --- a/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs +++ b/osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs @@ -28,7 +28,12 @@ namespace osu.Game.Tests.Beatmaps [Test] public void TestLocalCacheQueriedFirst() { - var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; + var localLookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 123456, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + BeatmapSetStatus = BeatmapOnlineStatus.Ranked, + }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) .Returns(true); @@ -42,6 +47,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: false); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); apiMetadataSourceMock.Verify(src => src.TryLookup(It.IsAny(), out It.Ref.IsAny!), Times.Never); } @@ -54,7 +60,12 @@ namespace osu.Game.Tests.Beatmaps localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) .Returns(false); - var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; + var onlineLookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 123456, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + BeatmapSetStatus = BeatmapOnlineStatus.Ranked, + }; apiMetadataSourceMock.Setup(src => src.Available).Returns(true); apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out onlineLookupResult)) .Returns(true); @@ -66,6 +77,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: false); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); } @@ -73,12 +85,22 @@ namespace osu.Game.Tests.Beatmaps [Test] public void TestPreferOnlineFetch() { - var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; + var localLookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 123456, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + BeatmapSetStatus = BeatmapOnlineStatus.Ranked, + }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) .Returns(true); - var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Graveyard }; + var onlineLookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 123456, + BeatmapStatus = BeatmapOnlineStatus.Graveyard, + BeatmapSetStatus = BeatmapOnlineStatus.Graveyard, + }; apiMetadataSourceMock.Setup(src => src.Available).Returns(true); apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out onlineLookupResult)) .Returns(true); @@ -90,6 +112,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: true); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Graveyard)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Graveyard)); localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Never); apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); } @@ -97,7 +120,12 @@ namespace osu.Game.Tests.Beatmaps [Test] public void TestPreferOnlineFetchFallsBackToLocalCacheIfOnlineSourceUnavailable() { - var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked }; + var localLookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 123456, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + BeatmapSetStatus = BeatmapOnlineStatus.Ranked, + }; localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true); localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny(), out localLookupResult)) .Returns(true); @@ -111,6 +139,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: true); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked)); localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Never); } @@ -135,6 +164,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch: false); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None)); Assert.That(beatmap.OnlineID, Is.EqualTo(-1)); localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref.IsAny!), Times.Once); @@ -163,6 +193,7 @@ namespace osu.Game.Tests.Beatmaps metadataLookup.Update(beatmapSet, preferOnlineFetch); Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None)); + Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None)); Assert.That(beatmap.OnlineID, Is.EqualTo(123456)); } @@ -193,5 +224,217 @@ namespace osu.Game.Tests.Beatmaps Assert.That(beatmap.OnlineID, Is.EqualTo(123456)); } + + [Test] + public void TestReturnedMetadataHasDifferentOnlineID([Values] bool preferOnlineFetch) + { + var lookupResult = new OnlineBeatmapMetadata { BeatmapID = 654321, BeatmapStatus = BeatmapOnlineStatus.Ranked }; + + 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 { OnlineID = 123456 }; + 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 TestMetadataLookupForBeatmapWithoutPopulatedIDAndCorrectHash([Values] bool preferOnlineFetch) + { + var lookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 654321, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + MD5Hash = @"deadbeef", + }; + + 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.Ranked)); + 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) + { + var lookupResult = new OnlineBeatmapMetadata + { + BeatmapID = 654321, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + MD5Hash = @"deadbeef" + }; + + 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 + { + OnlineID = 654321, + MD5Hash = @"cafebabe", + }; + 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(654321)); + } + + [Test] + public void TestPartiallyModifiedSet([Values] bool preferOnlineFetch) + { + var firstResult = new OnlineBeatmapMetadata + { + BeatmapID = 654321, + BeatmapStatus = BeatmapOnlineStatus.Ranked, + BeatmapSetStatus = BeatmapOnlineStatus.Ranked, + MD5Hash = @"cafebabe" + }; + var secondResult = new OnlineBeatmapMetadata + { + BeatmapID = 666666, + 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(bi => bi.OnlineID == 654321), out firstResult)) + .Returns(true); + targetMock.Setup(src => src.TryLookup(It.Is(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(666666)); + + 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(bi => bi.OnlineID == 654321), out firstResult)) + .Returns(true); + targetMock.Setup(src => src.TryLookup(It.Is(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)); + } } } diff --git a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs b/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs index b32310990c..f395718a93 100644 --- a/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs +++ b/osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using System.Diagnostics; using System.Linq; using osu.Framework.Platform; @@ -38,17 +39,22 @@ namespace osu.Game.Beatmaps /// Whether metadata from an online source should be preferred. If true, the local cache will be skipped to ensure the freshest data state possible. public void Update(BeatmapSetInfo beatmapSet, bool preferOnlineFetch) { + var lookupResults = new List(); + foreach (var beatmapInfo in beatmapSet.Beatmaps) { if (!tryLookup(beatmapInfo, preferOnlineFetch, out var res)) continue; - if (res == null) + if (res == null || shouldDiscardLookupResult(res, beatmapInfo)) { beatmapInfo.ResetOnlineInfo(); + lookupResults.Add(null); // mark lookup failure continue; } + lookupResults.Add(res); + beatmapInfo.OnlineID = res.BeatmapID; beatmapInfo.OnlineMD5Hash = res.MD5Hash; beatmapInfo.LastOnlineUpdate = res.LastUpdated; @@ -57,19 +63,34 @@ namespace osu.Game.Beatmaps beatmapInfo.BeatmapSet.OnlineID = res.BeatmapSetID; // Some metadata should only be applied if there's no local changes. - if (shouldSaveOnlineMetadata(beatmapInfo)) + if (beatmapInfo.MatchesOnlineVersion) { beatmapInfo.Status = res.BeatmapStatus; beatmapInfo.Metadata.Author.OnlineID = res.AuthorID; } - - if (beatmapInfo.BeatmapSet.Beatmaps.All(shouldSaveOnlineMetadata)) - { - beatmapInfo.BeatmapSet.Status = res.BeatmapSetStatus ?? BeatmapOnlineStatus.None; - beatmapInfo.BeatmapSet.DateRanked = res.DateRanked; - beatmapInfo.BeatmapSet.DateSubmitted = res.DateSubmitted; - } } + + if (beatmapSet.Beatmaps.All(b => b.MatchesOnlineVersion) + && lookupResults.All(r => r != null) + && lookupResults.Select(r => r!.BeatmapSetID).Distinct().Count() == 1) + { + var representative = lookupResults.First()!; + + beatmapSet.Status = representative.BeatmapSetStatus ?? BeatmapOnlineStatus.None; + beatmapSet.DateRanked = representative.DateRanked; + beatmapSet.DateSubmitted = representative.DateSubmitted; + } + } + + private bool shouldDiscardLookupResult(OnlineBeatmapMetadata result, BeatmapInfo beatmapInfo) + { + if (beatmapInfo.OnlineID > 0 && result.BeatmapID != beatmapInfo.OnlineID) + return true; + + if (beatmapInfo.OnlineID == -1 && result.MD5Hash != beatmapInfo.MD5Hash) + return true; + + return false; } /// @@ -104,12 +125,6 @@ namespace osu.Game.Beatmaps return false; } - /// - /// Check whether the provided beatmap is in a state where online "ranked" status metadata should be saved against it. - /// Handles the case where a user may have locally modified a beatmap in the editor and expects the local status to stick. - /// - private static bool shouldSaveOnlineMetadata(BeatmapInfo beatmapInfo) => beatmapInfo.MatchesOnlineVersion || beatmapInfo.Status != BeatmapOnlineStatus.LocallyModified; - public void Dispose() { apiMetadataSource.Dispose();