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

@@ -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

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<TrashIdMapping> TrashIdMappings { get; init; } = new List<TrashIdMapping>();
 
-    public CustomFormatCache Update(IEnumerable<CustomFormatData> 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()
         };
     }

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));
     }
 }

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<IServiceConfiguration>();
-
-        // Load initial CfCache just to test that it gets replaced
-        ctx.ServiceCache.Load<CustomFormatCache>(config).Returns(new CustomFormatCache
-        {
-            TrashIdMappings = new Collection<TrashIdMapping> {new("trashid", "", 1)}
-        });
-
-        var result = ctx.Persister.Load(config);
-
-        // Update with new cached items
-        var customFormatData = new List<CustomFormatData>
-        {
-            NewCf.Data("trashid", "name", 5)
-        };
-
-        result = result.Update(customFormatData);
-
-        result.Should().BeEquivalentTo(new CustomFormatCache
-        {
-            TrashIdMappings = new Collection<TrashIdMapping>
-            {
-                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<CustomFormatData>
-        {
-            NewCf.Data("trashid1", "name", 5),
-            NewCf.Data("trashid2", "invalid")
-        };
-
-        var cache = new CustomFormatCache().Update(customFormatData);
-
-        cache.TrashIdMappings.Should().BeEquivalentTo(new Collection<TrashIdMapping>
-        {
-            new(customFormatData[0].TrashId, customFormatData[0].Name, customFormatData[0].Id)
-        });
-    }
 }

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());
+    }
+}