Perform online ID checks during import in more proper sequence #36102
Conversation
The end of `Populate()` is too early to do any validation of online IDs, as population happens in `PostImport()` via `ProcessBeatmap`. Additionally, this modifies the check to also include the set's ID. This is one part of curtailing issues like https://osu.ppy.sh/community/forums/topics/2162457?n=8 - the featured artist template beatmap in question has a single difficulty with a bogus positive beatmap set ID and a beatmap ID of 0, and my opinion is that this sort of situation should for all intents and purposes cause the beatmap set's ID to be reset.
…t's validated Even without the ID resetting logic before `Populate()` getting broken and annotated with a TODO, this was kinda stupid. Why was purging logic allowed to run *using a not-yet-completely-validated online ID of the set*? This is the other part of hopefully fixing scenarios like https://osu.ppy.sh/community/forums/topics/2162457?n=8 (hopefully with no babies poured out in the mean time, but only time will tell).
bdach
added
area:database
|
I've stared at the test failures for almost 2 hours and I can't figure out how to fix them. The tests are doing dodgy crap with sorta-kinda-maybe-possibly relying on the old broken code not firing in situations where you'd want it to fire in real life scenarios. The sticking point is specifically the added safety checking the set's online ID as well as the beatmaps' in osu/osu.Game/Beatmaps/BeatmapImporter.cs Line 200 in 44df904 I can't just remove it since that would kneecap the fix, but I also can't figure out how to sanely rewrite the tests so that they get even the semblance of correct operation. Marking blocked for now. Will revisit after christmas break maybe if I have enough energy to figure this out. If I don't I'll just close. |
1 participant
The overarching theme here is preventing https://osu.ppy.sh/community/forums/topics/2162457?n=8 from happening again.
Recommend reading commit-by-commit because the diff of the entire PR makes it look way worse than it actually is.
Move online ID validation check into its proper location
The end of
Populate()is too early to do any validation of online IDs of this kind, as population happens inPostImport()viaProcessBeatmap.Additionally, this modifies the check to also include the set's ID. This is one part of curtailing issues like https://osu.ppy.sh/community/forums/topics/2162457?n=8 - the featured artist template beatmap in question has a single difficulty with a bogus positive beatmap set ID and a beatmap ID of 0, and my opinion is that this sort of situation should for all intents and purposes cause the beatmap set's ID to be reset even if you have multiple difficulties with clearly invalid IDs.
Only perform replace-on-import based on online beatmap set ID after it's validated
Even without the ID resetting logic before
Populate()getting broken and annotated with a TODO, this was kinda stupid. Why was purging logic allowed to run using a not-yet-completely-validated online ID of the set?This is the other part of hopefully fixing scenarios like https://osu.ppy.sh/community/forums/topics/2162457?n=8 (hopefully with no babies poured out with the bathwater, but only time will tell).
At least the update flow appears to still function correctly.