Skip to content

Commit 50be7fb

Browse files
peppypeppy
authored
Merge pull request #30453 from bdach/everything-is-terrible
Fix several issues with beatmap online ID management
2 parents d52f8c6 + 7e3564c commit 50be7fb

File tree

7 files changed

+34
-119
lines changed

7 files changed

+34
-119
lines changed

‎osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs‎

Lines changed: 2 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -241,8 +241,8 @@ public void TestReturnedMetadataHasDifferentOnlineID([Values] bool preferOnlineF
241241

242242
metadataLookup.Update(beatmapSet, preferOnlineFetch);
243243

244-
Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
245-
Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
244+
Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
245+
Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
246246
}
247247

248248
[Test]
@@ -273,34 +273,6 @@ public void TestMetadataLookupForBeatmapWithoutPopulatedIDAndCorrectHash([Values
273273
Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
274274
}
275275

276-
[Test]
277-
public void TestMetadataLookupForBeatmapWithoutPopulatedIDAndIncorrectHash([Values] bool preferOnlineFetch)
278-
{
279-
var lookupResult = new OnlineBeatmapMetadata
280-
{
281-
BeatmapID = 654321,
282-
BeatmapStatus = BeatmapOnlineStatus.Ranked,
283-
MD5Hash = @"cafebabe",
284-
};
285-
286-
var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
287-
targetMock.Setup(src => src.Available).Returns(true);
288-
targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
289-
.Returns(true);
290-
291-
var beatmap = new BeatmapInfo
292-
{
293-
MD5Hash = @"deadbeef"
294-
};
295-
var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
296-
beatmap.BeatmapSet = beatmapSet;
297-
298-
metadataLookup.Update(beatmapSet, preferOnlineFetch);
299-
300-
Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
301-
Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
302-
}
303-
304276
[Test]
305277
public void TestReturnedMetadataHasDifferentHash([Values] bool preferOnlineFetch)
306278
{
@@ -383,58 +355,5 @@ public void TestPartiallyModifiedSet([Values] bool preferOnlineFetch)
383355

384356
Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
385357
}
386-
387-
[Test]
388-
public void TestPartiallyMaliciousSet([Values] bool preferOnlineFetch)
389-
{
390-
var firstResult = new OnlineBeatmapMetadata
391-
{
392-
BeatmapID = 654321,
393-
BeatmapStatus = BeatmapOnlineStatus.Ranked,
394-
BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
395-
MD5Hash = @"cafebabe"
396-
};
397-
var secondResult = new OnlineBeatmapMetadata
398-
{
399-
BeatmapStatus = BeatmapOnlineStatus.Ranked,
400-
BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
401-
MD5Hash = @"dededede"
402-
};
403-
404-
var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
405-
targetMock.Setup(src => src.Available).Returns(true);
406-
targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 654321), out firstResult))
407-
.Returns(true);
408-
targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 666666), out secondResult))
409-
.Returns(true);
410-
411-
var firstBeatmap = new BeatmapInfo
412-
{
413-
OnlineID = 654321,
414-
MD5Hash = @"cafebabe",
415-
};
416-
var secondBeatmap = new BeatmapInfo
417-
{
418-
OnlineID = 666666,
419-
MD5Hash = @"deadbeef"
420-
};
421-
var beatmapSet = new BeatmapSetInfo(new[]
422-
{
423-
firstBeatmap,
424-
secondBeatmap
425-
});
426-
firstBeatmap.BeatmapSet = beatmapSet;
427-
secondBeatmap.BeatmapSet = beatmapSet;
428-
429-
metadataLookup.Update(beatmapSet, preferOnlineFetch);
430-
431-
Assert.That(firstBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
432-
Assert.That(firstBeatmap.OnlineID, Is.EqualTo(654321));
433-
434-
Assert.That(secondBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
435-
Assert.That(secondBeatmap.OnlineID, Is.EqualTo(-1));
436-
437-
Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
438-
}
439358
}
440359
}

‎osu.Game/Beatmaps/APIBeatmapMetadataSource.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public bool TryLookup(BeatmapInfo beatmapInfo, out OnlineBeatmapMetadata? online
3333

3434
Debug.Assert(beatmapInfo.BeatmapSet != null);
3535

36-
var req = new GetBeatmapRequest(beatmapInfo);
36+
var req = new GetBeatmapRequest(md5Hash: beatmapInfo.MD5Hash, filename: beatmapInfo.Path);
3737

3838
try
3939
{

‎osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs‎

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.Linq;
8-
using osu.Framework.Logging;
98
using osu.Framework.Platform;
109
using osu.Game.Online.API;
1110

@@ -44,10 +43,19 @@ public void Update(BeatmapSetInfo beatmapSet, bool preferOnlineFetch)
4443

4544
foreach (var beatmapInfo in beatmapSet.Beatmaps)
4645
{
46+
// note that these lookups DO NOT ACTUALLY FULLY GUARANTEE that the beatmap is what it claims it is,
47+
// i.e. the correctness of this lookup should be treated as APPROXIMATE AT WORST.
48+
// this is because the beatmap filename is used as a fallback in some scenarios where the MD5 of the beatmap may mismatch.
49+
// this is considered to be an acceptable casualty so that things can continue to work as expected for users in some rare scenarios
50+
// (stale beatmap files in beatmap packs, beatmap mirror desyncs).
51+
// 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.
52+
//
53+
// 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
54+
// 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.
4755
if (!tryLookup(beatmapInfo, preferOnlineFetch, out var res))
4856
continue;
4957

50-
if (res == null || shouldDiscardLookupResult(res, beatmapInfo))
58+
if (res == null)
5159
{
5260
beatmapInfo.ResetOnlineInfo();
5361
lookupResults.Add(null); // mark lookup failure
@@ -83,23 +91,6 @@ public void Update(BeatmapSetInfo beatmapSet, bool preferOnlineFetch)
8391
}
8492
}
8593

86-
private bool shouldDiscardLookupResult(OnlineBeatmapMetadata result, BeatmapInfo beatmapInfo)
87-
{
88-
if (beatmapInfo.OnlineID > 0 && result.BeatmapID != beatmapInfo.OnlineID)
89-
{
90-
Logger.Log($"Discarding metadata lookup result due to mismatching online ID (expected: {beatmapInfo.OnlineID} actual: {result.BeatmapID})", LoggingTarget.Database);
91-
return true;
92-
}
93-
94-
if (beatmapInfo.OnlineID == -1 && result.MD5Hash != beatmapInfo.MD5Hash)
95-
{
96-
Logger.Log($"Discarding metadata lookup result due to mismatching hash (expected: {beatmapInfo.MD5Hash} actual: {result.MD5Hash})", LoggingTarget.Database);
97-
return true;
98-
}
99-
100-
return false;
101-
}
102-
10394
/// <summary>
10495
/// Attempts to retrieve the <see cref="OnlineBeatmapMetadata"/> for the given <paramref name="beatmapInfo"/>.
10596
/// </summary>

‎osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,7 @@ public bool TryLookup(BeatmapInfo beatmapInfo, [NotNullWhen(true)] out OnlineBea
9090
}
9191

9292
if (string.IsNullOrEmpty(beatmapInfo.MD5Hash)
93-
&& string.IsNullOrEmpty(beatmapInfo.Path)
94-
&& beatmapInfo.OnlineID <= 0)
93+
&& string.IsNullOrEmpty(beatmapInfo.Path))
9594
{
9695
onlineMetadata = null;
9796
return false;
@@ -240,10 +239,9 @@ private bool queryCacheVersion1(SqliteConnection db, BeatmapInfo beatmapInfo, ou
240239
using var cmd = db.CreateCommand();
241240

242241
cmd.CommandText =
243-
@"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash OR beatmap_id = @OnlineID OR filename = @Path";
242+
@"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash OR filename = @Path";
244243

245244
cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
246-
cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
247245
cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
248246

249247
using var reader = cmd.ExecuteReader();
@@ -281,11 +279,10 @@ private bool queryCacheVersion2(SqliteConnection db, BeatmapInfo beatmapInfo, ou
281279
SELECT `b`.`beatmapset_id`, `b`.`beatmap_id`, `b`.`approved`, `b`.`user_id`, `b`.`checksum`, `b`.`last_update`, `s`.`submit_date`, `s`.`approved_date`
282280
FROM `osu_beatmaps` AS `b`
283281
JOIN `osu_beatmapsets` AS `s` ON `s`.`beatmapset_id` = `b`.`beatmapset_id`
284-
WHERE `b`.`checksum` = @MD5Hash OR `b`.`beatmap_id` = @OnlineID OR `b`.`filename` = @Path
282+
WHERE `b`.`checksum` = @MD5Hash OR `b`.`filename` = @Path
285283
""";
286284

287285
cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
288-
cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
289286
cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
290287

291288
using var reader = cmd.ExecuteReader();

‎osu.Game/Database/RealmArchiveModelImporter.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ protected virtual void PostImport(TModel model, Realm realm, ImportParameters pa
528528
/// <param name="model">The new model proposed for import.</param>
529529
/// <param name="realm">The current realm context.</param>
530530
/// <returns>An existing model which matches the criteria to skip importing, else null.</returns>
531-
protected TModel? CheckForExisting(TModel model, Realm realm) => string.IsNullOrEmpty(model.Hash) ? null : realm.All<TModel>().FirstOrDefault(b => b.Hash == model.Hash);
531+
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);
532532

533533
/// <summary>
534534
/// Whether import can be skipped after finding an existing import early in the process.

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

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
22
// See the LICENCE file in the repository root for full licence text.
33

4+
using System.Globalization;
45
using osu.Framework.IO.Network;
56
using osu.Game.Beatmaps;
67
using osu.Game.Online.API.Requests.Responses;
@@ -9,23 +10,30 @@ namespace osu.Game.Online.API.Requests
910
{
1011
public class GetBeatmapRequest : APIRequest<APIBeatmap>
1112
{
12-
public readonly IBeatmapInfo BeatmapInfo;
13-
public readonly string Filename;
13+
public readonly int OnlineID;
14+
public readonly string? MD5Hash;
15+
public readonly string? Filename;
1416

1517
public GetBeatmapRequest(IBeatmapInfo beatmapInfo)
18+
: this(onlineId: beatmapInfo.OnlineID, md5Hash: beatmapInfo.MD5Hash, filename: (beatmapInfo as BeatmapInfo)?.Path)
1619
{
17-
BeatmapInfo = beatmapInfo;
18-
Filename = (beatmapInfo as BeatmapInfo)?.Path ?? string.Empty;
20+
}
21+
22+
public GetBeatmapRequest(int onlineId = -1, string? md5Hash = null, string? filename = null)
23+
{
24+
OnlineID = onlineId;
25+
MD5Hash = md5Hash;
26+
Filename = filename;
1927
}
2028

2129
protected override WebRequest CreateWebRequest()
2230
{
2331
var request = base.CreateWebRequest();
2432

25-
if (BeatmapInfo.OnlineID > 0)
26-
request.AddParameter(@"id", BeatmapInfo.OnlineID.ToString());
27-
if (!string.IsNullOrEmpty(BeatmapInfo.MD5Hash))
28-
request.AddParameter(@"checksum", BeatmapInfo.MD5Hash);
33+
if (OnlineID > 0)
34+
request.AddParameter(@"id", OnlineID.ToString(CultureInfo.InvariantCulture));
35+
if (!string.IsNullOrEmpty(MD5Hash))
36+
request.AddParameter(@"checksum", MD5Hash);
2937
if (!string.IsNullOrEmpty(Filename))
3038
request.AddParameter(@"filename", Filename);
3139

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ public bool HandleRequest(APIRequest request, APIUser localUser, BeatmapManager
188188

189189
case GetBeatmapRequest getBeatmapRequest:
190190
{
191-
getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.BeatmapInfo.OnlineID).Single());
191+
getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.OnlineID).Single());
192192
return true;
193193
}
194194

0 commit comments

Comments
 (0)