From 588c1719787fc323d33d8583826c8df68ef7eb2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 7 May 2025 12:52:49 +0200 Subject: [PATCH] Improve logging around import-as-update flow - It was logging success before actually succeeding. - It appears in practice that this code can somehow actually nullref. Unfortunately logs provided in that instance were not enough to pinpoint what (because of lack of line numbers). I'm hoping that by logging as error, and therefore to sentry, we can actually retrieve this information so that there's no need to work blind. --- osu.Game/Beatmaps/BeatmapImporter.cs | 96 +++++++++++++++------------- 1 file changed, 52 insertions(+), 44 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index 77aca5eecf..28997509dc 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -72,58 +72,66 @@ namespace osu.Game.Beatmaps first.PerformWrite(updated => { - var realm = updated.Realm; - - Logger.Log($"Beatmap \"{updated}\" update completed successfully", LoggingTarget.Database); - - // Re-fetch as we are likely on a different thread. - original = realm!.Find(originalId)!; - - // Generally the import process will do this for us if the OnlineIDs match, - // but that isn't a guarantee (ie. if the .osu file doesn't have OnlineIDs populated). - original.DeletePending = true; - - // Transfer local values which should be persisted across a beatmap update. - updated.DateAdded = originalDateAdded; - - transferCollectionReferences(realm, original, updated); - - foreach (var beatmap in original.Beatmaps.ToArray()) + try { - var updatedBeatmap = updated.Beatmaps.FirstOrDefault(b => b.Hash == beatmap.Hash); + var realm = updated.Realm; - if (updatedBeatmap != null) + // Re-fetch as we are likely on a different thread. + original = realm!.Find(originalId)!; + + // Generally the import process will do this for us if the OnlineIDs match, + // but that isn't a guarantee (ie. if the .osu file doesn't have OnlineIDs populated). + original.DeletePending = true; + + // Transfer local values which should be persisted across a beatmap update. + updated.DateAdded = originalDateAdded; + + transferCollectionReferences(realm, original, updated); + + foreach (var beatmap in original.Beatmaps.ToArray()) { - // If the updated beatmap matches an existing one, transfer any user data across.. - if (beatmap.Scores.Any()) + var updatedBeatmap = updated.Beatmaps.FirstOrDefault(b => b.Hash == beatmap.Hash); + + if (updatedBeatmap != null) { - Logger.Log($"Transferring {beatmap.Scores.Count()} scores for unchanged difficulty \"{beatmap}\"", LoggingTarget.Database); + // If the updated beatmap matches an existing one, transfer any user data across.. + if (beatmap.Scores.Any()) + { + Logger.Log($"Transferring {beatmap.Scores.Count()} scores for unchanged difficulty \"{beatmap}\"", LoggingTarget.Database); - foreach (var score in beatmap.Scores) - score.BeatmapInfo = updatedBeatmap; + foreach (var score in beatmap.Scores) + score.BeatmapInfo = updatedBeatmap; + } + + // ..then nuke the old beatmap completely. + // this is done instead of a soft deletion to avoid a user potentially creating weird + // interactions, like restoring the outdated beatmap then updating a second time + // (causing user data to be wiped). + original.Beatmaps.Remove(beatmap); + + realm.Remove(beatmap.Metadata); + realm.Remove(beatmap); + } + else + { + // If the beatmap differs in the original, leave it in a soft-deleted state but reset online info. + // This caters to the case where a user has made modifications they potentially want to restore, + // but after restoring we want to ensure it can't be used to trigger an update of the beatmap. + beatmap.ResetOnlineInfo(); } - - // ..then nuke the old beatmap completely. - // this is done instead of a soft deletion to avoid a user potentially creating weird - // interactions, like restoring the outdated beatmap then updating a second time - // (causing user data to be wiped). - original.Beatmaps.Remove(beatmap); - - realm.Remove(beatmap.Metadata); - realm.Remove(beatmap); - } - else - { - // If the beatmap differs in the original, leave it in a soft-deleted state but reset online info. - // This caters to the case where a user has made modifications they potentially want to restore, - // but after restoring we want to ensure it can't be used to trigger an update of the beatmap. - beatmap.ResetOnlineInfo(); } + + // If the original has no beatmaps left, delete the set as well. + if (!original.Beatmaps.Any()) + realm.Remove(original); + + Logger.Log($"Beatmap \"{updated}\" update completed successfully", LoggingTarget.Database); + } + catch (Exception ex) + { + Logger.Error(ex, $"Failed to update beatmap \"{updated}\"", LoggingTarget.Database); + throw; } - - // If the original has no beatmaps left, delete the set as well. - if (!original.Beatmaps.Any()) - realm.Remove(original); }); return first;