From 0418d70056c3211ff20b48293bf3e04de64fdafa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 16:38:02 +0200 Subject: [PATCH 1/7] Add test coverage for ignoring null mods returned by rulesets --- .../TestSceneBrokenRulesetHandling.cs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs diff --git a/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs b/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs new file mode 100644 index 0000000000..9d27abf0b4 --- /dev/null +++ b/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs @@ -0,0 +1,49 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +#nullable enable + +using System.Collections.Generic; +using System.Linq; +using NUnit.Framework; +using osu.Framework.Allocation; +using osu.Framework.Testing; +using osu.Game.Beatmaps; +using osu.Game.Rulesets; +using osu.Game.Rulesets.Difficulty; +using osu.Game.Rulesets.Mods; +using osu.Game.Rulesets.UI; +using osu.Game.Tests.Visual; + +namespace osu.Game.Tests.Rulesets +{ + [HeadlessTest] + public class TestSceneBrokenRulesetHandling : OsuTestScene + { + [Resolved] + private OsuGameBase gameBase { get; set; } = null!; + + [Test] + public void TestNullModsReturnedByRulesetAreIgnored() + { + AddStep("set ruleset with null mods", () => Ruleset.Value = new TestRulesetWithNullMods().RulesetInfo); + AddAssert("no null mods in available mods", () => gameBase.AvailableMods.Value.SelectMany(kvp => kvp.Value).All(mod => mod != null)); + } + +#nullable disable // purposefully disabling nullability to simulate broken or unannotated API user code. + + private class TestRulesetWithNullMods : Ruleset + { + public override string ShortName => "nullmods"; + public override string Description => "nullmods"; + + public override IEnumerable GetModsFor(ModType type) => new Mod[] { null }; + + public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList mods = null) => null; + public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => null; + public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => null; + } + +#nullable enable + } +} From 6e5e506fb46adca7971089acb75e7a1d446b130e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 16:18:56 +0200 Subject: [PATCH 2/7] Add protection against rulesets returning null mods --- osu.Game/OsuGameBase.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 5dbdf6f602..e6f1452c3f 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -519,7 +519,9 @@ namespace osu.Game foreach (ModType type in Enum.GetValues(typeof(ModType))) { - dict[type] = instance.GetModsFor(type).ToList(); + dict[type] = instance.GetModsFor(type) + .Where(mod => mod != null) + .ToList(); } if (!SelectedMods.Disabled) From 665ef5fdcc06c9d8bcdb8bfb705b1ebee8e61047 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 17:16:17 +0200 Subject: [PATCH 3/7] Add test coverage for API-incompatible rulesets wrt mods --- .../TestSceneBrokenRulesetHandling.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs b/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs index 9d27abf0b4..243a93b4e5 100644 --- a/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs +++ b/osu.Game.Tests/Rulesets/TestSceneBrokenRulesetHandling.cs @@ -3,6 +3,7 @@ #nullable enable +using System; using System.Collections.Generic; using System.Linq; using NUnit.Framework; @@ -12,6 +13,7 @@ using osu.Game.Beatmaps; using osu.Game.Rulesets; using osu.Game.Rulesets.Difficulty; using osu.Game.Rulesets.Mods; +using osu.Game.Rulesets.Osu; using osu.Game.Rulesets.UI; using osu.Game.Tests.Visual; @@ -23,6 +25,12 @@ namespace osu.Game.Tests.Rulesets [Resolved] private OsuGameBase gameBase { get; set; } = null!; + [SetUpSteps] + public void SetUpSteps() + { + AddStep("reset ruleset", () => Ruleset.Value = new OsuRuleset().RulesetInfo); + } + [Test] public void TestNullModsReturnedByRulesetAreIgnored() { @@ -30,6 +38,17 @@ namespace osu.Game.Tests.Rulesets AddAssert("no null mods in available mods", () => gameBase.AvailableMods.Value.SelectMany(kvp => kvp.Value).All(mod => mod != null)); } + [Test] + public void TestRulesetRevertedIfModsCannotBeRetrieved() + { + RulesetInfo ruleset = null!; + + AddStep("store current ruleset", () => ruleset = Ruleset.Value); + + AddStep("set API incompatible ruleset", () => Ruleset.Value = new TestAPIIncompatibleRuleset().RulesetInfo); + AddAssert("ruleset not changed", () => Ruleset.Value.Equals(ruleset)); + } + #nullable disable // purposefully disabling nullability to simulate broken or unannotated API user code. private class TestRulesetWithNullMods : Ruleset @@ -44,6 +63,19 @@ namespace osu.Game.Tests.Rulesets public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => null; } + private class TestAPIIncompatibleRuleset : Ruleset + { + public override string ShortName => "incompatible"; + public override string Description => "incompatible"; + + // simulate API incompatibility by throwing similar exceptions. + public override IEnumerable GetModsFor(ModType type) => throw new MissingMethodException(); + + public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList mods = null) => null; + public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => null; + public override DifficultyCalculator CreateDifficultyCalculator(IWorkingBeatmap beatmap) => null; + } + #nullable enable } } From e74b563b91e1526420363e4c507f19d494e241ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 17:22:39 +0200 Subject: [PATCH 4/7] Add protection against arbitrary exceptions from `GetModsFor()` --- osu.Game/OsuGameBase.cs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index e6f1452c3f..2885d9413d 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -511,23 +511,34 @@ namespace osu.Game if (instance == null) { // reject the change if the ruleset is not available. - Ruleset.Value = r.OldValue?.Available == true ? r.OldValue : RulesetStore.AvailableRulesets.First(); + revertRulesetChange(); return; } var dict = new Dictionary>(); - foreach (ModType type in Enum.GetValues(typeof(ModType))) + try { - dict[type] = instance.GetModsFor(type) - .Where(mod => mod != null) - .ToList(); + foreach (ModType type in Enum.GetValues(typeof(ModType))) + { + dict[type] = instance.GetModsFor(type) + .Where(mod => mod != null) + .ToList(); + } + } + catch (Exception e) + { + Logger.Error(e, $"Could not load mods for \"{instance.RulesetInfo.Name}\" ruleset. Current ruleset has been rolled back."); + revertRulesetChange(); + return; } if (!SelectedMods.Disabled) SelectedMods.Value = Array.Empty(); AvailableMods.Value = dict; + + void revertRulesetChange() => Ruleset.Value = r.OldValue?.Available == true ? r.OldValue : RulesetStore.AvailableRulesets.First(); } private int allowableExceptions; From 30382b0445e4676ecf73710c8cfb4d7fcc5fb622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 17:26:54 +0200 Subject: [PATCH 5/7] Fill out `GetModsFor()` xmldoc and annotate items as non-null --- osu.Game/OsuGameBase.cs | 2 ++ osu.Game/Rulesets/Ruleset.cs | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 2885d9413d..e34a67fd02 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -522,6 +522,8 @@ namespace osu.Game foreach (ModType type in Enum.GetValues(typeof(ModType))) { dict[type] = instance.GetModsFor(type) + // Rulesets should never return null mods, but let's be defensive just in case. + // ReSharper disable once ConditionIsAlwaysTrueOrFalse .Where(mod => mod != null) .ToList(); } diff --git a/osu.Game/Rulesets/Ruleset.cs b/osu.Game/Rulesets/Ruleset.cs index b7c6132bdb..9dec6984ea 100644 --- a/osu.Game/Rulesets/Ruleset.cs +++ b/osu.Game/Rulesets/Ruleset.cs @@ -93,6 +93,15 @@ namespace osu.Game.Rulesets return AllMods.FirstOrDefault(m => m is T)?.CreateInstance() as T; } + /// + /// Creates an enumerable with mods that are supported by the ruleset for the supplied . + /// + /// + /// If there are no applicable mods from the given in this ruleset, + /// then the proper behaviour is to return an empty enumerable. + /// mods should not be present in the returned enumerable. + /// + [ItemNotNull] public abstract IEnumerable GetModsFor(ModType type); /// From 6ee1c35c8f4db78a4c74970618656a424be59a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 17:28:31 +0200 Subject: [PATCH 6/7] Fix template projects returning null mods from `GetModsFor()` --- .../osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs | 3 ++- .../osu.Game.Rulesets.Pippidon/PippidonRuleset.cs | 3 ++- .../osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs | 3 ++- .../osu.Game.Rulesets.Pippidon/PippidonRuleset.cs | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Templates/Rulesets/ruleset-empty/osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs b/Templates/Rulesets/ruleset-empty/osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs index baf503085d..00754c6346 100644 --- a/Templates/Rulesets/ruleset-empty/osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs +++ b/Templates/Rulesets/ruleset-empty/osu.Game.Rulesets.EmptyFreeform/EmptyFreeformRuleset.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -41,7 +42,7 @@ namespace osu.Game.Rulesets.EmptyFreeform return new[] { new EmptyFreeformModAutoplay() }; default: - return new Mod[] { null }; + return Array.Empty(); } } diff --git a/Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs b/Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs index 15e988f466..7dcdac7019 100644 --- a/Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs +++ b/Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using osu.Framework.Graphics; using osu.Framework.Graphics.Sprites; @@ -37,7 +38,7 @@ namespace osu.Game.Rulesets.Pippidon return new[] { new PippidonModAutoplay() }; default: - return new Mod[] { null }; + return Array.Empty(); } } diff --git a/Templates/Rulesets/ruleset-scrolling-empty/osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs b/Templates/Rulesets/ruleset-scrolling-empty/osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs index b9bed74c88..c8f0c07724 100644 --- a/Templates/Rulesets/ruleset-scrolling-empty/osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs +++ b/Templates/Rulesets/ruleset-scrolling-empty/osu.Game.Rulesets.EmptyScrolling/EmptyScrollingRuleset.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using osu.Framework.Graphics; using osu.Framework.Graphics.Sprites; @@ -34,7 +35,7 @@ namespace osu.Game.Rulesets.EmptyScrolling return new[] { new EmptyScrollingModAutoplay() }; default: - return new Mod[] { null }; + return Array.Empty(); } } diff --git a/Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs b/Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs index ea94ceb4b5..d4566477db 100644 --- a/Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs +++ b/Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/PippidonRuleset.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using osu.Framework.Graphics; using osu.Framework.Graphics.Sprites; @@ -34,7 +35,7 @@ namespace osu.Game.Rulesets.Pippidon return new[] { new PippidonModAutoplay() }; default: - return new Mod[] { null }; + return Array.Empty(); } } From 1acbb87aa6d88555f3f783b8f4088c783e82792a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 15 Jun 2022 17:39:30 +0200 Subject: [PATCH 7/7] Ensure `AvailableMods.Value` is never null --- osu.Game/OsuGameBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index e34a67fd02..993ca5b5e8 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -159,7 +159,7 @@ namespace osu.Game /// /// Mods available for the current . /// - public readonly Bindable>> AvailableMods = new Bindable>>(); + public readonly Bindable>> AvailableMods = new Bindable>>(new Dictionary>()); private BeatmapDifficultyCache difficultyCache;