diff --git a/CHANGELOG.md b/CHANGELOG.md index accfbde5..6232094c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Commenting/uncommenting CFs in configuration YAML no longer causes duplicate CF warnings when + `replace_existing_custom_formats` is omitted or set to `false` (better caching logic). + ## [5.0.1] - 2023-06-23 ### Changed diff --git a/src/Recyclarr.Cli/Cache/CustomFormatCache.cs b/src/Recyclarr.Cli/Cache/CustomFormatCache.cs index 3aa63024..48385226 100644 --- a/src/Recyclarr.Cli/Cache/CustomFormatCache.cs +++ b/src/Recyclarr.Cli/Cache/CustomFormatCache.cs @@ -1,3 +1,4 @@ +using Recyclarr.Cli.Pipelines.CustomFormat; using Recyclarr.TrashLib.Models; namespace Recyclarr.Cli.Cache; @@ -10,13 +11,31 @@ public record CustomFormatCache public int Version { get; init; } = LatestVersion; public IReadOnlyList TrashIdMappings { get; init; } = new List(); - public CustomFormatCache Update(IEnumerable customFormats) + public CustomFormatCache Update(CustomFormatTransactionData transactions) { + // Assume that RemoveStale() is called before this method, and that TrashIdMappings contains existing CFs + // in the remote service that we want to keep and update. + + var existingCfs = transactions.UpdatedCustomFormats + .Concat(transactions.UnchangedCustomFormats) + .Concat(transactions.NewCustomFormats); + return this with { - TrashIdMappings = customFormats - .Where(cf => cf.Id is not 0) - .Select(cf => new TrashIdMapping(cf.TrashId, cf.Name, cf.Id)) + TrashIdMappings = TrashIdMappings + .DistinctBy(x => x.CustomFormatId) + .Where(x => transactions.DeletedCustomFormats.All(y => y.CustomFormatId != x.CustomFormatId)) + .FullOuterJoin(existingCfs, JoinType.Hash, + l => l.CustomFormatId, + r => r.Id, + // Keep existing service CFs, even if they aren't in user config + l => l, + // Add a new mapping for CFs in user's config + r => new TrashIdMapping(r.TrashId, r.Name, r.Id), + // Update existing mappings for CFs in user's config + (l, r) => l with { TrashId = r.TrashId, CustomFormatName = r.Name }) + .Where(x => x.CustomFormatId != 0) + .OrderBy(x => x.CustomFormatId) .ToList() }; } @@ -26,7 +45,7 @@ public record CustomFormatCache return this with { TrashIdMappings = TrashIdMappings - .Where(x => serviceCfs.Any(y => y.Id == x.CustomFormatId)) + .Where(x => x.CustomFormatId != 0 && serviceCfs.Any(y => y.Id == x.CustomFormatId)) .ToList() }; } diff --git a/src/Recyclarr.Cli/Pipelines/CustomFormat/CustomFormatSyncPipeline.cs b/src/Recyclarr.Cli/Pipelines/CustomFormat/CustomFormatSyncPipeline.cs index ed327c6f..77eb3db9 100644 --- a/src/Recyclarr.Cli/Pipelines/CustomFormat/CustomFormatSyncPipeline.cs +++ b/src/Recyclarr.Cli/Pipelines/CustomFormat/CustomFormatSyncPipeline.cs @@ -69,6 +69,6 @@ public class CustomFormatSyncPipeline : ISyncPipeline await _phases.ApiPersistencePhase.Execute(config, transactions); - _cachePersister.Save(config, cache.Update(guideCfs)); + _cachePersister.Save(config, cache.Update(transactions)); } } diff --git a/src/tests/Recyclarr.Cli.Tests/Cache/CachePersisterTest.cs b/src/tests/Recyclarr.Cli.Tests/Cache/CachePersisterTest.cs index 8f9e71ff..4fe7a265 100644 --- a/src/tests/Recyclarr.Cli.Tests/Cache/CachePersisterTest.cs +++ b/src/tests/Recyclarr.Cli.Tests/Cache/CachePersisterTest.cs @@ -1,8 +1,6 @@ using System.Collections.ObjectModel; using Recyclarr.Cli.Cache; using Recyclarr.TrashLib.Config.Services; -using Recyclarr.TrashLib.Models; -using Recyclarr.TrashLib.TestLibrary; namespace Recyclarr.Cli.Tests.Cache; @@ -85,53 +83,4 @@ public class CachePersisterTest ctx.ServiceCache.Received().Save(testCfObj, config); } - - [Test] - public void Updating_overwrites_previous_cf_cache_and_updates_cf_data() - { - var ctx = new Context(); - var config = Substitute.For(); - - // Load initial CfCache just to test that it gets replaced - ctx.ServiceCache.Load(config).Returns(new CustomFormatCache - { - TrashIdMappings = new Collection {new("trashid", "", 1)} - }); - - var result = ctx.Persister.Load(config); - - // Update with new cached items - var customFormatData = new List - { - NewCf.Data("trashid", "name", 5) - }; - - result = result.Update(customFormatData); - - result.Should().BeEquivalentTo(new CustomFormatCache - { - TrashIdMappings = new Collection - { - new(customFormatData[0].TrashId, customFormatData[0].Name, customFormatData[0].Id) - } - }); - } - - [Test] - public void Cache_update_skips_custom_formats_with_zero_id() - { - // Update with new cached items - var customFormatData = new List - { - NewCf.Data("trashid1", "name", 5), - NewCf.Data("trashid2", "invalid") - }; - - var cache = new CustomFormatCache().Update(customFormatData); - - cache.TrashIdMappings.Should().BeEquivalentTo(new Collection - { - new(customFormatData[0].TrashId, customFormatData[0].Name, customFormatData[0].Id) - }); - } } diff --git a/src/tests/Recyclarr.Cli.Tests/Cache/CustomFormatCacheTest.cs b/src/tests/Recyclarr.Cli.Tests/Cache/CustomFormatCacheTest.cs new file mode 100644 index 00000000..1bc1c646 --- /dev/null +++ b/src/tests/Recyclarr.Cli.Tests/Cache/CustomFormatCacheTest.cs @@ -0,0 +1,230 @@ +using Recyclarr.Cli.Cache; +using Recyclarr.Cli.Pipelines.CustomFormat; +using Recyclarr.TrashLib.TestLibrary; + +namespace Recyclarr.Cli.Tests.Cache; + +[TestFixture] +[Parallelizable(ParallelScope.All)] +public class CustomFormatCacheTest +{ + [Test] + public void New_updated_and_changed_are_added() + { + var transactions = new CustomFormatTransactionData + { + NewCustomFormats = + { + NewCf.Data("one", "1", 1), + NewCf.Data("two", "2", 2) + }, + UpdatedCustomFormats = + { + NewCf.Data("three", "3", 3) + }, + UnchangedCustomFormats = + { + NewCf.Data("four", "4", 4) + } + }; + + var cache = new CustomFormatCache(); + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + }); + } + + [Test] + public void Deleted_cfs_are_removed() + { + var transactions = new CustomFormatTransactionData + { + NewCustomFormats = + { + NewCf.Data("one", "1", 1), + NewCf.Data("two", "2", 2) + }, + DeletedCustomFormats = + { + new TrashIdMapping("3", "three", 3) + } + }; + + var cache = new CustomFormatCache + { + TrashIdMappings = new[] + { + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + } + }; + + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("4", "four", 4) + }); + } + + [Test] + public void Cfs_not_in_service_are_removed() + { + var serviceCfs = new[] + { + NewCf.Data("one", "1", 1), + NewCf.Data("two", "2", 2) + }; + + var cache = new CustomFormatCache + { + TrashIdMappings = new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + } + }; + + var result = cache.RemoveStale(serviceCfs); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2) + }); + } + + [Test] + public void Cache_update_skips_custom_formats_with_zero_id() + { + var transactions = new CustomFormatTransactionData + { + NewCustomFormats = + { + NewCf.Data("one", "1", 1), + NewCf.Data("zero", "0") + }, + UpdatedCustomFormats = + { + NewCf.Data("two", "2", 2) + } + }; + + var cache = new CustomFormatCache(); + + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2) + }); + } + + [Test] + public void Existing_matching_mappings_should_be_replaced() + { + var transactions = new CustomFormatTransactionData + { + NewCustomFormats = + { + NewCf.Data("one_new", "1", 1), + NewCf.Data("two_new", "2", 2) + }, + UpdatedCustomFormats = + { + NewCf.Data("three_new", "3", 3) + }, + UnchangedCustomFormats = + { + NewCf.Data("four_new", "4", 4) + } + }; + + var cache = new CustomFormatCache + { + TrashIdMappings = new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + } + }; + + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one_new", 1), + new TrashIdMapping("2", "two_new", 2), + new TrashIdMapping("3", "three_new", 3), + new TrashIdMapping("4", "four_new", 4) + }); + } + + [Test] + public void Duplicate_mappings_should_be_removed() + { + var transactions = new CustomFormatTransactionData(); + + var cache = new CustomFormatCache + { + TrashIdMappings = new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("12", "one2", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + } + }; + + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + }); + } + + [Test] + public void Mappings_ordered_by_id() + { + var transactions = new CustomFormatTransactionData(); + + var cache = new CustomFormatCache + { + TrashIdMappings = new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4), + new TrashIdMapping("2", "two", 2), + } + }; + + var result = cache.Update(transactions); + + result.TrashIdMappings.Should().BeEquivalentTo(new[] + { + new TrashIdMapping("1", "one", 1), + new TrashIdMapping("2", "two", 2), + new TrashIdMapping("3", "three", 3), + new TrashIdMapping("4", "four", 4) + }, o => o.WithStrictOrdering()); + } +}