From f0d3980854faa01d97008d7528a2d7516bec9230 Mon Sep 17 00:00:00 2001 From: Paul Hebble Date: Sat, 16 Sep 2023 16:06:36 -0500 Subject: [PATCH] Fix duplicate key exception in CompatibilitySorter --- Core/Registry/CompatibilitySorter.cs | 50 +++++----- Core/Registry/Registry.cs | 6 +- Core/Repositories/AvailableModule.cs | 15 ++- Core/Repositories/RepositoryDataManager.cs | 1 + Tests/Core/Registry/CompatibilitySorter.cs | 103 +++++++++++++++++++++ Tests/Data/TemporaryRepository.cs | 11 ++- 6 files changed, 150 insertions(+), 36 deletions(-) create mode 100644 Tests/Core/Registry/CompatibilitySorter.cs diff --git a/Core/Registry/CompatibilitySorter.cs b/Core/Registry/CompatibilitySorter.cs index f785676042..b5d5fa9024 100644 --- a/Core/Registry/CompatibilitySorter.cs +++ b/Core/Registry/CompatibilitySorter.cs @@ -1,6 +1,8 @@ using System.Collections.Generic; using System.Linq; + using log4net; + using CKAN.Extensions; using CKAN.Versioning; @@ -31,7 +33,12 @@ public CompatibilitySorter(GameVersionCriteria crit this.installed = installed; this.dlls = dlls; this.dlc = dlc; - PartitionModules(available, CompatibleProviders(crit, providers)); + var merged = available.SelectMany(dict => dict) + .GroupBy(kvp => kvp.Key, + kvp => kvp.Value) + .ToDictionary(grp => grp.Key, + grp => AvailableModule.Merge(grp.ToList())); + PartitionModules(merged, CompatibleProviders(crit, providers)); } /// @@ -138,33 +145,30 @@ private Dictionary> CompatibleProviders( /// /// All mods available from registry /// Mapping from identifiers to mods providing those identifiers - private void PartitionModules(IEnumerable> dicts, + private void PartitionModules(Dictionary available, Dictionary> providers) { log.Debug("Partitioning modules by compatibility"); - foreach (var available in dicts) + // First get the ones that are trivially [in]compatible. + foreach (var kvp in available) { - // First get the ones that are trivially [in]compatible. - foreach (var kvp in available) + if (kvp.Value.AllAvailable().All(m => !m.IsCompatible(CompatibleVersions))) { - if (kvp.Value.AllAvailable().All(m => !m.IsCompatible(CompatibleVersions))) - { - // No versions compatible == incompatible - log.DebugFormat("Trivially incompatible: {0}", kvp.Key); - Incompatible.Add(kvp.Key, kvp.Value); - } - else if (kvp.Value.AllAvailable().All(m => m.depends == null)) - { - // No dependencies == compatible - log.DebugFormat("Trivially compatible: {0}", kvp.Key); - Compatible.Add(kvp.Key, kvp.Value); - } - else - { - // Need to investigate this one more later - log.DebugFormat("Trivially indeterminate: {0}", kvp.Key); - Indeterminate.Add(kvp.Key, kvp.Value); - } + // No versions compatible == incompatible + log.DebugFormat("Trivially incompatible: {0}", kvp.Key); + Incompatible.Add(kvp.Key, kvp.Value); + } + else if (kvp.Value.AllAvailable().All(m => m.depends == null)) + { + // No dependencies == compatible + log.DebugFormat("Trivially compatible: {0}", kvp.Key); + Compatible.Add(kvp.Key, kvp.Value); + } + else + { + // Need to investigate this one more later + log.DebugFormat("Trivially indeterminate: {0}", kvp.Key); + Indeterminate.Add(kvp.Key, kvp.Value); } } log.Debug("Trivial mods done, moving on to indeterminates"); diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs index f68cbad9ec..3505b74cc1 100644 --- a/Core/Registry/Registry.cs +++ b/Core/Registry/Registry.cs @@ -547,7 +547,11 @@ public CompatibilitySorter SetCompatibleVersion(GameVersionCriteria versCrit) BuildProvidesIndex(); } sorter = new CompatibilitySorter( - versCrit, repoDataMgr?.GetAllAvailDicts(repositories.Values), + versCrit, + repoDataMgr?.GetAllAvailDicts( + repositories.Values.OrderBy(r => r.priority) + // Break ties alphanumerically + .ThenBy(r => r.name)), providers, installed_modules, InstalledDlls.ToHashSet(), InstalledDlc); } diff --git a/Core/Repositories/AvailableModule.cs b/Core/Repositories/AvailableModule.cs index 4a590ca611..f116972d21 100644 --- a/Core/Repositories/AvailableModule.cs +++ b/Core/Repositories/AvailableModule.cs @@ -53,6 +53,11 @@ internal void DeserialisationFixes(StreamingContext context) Debug.Assert(module_version.Values.All(m => identifier.Equals(m.identifier))); } + public static AvailableModule Merge(IList availMods) + => availMods.Count == 1 ? availMods.First() + : new AvailableModule(availMods.First().identifier, + availMods.Reverse().SelectMany(am => am.AllAvailable())); + // The map of versions -> modules, that's what we're about! // First element is the oldest version, last is the newest. [JsonProperty] @@ -71,7 +76,7 @@ private void OnError(StreamingContext context, ErrorContext errorContext) /// /// Record the given module version as being available. /// - public void Add(CkanModule module) + private void Add(CkanModule module) { if (!module.identifier.Equals(identifier)) throw new ArgumentException( @@ -81,14 +86,6 @@ public void Add(CkanModule module) module_version[module.version] = module; } - /// - /// Remove the given version from our list of available. - /// - public void Remove(ModuleVersion version) - { - module_version.Remove(version); - } - /// /// Return the most recent release of a module with a optional ksp version to target and a RelationshipDescriptor to satisfy. /// diff --git a/Core/Repositories/RepositoryDataManager.cs b/Core/Repositories/RepositoryDataManager.cs index 08b791dab4..0c1362b651 100644 --- a/Core/Repositories/RepositoryDataManager.cs +++ b/Core/Repositories/RepositoryDataManager.cs @@ -272,6 +272,7 @@ private RepositoryData LoadRepoData(Repository repo, IProgress progress) private IEnumerable GetRepoDatas(IEnumerable repos) => repos?.OrderBy(repo => repo.priority) + .ThenBy(repo => repo.name) .Select(repo => GetRepoData(repo)) .Where(data => data != null) ?? Enumerable.Empty(); diff --git a/Tests/Core/Registry/CompatibilitySorter.cs b/Tests/Core/Registry/CompatibilitySorter.cs new file mode 100644 index 0000000000..390c353b98 --- /dev/null +++ b/Tests/Core/Registry/CompatibilitySorter.cs @@ -0,0 +1,103 @@ +using System.Collections.Generic; +using System.Linq; + +using NUnit.Framework; + +using CKAN; +using CKAN.Versioning; +using CKAN.Extensions; + +using Tests.Data; + +namespace Tests.Core.Registry +{ + [TestFixture] + public class CompatibilitySorterTests + { + [Test, + TestCase(new string[] + { + @"{ + ""spec_version"": ""v1.4"", + ""identifier"": ""kOS-EVA"", + ""name"": ""kOS-EVA"", + ""abstract"": ""Addon for kOS that allows controlling a kerbal while on EVA"", + ""version"": ""0.2.0.0"", + ""ksp_version_min"": ""1.8.0"", + ""ksp_version_max"": ""1.12.99"", + ""license"": ""GPL-3.0"", + ""download"": ""https://github.com/"", + ""depends"": [ + { ""name"": ""Harmony2"" } + ] + }", + @"{ + ""spec_version"": ""v1.4"", + ""identifier"": ""Harmony2"", + ""name"": ""Harmony 2"", + ""abstract"": ""A library for patching, replacing and decorating .NET and Mono methods during runtime"", + ""version"": ""2.2.1.0"", + ""ksp_version_min"": ""1.8.0"", + ""ksp_version_max"": ""1.12.99"", + ""license"": ""MIT"", + ""download"": ""https://spacedockinfo/"" + }", + }, + new string[] + { + @"{ + ""spec_version"": ""v1.4"", + ""identifier"": ""kOS-EVA"", + ""name"": ""kOS-EVA"", + ""abstract"": ""Addon for kOS that allows controlling a kerbal while on EVA"", + ""version"": ""0.2.0.0"", + ""ksp_version_min"": ""1.8.0"", + ""ksp_version_max"": ""1.12.99"", + ""license"": ""GPL-3.0"", + ""download"": ""https://github.com/"" + }", + }, + "kOS-EVA"), + ] + public void Constructor_OverlappingModules_HigherPriorityOverrides(string[] modules1, + string[] modules2, + string identifier) + { + // Arrange + var user = new NullUser(); + using (var repo1 = new TemporaryRepository(0, modules1)) + using (var repo2 = new TemporaryRepository(1, modules2)) + using (var repoData = new TemporaryRepositoryData(user, repo1.repo, + repo2.repo)) + { + var versCrit = new GameVersionCriteria(GameVersion.Parse("1.12.5")); + var repos = new Repository[] { repo1.repo, repo2.repo }; + var providers = repoData.Manager + .GetAllAvailableModules(repos) + .GroupBy(am => am.AllAvailable().First().identifier) + .ToDictionary(grp => grp.Key, + grp => grp.ToHashSet()); + var installed = new Dictionary(); + var dlls = new HashSet(); + var dlcs = new Dictionary(); + var highPrio = repoData.Manager + .GetAvailableModules(Enumerable.Repeat(repo1.repo, 1), + identifier) + .First() + .Latest(); + + // Act + var sorter = new CompatibilitySorter( + versCrit, + repoData.Manager.GetAllAvailDicts(repos), + providers, installed, dlls, dlcs); + + // Assert + Assert.AreEqual(0, sorter.LatestIncompatible.Count); + Assert.AreEqual(2, sorter.LatestCompatible.Count); + Assert.AreEqual(CkanModule.ToJson(highPrio), + CkanModule.ToJson(sorter.LatestCompatible.First(m => m.identifier == identifier))); + } + } + } +} diff --git a/Tests/Data/TemporaryRepository.cs b/Tests/Data/TemporaryRepository.cs index 2147d2b25f..9f94d68fda 100644 --- a/Tests/Data/TemporaryRepository.cs +++ b/Tests/Data/TemporaryRepository.cs @@ -17,9 +17,10 @@ namespace Tests.Data /// public class TemporaryRepository : IDisposable { - public TemporaryRepository(params string[] fileContents) + public TemporaryRepository(int priority, params string[] fileContents) { path = Path.GetTempFileName(); + repo = new Repository("temp", path, priority); using (var outputStream = File.OpenWrite(path)) using (var gzipStream = new GZipOutputStream(outputStream)) @@ -39,8 +40,12 @@ public TemporaryRepository(params string[] fileContents) } } - public Uri uri => new Uri(path); - public Repository repo => new Repository("temp", uri); + public TemporaryRepository(params string[] fileContents) + : this(0, fileContents) + { } + + public Uri uri => new Uri(path); + public readonly Repository repo; public void Dispose() {