From 0c242443400529b107b567c318198210f2a846f6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Jul 2018 16:43:46 +0900 Subject: [PATCH 1/3] Remove SingletonContextFactory It is dangerous to use this as it doesn't correctly handle contexts and can cause issues that will never actually arise in normal execution. # Conflicts: # osu.Game/Database/SingletonContextFactory.cs --- .../Visual/TestCasePlaySongSelect.cs | 7 ++++++- osu.Game/Database/DatabaseContextFactory.cs | 10 +++++----- osu.Game/Database/SingletonContextFactory.cs | 19 ------------------- osu.Game/OsuGameBase.cs | 2 +- osu.Game/Tests/Platform/TestStorage.cs | 8 +++----- 5 files changed, 15 insertions(+), 31 deletions(-) delete mode 100644 osu.Game/Database/SingletonContextFactory.cs diff --git a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs index dab7f7e037..4afb76a7e2 100644 --- a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs @@ -62,7 +62,12 @@ namespace osu.Game.Tests.Visual var storage = new TestStorage(@"TestCasePlaySongSelect"); // this is by no means clean. should be replacing inside of OsuGameBase somehow. - IDatabaseContextFactory factory = new SingletonContextFactory(new OsuDbContext()); + DatabaseContextFactory factory = new DatabaseContextFactory(storage); + + factory.ResetDatabase(); + + using (var usage = factory.Get()) + usage.Migrate(); Dependencies.Cache(rulesets = new RulesetStore(factory)); Dependencies.Cache(manager = new BeatmapManager(storage, factory, rulesets, null, null) diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index 5160239c38..c20d4569f6 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -11,7 +11,7 @@ namespace osu.Game.Database { public class DatabaseContextFactory : IDatabaseContextFactory { - private readonly GameHost host; + private readonly Storage storage; private const string database_name = @"client"; @@ -26,9 +26,9 @@ namespace osu.Game.Database private IDbContextTransaction currentWriteTransaction; - public DatabaseContextFactory(GameHost host) + public DatabaseContextFactory(Storage storage) { - this.host = host; + this.storage = storage; recycleThreadContexts(); } @@ -117,7 +117,7 @@ namespace osu.Game.Database private void recycleThreadContexts() => threadContexts = new ThreadLocal(CreateContext); - protected virtual OsuDbContext CreateContext() => new OsuDbContext(host.Storage.GetDatabaseConnectionString(database_name)) + protected virtual OsuDbContext CreateContext() => new OsuDbContext(storage.GetDatabaseConnectionString(database_name)) { Database = { AutoTransactionsEnabled = false } }; @@ -129,7 +129,7 @@ namespace osu.Game.Database recycleThreadContexts(); GC.Collect(); GC.WaitForPendingFinalizers(); - host.Storage.DeleteDatabase(database_name); + storage.DeleteDatabase(database_name); } } } diff --git a/osu.Game/Database/SingletonContextFactory.cs b/osu.Game/Database/SingletonContextFactory.cs deleted file mode 100644 index ce3fbf6881..0000000000 --- a/osu.Game/Database/SingletonContextFactory.cs +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright (c) 2007-2018 ppy Pty Ltd . -// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE - -namespace osu.Game.Database -{ - public class SingletonContextFactory : IDatabaseContextFactory - { - private readonly OsuDbContext context; - - public SingletonContextFactory(OsuDbContext context) - { - this.context = context; - } - - public OsuDbContext Get() => context; - - public DatabaseWriteUsage GetForWrite(bool withTransaction = true) => new DatabaseWriteUsage(context, null); - } -} diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index a9b74d6740..63cc883844 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -107,7 +107,7 @@ namespace osu.Game { Resources.AddStore(new DllResourceStore(@"osu.Game.Resources.dll")); - dependencies.Cache(contextFactory = new DatabaseContextFactory(Host)); + dependencies.Cache(contextFactory = new DatabaseContextFactory(Host.Storage)); dependencies.Cache(new LargeTextureStore(new RawTextureLoaderStore(new NamespacedResourceStore(Resources, @"Textures")))); diff --git a/osu.Game/Tests/Platform/TestStorage.cs b/osu.Game/Tests/Platform/TestStorage.cs index 5b31c7b4d0..a6b6b5530d 100644 --- a/osu.Game/Tests/Platform/TestStorage.cs +++ b/osu.Game/Tests/Platform/TestStorage.cs @@ -7,13 +7,11 @@ namespace osu.Game.Tests.Platform { public class TestStorage : DesktopStorage { - public TestStorage(string baseName) : base(baseName, null) + public TestStorage(string baseName) + : base(baseName, null) { } - public override string GetDatabaseConnectionString(string name) - { - return "DataSource=:memory:"; - } + public override string GetDatabaseConnectionString(string name) => "Data Source=" + GetUsablePathFor($"{(object)name}.db", true); } } From 41441771ae2d4cb0bc95ecceef32b3a8d2b19593 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Jul 2018 22:22:28 +0900 Subject: [PATCH 2/3] Remove unnecessary cast --- osu.Game/Tests/Platform/TestStorage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Tests/Platform/TestStorage.cs b/osu.Game/Tests/Platform/TestStorage.cs index a6b6b5530d..8391de3405 100644 --- a/osu.Game/Tests/Platform/TestStorage.cs +++ b/osu.Game/Tests/Platform/TestStorage.cs @@ -12,6 +12,6 @@ namespace osu.Game.Tests.Platform { } - public override string GetDatabaseConnectionString(string name) => "Data Source=" + GetUsablePathFor($"{(object)name}.db", true); + public override string GetDatabaseConnectionString(string name) => "Data Source=" + GetUsablePathFor($"{name}.db", true); } } From 7be3a5d466bafa64a4e462a5c1ec0ca70f6d5d7b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 19 Jul 2018 14:07:55 +0900 Subject: [PATCH 3/3] Centralise test storage logic --- osu.Game.Tests/Visual/TestCasePlaySongSelect.cs | 10 +++------- osu.Game/Tests/Platform/TestStorage.cs | 17 ----------------- osu.Game/Tests/Visual/OsuTestCase.cs | 13 +++++++++++++ 3 files changed, 16 insertions(+), 24 deletions(-) delete mode 100644 osu.Game/Tests/Platform/TestStorage.cs diff --git a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs index 4afb76a7e2..b94fb42bf0 100644 --- a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs @@ -16,7 +16,6 @@ using osu.Game.Rulesets; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Carousel; using osu.Game.Screens.Select.Filter; -using osu.Game.Tests.Platform; namespace osu.Game.Tests.Visual { @@ -28,6 +27,7 @@ namespace osu.Game.Tests.Visual private RulesetStore rulesets; private WorkingBeatmap defaultBeatmap; + private DatabaseContextFactory factory; public override IReadOnlyList RequiredTypes => new[] { @@ -59,18 +59,14 @@ namespace osu.Game.Tests.Visual { TestSongSelect songSelect = null; - var storage = new TestStorage(@"TestCasePlaySongSelect"); - - // this is by no means clean. should be replacing inside of OsuGameBase somehow. - DatabaseContextFactory factory = new DatabaseContextFactory(storage); - + factory = new DatabaseContextFactory(LocalStorage); factory.ResetDatabase(); using (var usage = factory.Get()) usage.Migrate(); Dependencies.Cache(rulesets = new RulesetStore(factory)); - Dependencies.Cache(manager = new BeatmapManager(storage, factory, rulesets, null, null) + Dependencies.Cache(manager = new BeatmapManager(LocalStorage, factory, rulesets, null, null) { DefaultBeatmap = defaultBeatmap = Beatmap.Default }); diff --git a/osu.Game/Tests/Platform/TestStorage.cs b/osu.Game/Tests/Platform/TestStorage.cs deleted file mode 100644 index 8391de3405..0000000000 --- a/osu.Game/Tests/Platform/TestStorage.cs +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright (c) 2007-2018 ppy Pty Ltd . -// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE - -using osu.Framework.Platform; - -namespace osu.Game.Tests.Platform -{ - public class TestStorage : DesktopStorage - { - public TestStorage(string baseName) - : base(baseName, null) - { - } - - public override string GetDatabaseConnectionString(string name) => "Data Source=" + GetUsablePathFor($"{name}.db", true); - } -} diff --git a/osu.Game/Tests/Visual/OsuTestCase.cs b/osu.Game/Tests/Visual/OsuTestCase.cs index fcbab5b8f5..8e09ec849c 100644 --- a/osu.Game/Tests/Visual/OsuTestCase.cs +++ b/osu.Game/Tests/Visual/OsuTestCase.cs @@ -1,10 +1,12 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE +using System; using System.Linq; using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Configuration; +using osu.Framework.Platform; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Rulesets; @@ -20,6 +22,9 @@ namespace osu.Game.Tests.Visual protected DependencyContainer Dependencies { get; private set; } + private readonly Lazy localStorage; + protected Storage LocalStorage => localStorage.Value; + protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) { Dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); @@ -33,6 +38,11 @@ namespace osu.Game.Tests.Visual return Dependencies; } + protected OsuTestCase() + { + localStorage = new Lazy(() => new DesktopStorage($"{GetType().Name}-{Guid.NewGuid()}", null)); + } + [BackgroundDependencyLoader] private void load(AudioManager audioManager, RulesetStore rulesets) { @@ -50,6 +60,9 @@ namespace osu.Game.Tests.Visual beatmap.Disabled = true; beatmap.Value.Track.Stop(); } + + if (localStorage.IsValueCreated) + localStorage.Value.DeleteDirectory("."); } protected override ITestCaseTestRunner CreateRunner() => new OsuTestCaseTestRunner();