From f05ff6e04bff0e6ba1a1a3de3c25a0cad3c30a51 Mon Sep 17 00:00:00 2001 From: Robert Dailey Date: Fri, 23 Jun 2023 13:35:12 -0500 Subject: [PATCH] fix: Service cache now remembers CFs no longer in config With `delete_old_custom_formats: false` and `replace_existing_custom_formats: false`, if you comment out a CF in your configuration, sync, uncomment it and sync again, you get an error about duplicate CFs. This is because, once a CF is removed from the configuration, it's also removed from the cache. This change makes the cache more flexible. As long as a CF (created by Recyclarr) exists either in the config OR in the service itself, it will be kept in the cache. This means that temporarily disabling CFs in configuration won't cause ownership issues. --- CHANGELOG.md | 5 + src/Recyclarr.Cli/Cache/CustomFormatCache.cs | 29 ++- .../CustomFormat/CustomFormatSyncPipeline.cs | 2 +- .../Cache/CachePersisterTest.cs | 51 ---- .../Cache/CustomFormatCacheTest.cs | 230 ++++++++++++++++++ 5 files changed, 260 insertions(+), 57 deletions(-) create mode 100644 src/tests/Recyclarr.Cli.Tests/Cache/CustomFormatCacheTest.cs 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()); + } +}