From e3fb781a5a712883a83380b2023e2e09f11d4b83 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Jul 2018 12:58:28 +0900 Subject: [PATCH 1/2] Fix ArchiveModelManager's model import method not running import logic --- osu.Game/Beatmaps/BeatmapManager.cs | 3 +- osu.Game/Database/ArchiveModelManager.cs | 42 ++++++++++++-------- osu.Game/Database/SingletonContextFactory.cs | 2 +- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 78042349d1..4ff16b604a 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -91,7 +91,8 @@ namespace osu.Game.Beatmaps protected override void Populate(BeatmapSetInfo model, ArchiveReader archive) { - model.Beatmaps = createBeatmapDifficulties(archive); + if (archive != null) + model.Beatmaps = createBeatmapDifficulties(archive); foreach (BeatmapInfo b in model.Beatmaps) { diff --git a/osu.Game/Database/ArchiveModelManager.cs b/osu.Game/Database/ArchiveModelManager.cs index cbf0df3227..0465c0ad73 100644 --- a/osu.Game/Database/ArchiveModelManager.cs +++ b/osu.Game/Database/ArchiveModelManager.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.IO; using System.Linq; +using JetBrains.Annotations; using Microsoft.EntityFrameworkCore; using osu.Framework.IO.File; using osu.Framework.Logging; @@ -175,7 +176,24 @@ namespace osu.Game.Database /// The archive to be imported. public TModel Import(ArchiveReader archive) { - TModel item = null; + try + { + return Import(CreateModel(archive), archive); + } + catch (Exception e) + { + Logger.Error(e, $"Model creation of {archive.Name} failed.", LoggingTarget.Database); + return null; + } + } + + /// + /// Import an item from a . + /// + /// The model to be imported. + /// An optional archive to use for model population. + public TModel Import(TModel item, ArchiveReader archive = null) + { delayEvents(); try @@ -186,18 +204,16 @@ namespace osu.Game.Database { if (!write.IsTransactionLeader) throw new InvalidOperationException($"Ensure there is no parent transaction so errors can correctly be handled by {this}"); - // create a new model (don't yet add to database) - item = CreateModel(archive); - var existing = CheckForExisting(item); if (existing != null) { - Logger.Log($"Found existing {typeof(TModel)} for {archive.Name} (ID {existing.ID}). Skipping import.", LoggingTarget.Database); + Logger.Log($"Found existing {typeof(TModel)} for {item} (ID {existing.ID}). Skipping import.", LoggingTarget.Database); return existing; } - item.Files = createFileInfos(archive, Files); + if (archive != null) + item.Files = createFileInfos(archive, Files); Populate(item, archive); @@ -211,11 +227,11 @@ namespace osu.Game.Database } } - Logger.Log($"Import of {archive.Name} successfully completed!", LoggingTarget.Database); + Logger.Log($"Import of {item} successfully completed!", LoggingTarget.Database); } catch (Exception e) { - Logger.Error(e, $"Import of {archive.Name} failed and has been rolled back.", LoggingTarget.Database); + Logger.Error(e, $"Import of {item} failed and has been rolled back.", LoggingTarget.Database); item = null; } finally @@ -227,12 +243,6 @@ namespace osu.Game.Database return item; } - /// - /// Import an item from a . - /// - /// The model to be imported. - public void Import(TModel item) => ModelStore.Add(item); - /// /// Perform an update of the specified item. /// TODO: Support file changes. @@ -385,8 +395,8 @@ namespace osu.Game.Database /// After this method, the model should be in a state ready to commit to a store. /// /// The model to populate. - /// The archive to use as a reference for population. - protected virtual void Populate(TModel model, ArchiveReader archive) + /// The archive to use as a reference for population. May be null. + protected virtual void Populate(TModel model, [CanBeNull] ArchiveReader archive) { } diff --git a/osu.Game/Database/SingletonContextFactory.cs b/osu.Game/Database/SingletonContextFactory.cs index ce3fbf6881..a7158c0583 100644 --- a/osu.Game/Database/SingletonContextFactory.cs +++ b/osu.Game/Database/SingletonContextFactory.cs @@ -14,6 +14,6 @@ namespace osu.Game.Database public OsuDbContext Get() => context; - public DatabaseWriteUsage GetForWrite(bool withTransaction = true) => new DatabaseWriteUsage(context, null); + public DatabaseWriteUsage GetForWrite(bool withTransaction = true) => new DatabaseWriteUsage(context, null) { IsTransactionLeader = true }; } } From 68614f1512e2becfb23b2845e90265a1d54540e2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 19 Jul 2018 13:41:09 +0900 Subject: [PATCH 2/2] Ensure online IDs are validated for imports that don't have an associated archive too --- osu.Game/Beatmaps/BeatmapManager.cs | 73 ++++++++++++----------------- 1 file changed, 30 insertions(+), 43 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 4ff16b604a..3bd5c440c1 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -89,34 +89,46 @@ namespace osu.Game.Beatmaps this.audioManager = audioManager; } - protected override void Populate(BeatmapSetInfo model, ArchiveReader archive) + protected override void Populate(BeatmapSetInfo beatmapSet, ArchiveReader archive) { if (archive != null) - model.Beatmaps = createBeatmapDifficulties(archive); + beatmapSet.Beatmaps = createBeatmapDifficulties(archive); - foreach (BeatmapInfo b in model.Beatmaps) + foreach (BeatmapInfo b in beatmapSet.Beatmaps) { // remove metadata from difficulties where it matches the set - if (model.Metadata.Equals(b.Metadata)) + if (beatmapSet.Metadata.Equals(b.Metadata)) b.Metadata = null; - // by setting the model here, we can update the noline set id below. - b.BeatmapSet = model; - - fetchAndPopulateOnlineIDs(b, model.Beatmaps); + b.BeatmapSet = beatmapSet; } // check if a set already exists with the same online id, delete if it does. - if (model.OnlineBeatmapSetID != null) + if (beatmapSet.OnlineBeatmapSetID != null) { - var existingOnlineId = beatmaps.ConsumableItems.FirstOrDefault(b => b.OnlineBeatmapSetID == model.OnlineBeatmapSetID); + var existingOnlineId = beatmaps.ConsumableItems.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); if (existingOnlineId != null) { Delete(existingOnlineId); beatmaps.PurgeDeletable(s => s.ID == existingOnlineId.ID); - Logger.Log($"Found existing beatmap set with same OnlineBeatmapSetID ({model.OnlineBeatmapSetID}). It has been purged.", LoggingTarget.Database); + Logger.Log($"Found existing beatmap set with same OnlineBeatmapSetID ({beatmapSet.OnlineBeatmapSetID}). It has been purged.", LoggingTarget.Database); } } + + validateOnlineIds(beatmapSet.Beatmaps); + + foreach (BeatmapInfo b in beatmapSet.Beatmaps) + fetchAndPopulateOnlineIDs(b, beatmapSet.Beatmaps); + } + + private void validateOnlineIds(List beatmaps) + { + var beatmapIds = beatmaps.Where(b => b.OnlineBeatmapID.HasValue).Select(b => b.OnlineBeatmapID); + + // ensure all IDs are unique in this set and none match existing IDs in the local beatmap store. + if (beatmapIds.GroupBy(b => b).Any(g => g.Count() > 1) || QueryBeatmaps(b => beatmapIds.Contains(b.OnlineBeatmapID)).Any()) + // remove all online IDs if any problems were found. + beatmaps.ForEach(b => b.OnlineBeatmapID = null); } protected override BeatmapSetInfo CheckForExisting(BeatmapSetInfo model) @@ -297,7 +309,7 @@ namespace osu.Game.Beatmaps /// /// The query. /// Results from the provided query. - public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); + public IQueryable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); /// /// Denotes whether an osu-stable installation is present to perform automated imports from. @@ -360,8 +372,6 @@ namespace osu.Game.Beatmaps { var beatmapInfos = new List(); - bool invalidateOnlineIDs = false; - foreach (var name in reader.Filenames.Where(f => f.EndsWith(".osu"))) { using (var raw = reader.GetStream(name)) @@ -378,38 +388,15 @@ namespace osu.Game.Beatmaps beatmap.BeatmapInfo.Hash = ms.ComputeSHA2Hash(); beatmap.BeatmapInfo.MD5Hash = ms.ComputeMD5Hash(); - if (beatmap.BeatmapInfo.OnlineBeatmapID.HasValue) - { - var ourId = beatmap.BeatmapInfo.OnlineBeatmapID; - - // check that no existing beatmap in database exists that is imported with the same online beatmap ID. if so, give it precedence. - if (QueryBeatmap(b => b.OnlineBeatmapID.Value == ourId) != null) - beatmap.BeatmapInfo.OnlineBeatmapID = null; - - // check that no other beatmap in this imported set has a conflicting online beatmap ID. If so, presume *all* are incorrect. - if (beatmapInfos.Any(b => b.OnlineBeatmapID == ourId)) - invalidateOnlineIDs = true; - } - - RulesetInfo ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); - + var ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); beatmap.BeatmapInfo.Ruleset = ruleset; - - if (ruleset != null) - { - // TODO: this should be done in a better place once we actually need to dynamically update it. - beatmap.BeatmapInfo.StarDifficulty = ruleset.CreateInstance().CreateDifficultyCalculator(new DummyConversionBeatmap(beatmap)).Calculate().StarRating; - } - else - beatmap.BeatmapInfo.StarDifficulty = 0; + // TODO: this should be done in a better place once we actually need to dynamically update it. + beatmap.BeatmapInfo.StarDifficulty = ruleset?.CreateInstance().CreateDifficultyCalculator(new DummyConversionBeatmap(beatmap)).Calculate().StarRating ?? 0; beatmapInfos.Add(beatmap.BeatmapInfo); } } - if (invalidateOnlineIDs) - beatmapInfos.ForEach(b => b.OnlineBeatmapID = null); - return beatmapInfos; } @@ -422,12 +409,12 @@ namespace osu.Game.Beatmaps /// True if population was successful. private bool fetchAndPopulateOnlineIDs(BeatmapInfo beatmap, IEnumerable otherBeatmaps, bool force = false) { + if (api?.State != APIState.Online) + return false; + if (!force && beatmap.OnlineBeatmapID != null && beatmap.BeatmapSet.OnlineBeatmapSetID != null) return true; - if (api.State != APIState.Online) - return false; - Logger.Log("Attempting online lookup for IDs...", LoggingTarget.Database); try