From 4ecc724841ac075f24f8d5695fb277672ccceca8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 27 Jun 2023 18:18:58 +0900 Subject: [PATCH 1/4] Add test coverage of save failure when beatmap is detached from set --- .../Visual/Editing/TestSceneEditorSaving.cs | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 64c48e74cf..7191ae6a57 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -96,6 +94,33 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("Beatmap still has correct timeline zoom", () => EditorBeatmap.BeatmapInfo.TimelineZoom == changedTimelineZoom); } + [Test] + public void TestSaveWithDetachedBeatmap() + { + AddStep("Set overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty = 7); + + SaveEditor(); + + AddStep("Detach beatmap from set", () => + { + Realm.Write(r => + { + BeatmapSetInfo? beatmapSet = r.Find(EditorBeatmap.BeatmapInfo.BeatmapSet!.ID); + BeatmapInfo? beatmap = r.Find(EditorBeatmap.BeatmapInfo.ID); + + beatmapSet.Beatmaps.Remove(beatmap); + }); + }); + + SaveEditor(); + + AddAssert("Beatmap has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); + + ReloadEditorToSameBeatmap(); + + AddAssert("Beatmap still has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); + } + [Test] public void TestDifficulty() { @@ -130,7 +155,7 @@ namespace osu.Game.Tests.Visual.Editing [Test] public void TestLengthAndStarRatingUpdated() { - WorkingBeatmap working = null; + WorkingBeatmap working = null!; double lastStarRating = 0; double lastLength = 0; From 8e80e2fa323337f885ae8119a5a14b5b54e68596 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 27 Jun 2023 18:19:59 +0900 Subject: [PATCH 2/4] Fix incorrect realm copy logic when a beatmap becomes detached from its set The code here was assuming that if the beatmap which is having changes copied across does not exist within the `BeatmapSet.Beatmaps` list, it was not yet persisted to realm. In some edge case, it can happen that the beatmap *is* persisted to realm but not correctly attached to the beatmap set. I don't yet know how this occurs, but it has caused loss of data for at least two users. The fix here is to check realm-wide for the beatmap (using its primary key) rather than only in the list. We then handle the scenario where the beatmap needs to be reattached to the set as a seprate step. --- This does raise others questions like "are we even structuring this correctly? couldn't a single beatmap exist in two different sets?" Maybe, but let's deal with that if/when it comes up. --- osu.Game/Database/RealmObjectExtensions.cs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index a771aa04df..888c1cf8ce 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -52,10 +52,19 @@ namespace osu.Game.Database { foreach (var beatmap in s.Beatmaps) { - var existing = d.Beatmaps.FirstOrDefault(b => b.ID == beatmap.ID); + // Importantly, search all of realm for the beatmap (not just the set's beatmaps). + // It may have gotten detached, and if that's the case let's use this opportunity to fix + // things up. + var existingBeatmap = d.Realm.Find(beatmap.ID); - if (existing != null) - copyChangesToRealm(beatmap, existing); + if (existingBeatmap != null) + { + // As above, reattach if it happens to not be in the set's beatmaps. + if (!d.Beatmaps.Contains(existingBeatmap)) + d.Beatmaps.Add(existingBeatmap); + + copyChangesToRealm(beatmap, existingBeatmap); + } else { var newBeatmap = new BeatmapInfo @@ -64,6 +73,7 @@ namespace osu.Game.Database BeatmapSet = d, Ruleset = d.Realm.Find(beatmap.Ruleset.ShortName) }; + d.Beatmaps.Add(newBeatmap); copyChangesToRealm(beatmap, newBeatmap); } From 99e55bb9c03972fdee0569ee4ae7a5c37eac84a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Jun 2023 12:21:05 +0900 Subject: [PATCH 3/4] Add logging and `Debug.Fail` on detached beatmap detection --- osu.Game/Database/RealmObjectExtensions.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index 888c1cf8ce..5a6c2e3232 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -3,10 +3,12 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Runtime.Serialization; using AutoMapper; using AutoMapper.Internal; +using osu.Framework.Logging; using osu.Game.Beatmaps; using osu.Game.Input.Bindings; using osu.Game.Models; @@ -61,7 +63,11 @@ namespace osu.Game.Database { // As above, reattach if it happens to not be in the set's beatmaps. if (!d.Beatmaps.Contains(existingBeatmap)) + { + Debug.Fail("Beatmaps should never become detached under normal circumstances. If this ever triggers, it should be investigated further."); + Logger.Log("WARNING: One of the difficulties in a beatmap was detached from its set. Please save a copy of logs and report this to devs.", LoggingTarget.Database, LogLevel.Important); d.Beatmaps.Add(existingBeatmap); + } copyChangesToRealm(beatmap, existingBeatmap); } From 6ce0ca832e57f2b9d4e96d9a3b9a908d49896f28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Jun 2023 19:52:08 +0200 Subject: [PATCH 4/4] Delete test case covering beatmap detach scenario Due to being fundamentally incompatible with the `Debug.Fail()` call added in 99e55bb9c03972fdee0569ee4ae7a5c37eac84a9. This reverts commit 4ecc724841ac075f24f8d5695fb277672ccceca8. --- .../Visual/Editing/TestSceneEditorSaving.cs | 31 ++----------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 7191ae6a57..64c48e74cf 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -1,6 +1,8 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +#nullable disable + using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -94,33 +96,6 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("Beatmap still has correct timeline zoom", () => EditorBeatmap.BeatmapInfo.TimelineZoom == changedTimelineZoom); } - [Test] - public void TestSaveWithDetachedBeatmap() - { - AddStep("Set overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty = 7); - - SaveEditor(); - - AddStep("Detach beatmap from set", () => - { - Realm.Write(r => - { - BeatmapSetInfo? beatmapSet = r.Find(EditorBeatmap.BeatmapInfo.BeatmapSet!.ID); - BeatmapInfo? beatmap = r.Find(EditorBeatmap.BeatmapInfo.ID); - - beatmapSet.Beatmaps.Remove(beatmap); - }); - }); - - SaveEditor(); - - AddAssert("Beatmap has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); - - ReloadEditorToSameBeatmap(); - - AddAssert("Beatmap still has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); - } - [Test] public void TestDifficulty() { @@ -155,7 +130,7 @@ namespace osu.Game.Tests.Visual.Editing [Test] public void TestLengthAndStarRatingUpdated() { - WorkingBeatmap working = null!; + WorkingBeatmap working = null; double lastStarRating = 0; double lastLength = 0;