From c633b89ad970d5b40fad82f859fff4bbf7b02dc4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 20 Jul 2026 18:32:29 +0200 Subject: [PATCH] fix(497): reconcile music-video metadata collections on Jellyfin rescan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JellyfinMusicVideoLibraryScanner.UpdateMetadata copied only scalar fields for an EXISTING music video, so genres/tags/studios/artists edited in Jellyfin never reached ErsatzTV — the update path silently dropped every child collection (only the Add path ever persisted them). Root cause is inherited from upstream: unlike the movie/episode Jellyfin path, which reconciles collections inside the tracked repository GetOrAdd, MusicVideoRepository.GetOrAdd is AsNoTracking and the scanner never reconciled the collections itself. Fix mirrors PlexMovieLibraryScanner.UpdateMetadata's remove-stale + add-new idiom, reconciling exactly the collections that BOTH the Add path persists AND GetOrAdd eager-loads: Genres, Tags, Studios, Artists. Guids (add-persisted but not eager-loaded — would duplicate) and Directors (eager-loaded but not add-persisted for music videos) are deliberately out of scope. Movie/Episode paths do NOT have this gap (they reconcile in the tracked repo GetOrAdd), so no separate fix is needed there. Test is an interaction test (substituted repos, canned existing item) verifying the exact reconcile calls; proven non-vacuous. The real-DB double-scan approach can't drive this: the in-memory harness shares one SQLite connection across contexts and mid-scan GetOrAdd's First()-nav path predicate mis-resolves once the existing item carries metadata children — a harness-only quirk (prod uses per-context pooled connections). fixes #497 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../JellyfinMusicVideoLibraryScanner.cs | 103 ++++++++++- .../JellyfinMusicVideoLibraryScannerTests.cs | 164 +++++++++++++++++- 2 files changed, 263 insertions(+), 4 deletions(-) diff --git a/ErsatzTV.Scanner/Core/Jellyfin/JellyfinMusicVideoLibraryScanner.cs b/ErsatzTV.Scanner/Core/Jellyfin/JellyfinMusicVideoLibraryScanner.cs index 0b2936e22..90c5f5528 100644 --- a/ErsatzTV.Scanner/Core/Jellyfin/JellyfinMusicVideoLibraryScanner.cs +++ b/ErsatzTV.Scanner/Core/Jellyfin/JellyfinMusicVideoLibraryScanner.cs @@ -263,11 +263,112 @@ public class JellyfinMusicVideoLibraryScanner : IJellyfinMusicVideoLibraryScanne existing.DateUpdated = DateTime.UtcNow; existing.MetadataKind = MetadataKind.External; - return await _metadataRepository.Update(existing); + bool updated = await _metadataRepository.Update(existing); + + // ersatztv#497: the scalar Update above marks only the metadata row Modified; it does NOT touch + // child collections, and MusicVideoRepository.GetOrAdd loads them AsNoTracking — so tag/genre/studio/ + // artist edits made in Jellyfin never reached an EXISTING music video (only the Add path persisted + // them). Reconcile the collections that BOTH the Add path persists AND GetOrAdd eager-loads: + // Genres, Tags, Studios, Artists. (Guids are add-persisted but not eager-loaded here — reconciling + // them would see an empty `existing` and duplicate-insert every scan; Directors are eager-loaded + // but not add-persisted for music videos — both are deliberately out of scope.) Mirrors the + // remove-stale + add-new idiom PlexMovieLibraryScanner.UpdateMetadata uses. + updated = await ReconcileGenres(existing, incoming) || updated; + updated = await ReconcileTags(existing, incoming) || updated; + updated = await ReconcileStudios(existing, incoming) || updated; + updated = await ReconcileArtists(existing, incoming) || updated; + + return updated; } incoming.MusicVideoId = musicVideo.Id; musicVideo.MusicVideoMetadata = [incoming]; return await _metadataRepository.Add(incoming); } + + private async Task ReconcileGenres(MusicVideoMetadata existing, MusicVideoMetadata incoming) + { + existing.Genres ??= []; + List incomingGenres = incoming.Genres ?? []; + var updated = false; + + foreach (Genre genre in existing.Genres.Filter(g => incomingGenres.All(g2 => g2.Name != g.Name)).ToList()) + { + existing.Genres.Remove(genre); + updated = await _metadataRepository.RemoveGenre(genre) || updated; + } + + foreach (Genre genre in incomingGenres.Filter(g => existing.Genres.All(g2 => g2.Name != g.Name)).ToList()) + { + existing.Genres.Add(genre); + updated = await _musicVideoRepository.AddGenre(existing, genre) || updated; + } + + return updated; + } + + private async Task ReconcileTags(MusicVideoMetadata existing, MusicVideoMetadata incoming) + { + existing.Tags ??= []; + List incomingTags = incoming.Tags ?? []; + var updated = false; + + foreach (Tag tag in existing.Tags.Filter(t => incomingTags.All(t2 => t2.Name != t.Name)).ToList()) + { + existing.Tags.Remove(tag); + updated = await _metadataRepository.RemoveTag(tag) || updated; + } + + foreach (Tag tag in incomingTags.Filter(t => existing.Tags.All(t2 => t2.Name != t.Name)).ToList()) + { + existing.Tags.Add(tag); + updated = await _musicVideoRepository.AddTag(existing, tag) || updated; + } + + return updated; + } + + private async Task ReconcileStudios(MusicVideoMetadata existing, MusicVideoMetadata incoming) + { + existing.Studios ??= []; + List incomingStudios = incoming.Studios ?? []; + var updated = false; + + foreach (Studio studio in existing.Studios.Filter(s => incomingStudios.All(s2 => s2.Name != s.Name)).ToList()) + { + existing.Studios.Remove(studio); + updated = await _metadataRepository.RemoveStudio(studio) || updated; + } + + foreach (Studio studio in incomingStudios.Filter(s => existing.Studios.All(s2 => s2.Name != s.Name)).ToList()) + { + existing.Studios.Add(studio); + updated = await _musicVideoRepository.AddStudio(existing, studio) || updated; + } + + return updated; + } + + private async Task ReconcileArtists(MusicVideoMetadata existing, MusicVideoMetadata incoming) + { + existing.Artists ??= []; + List incomingArtists = incoming.Artists ?? []; + var updated = false; + + foreach (MusicVideoArtist artist in existing.Artists + .Filter(a => incomingArtists.All(a2 => a2.Name != a.Name)).ToList()) + { + existing.Artists.Remove(artist); + updated = await _musicVideoRepository.RemoveArtist(artist) || updated; + } + + foreach (MusicVideoArtist artist in incomingArtists + .Filter(a => existing.Artists.All(a2 => a2.Name != a.Name)).ToList()) + { + existing.Artists.Add(artist); + updated = await _musicVideoRepository.AddArtist(existing, artist) || updated; + } + + return updated; + } } diff --git a/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs b/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs index 2cd5d22e2..d1354f379 100644 --- a/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs +++ b/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs @@ -3,6 +3,7 @@ using ErsatzTV.Core.Domain; using ErsatzTV.Core.Interfaces.Jellyfin; using ErsatzTV.Core.Interfaces.Repositories; using ErsatzTV.Core.Jellyfin; +using ErsatzTV.Core.Metadata; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Data.Repositories; using ErsatzTV.Scanner.Core.Interfaces; @@ -16,6 +17,7 @@ using NUnit.Framework; using Shouldly; using IFileSystem = System.IO.Abstractions.IFileSystem; using Unit = LanguageExt.Unit; +using static LanguageExt.Prelude; namespace ErsatzTV.Tests.Integration; @@ -276,6 +278,150 @@ public class JellyfinMusicVideoLibraryScannerTests await scannerProxy.DidNotReceive().RemoveMediaItems(Arg.Any(), Arg.Any()); } + // ersatztv#497: metadata edits (genres/tags/studios/artists) made in Jellyfin to an EXISTING music video + // must reach ErsatzTV on the next scan. Before the fix, UpdateMetadata copied only scalar fields, so the + // update path silently dropped every child collection — add-new AND remove-stale. This is an interaction + // test: the repositories are substituted and GetOrAdd returns a canned existing item so we can verify the + // scanner issues the exact reconcile calls. (The real-DB double-scan approach can't drive this here: the + // in-memory harness shares ONE SQLite connection across contexts, and mid-scan GetOrAdd's + // `MediaVersions.First().MediaFiles.First().Path` predicate mis-resolves once the existing item carries + // metadata children — a harness-only quirk; prod uses per-context pooled connections and looks music videos + // up by that predicate only because they carry no server ItemId. See the #497 close comment.) + // Non-vacuous: reverting the Reconcile* calls in UpdateMetadata drops every Received() below. + [Test] + public async Task ScanLibrary_Should_Reconcile_Metadata_Collections_On_Rescan_Of_Existing_Item() + { + JellyfinLibrary library = BuildLibrary(1, "/data/music"); + var connectionParameters = new JellyfinConnectionParameters("http://jellyfin", "api-key", 1); + const string VideoPath = "/data/music/artist1/song1.mkv"; + + // the EXISTING item already in ErsatzTV, with the collections Jellyfin first gave it + var existing = new MusicVideo + { + Id = 7, + ArtistId = 3, + MediaVersions = new List + { + new() { MediaFiles = new List { new() { Path = VideoPath } }, Streams = new List() } + }, + MusicVideoMetadata = new List + { + new() + { + Id = 11, + Genres = new List { new() { Name = "Synthwave" }, new() { Name = "Retro" } }, + Tags = new List { new() { Name = "KeepTag" }, new() { Name = "DropTag" } }, + Studios = new List { new() { Name = "OldStudio" } }, + Artists = new List { new() { Name = "Artist 1" }, new() { Name = "Featured X" } } + } + } + }; + + var artistRepository = Substitute.For(); + artistRepository.GetArtistByMetadata(Arg.Any(), Arg.Any()) + .Returns(Some(new Artist { Id = 3, ArtistMetadata = new List() })); + artistRepository.DeleteEmptyArtists(Arg.Any()).Returns(new List()); + + var musicVideoRepository = Substitute.For(); + musicVideoRepository + .GetOrAdd(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Right>( + new MediaItemScanResult(existing) { IsAdded = false })); + musicVideoRepository.FindMusicVideoPaths(Arg.Any()) + .Returns(new List { VideoPath }.AsEnumerable()); + musicVideoRepository.AddGenre(Arg.Any(), Arg.Any()).Returns(true); + musicVideoRepository.AddTag(Arg.Any(), Arg.Any()).Returns(true); + musicVideoRepository.AddStudio(Arg.Any(), Arg.Any()).Returns(true); + musicVideoRepository.AddArtist(Arg.Any(), Arg.Any()).Returns(true); + musicVideoRepository.RemoveArtist(Arg.Any()).Returns(true); + + var metadataRepository = Substitute.For(); + metadataRepository.Update(Arg.Any()).Returns(true); + metadataRepository.UpdateStatistics(Arg.Any(), Arg.Any(), Arg.Any()).Returns(false); + metadataRepository.RemoveGenre(Arg.Any()).Returns(true); + metadataRepository.RemoveTag(Arg.Any()).Returns(true); + metadataRepository.RemoveStudio(Arg.Any()).Returns(true); + + var libraryRepository = Substitute.For(); + libraryRepository.GetParentFolderId(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Option.None); + libraryRepository.GetOrAddFolder(Arg.Any(), Arg.Any>(), Arg.Any()) + .Returns(new LibraryFolder { Id = 5, Path = "/data/music/artist1" }); + + JellyfinMusicVideoLibraryScanner scanner = BuildScannerWith( + FakeApi(() => BuildIncoming( + VideoPath, "Artist 1", "Song 1", + genres: new[] { "Synthwave", "Vaporwave" }, + tags: new[] { "KeepTag", "NewTag" }, + studios: new[] { "NewStudio" }, + artists: new[] { "Artist 1", "Featured Y" })), + artistRepository, + musicVideoRepository, + libraryRepository, + metadataRepository); + + (await scanner.ScanLibrary(connectionParameters, library, deepScan: false, CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + // remove-stale: Retro / DropTag / OldStudio / "Featured X" gone; kept items are NOT removed + await metadataRepository.Received(1).RemoveGenre(Arg.Is(g => g.Name == "Retro")); + await metadataRepository.DidNotReceive().RemoveGenre(Arg.Is(g => g.Name == "Synthwave")); + await metadataRepository.Received(1).RemoveTag(Arg.Is(t => t.Name == "DropTag")); + await metadataRepository.DidNotReceive().RemoveTag(Arg.Is(t => t.Name == "KeepTag")); + await metadataRepository.Received(1).RemoveStudio(Arg.Is(s => s.Name == "OldStudio")); + await musicVideoRepository.Received(1).RemoveArtist(Arg.Is(a => a.Name == "Featured X")); + await musicVideoRepository.DidNotReceive().RemoveArtist(Arg.Is(a => a.Name == "Artist 1")); + + // add-new: Vaporwave / NewTag / NewStudio / "Featured Y" added; kept items are NOT re-added + await musicVideoRepository.Received(1) + .AddGenre(Arg.Any(), Arg.Is(g => g.Name == "Vaporwave")); + await musicVideoRepository.DidNotReceive() + .AddGenre(Arg.Any(), Arg.Is(g => g.Name == "Synthwave")); + await musicVideoRepository.Received(1) + .AddTag(Arg.Any(), Arg.Is(t => t.Name == "NewTag")); + await musicVideoRepository.DidNotReceive() + .AddTag(Arg.Any(), Arg.Is(t => t.Name == "KeepTag")); + await musicVideoRepository.Received(1) + .AddStudio(Arg.Any(), Arg.Is(s => s.Name == "NewStudio")); + await musicVideoRepository.Received(1) + .AddArtist(Arg.Any(), Arg.Is(a => a.Name == "Featured Y")); + await musicVideoRepository.DidNotReceive() + .AddArtist(Arg.Any(), Arg.Is(a => a.Name == "Artist 1")); + } + + private JellyfinMusicVideoLibraryScanner BuildScannerWith( + IJellyfinApiClient apiClient, + IArtistRepository artistRepository, + IMusicVideoRepository musicVideoRepository, + ILibraryRepository libraryRepository, + IMetadataRepository metadataRepository) + { + var pathReplacement = Substitute.For(); + pathReplacement + .GetReplacementJellyfinPath(Arg.Any>(), Arg.Any(), Arg.Any()) + .Returns(ci => ci.ArgAt(1)); + + var mediaSourceRepository = Substitute.For(); + mediaSourceRepository.GetJellyfinPathReplacements(Arg.Any()) + .Returns(Task.FromResult(new List())); + + var scannerProxy = Substitute.For(); + scannerProxy.UpdateProgress(Arg.Any(), Arg.Any()).Returns(true); + scannerProxy.ReindexMediaItems(Arg.Any(), Arg.Any()).Returns(true); + scannerProxy.RemoveMediaItems(Arg.Any(), Arg.Any()).Returns(true); + + return new JellyfinMusicVideoLibraryScanner( + scannerProxy, + apiClient, + pathReplacement, + mediaSourceRepository, + artistRepository, + musicVideoRepository, + libraryRepository, + metadataRepository, + NullLogger.Instance); + } + private async Task MusicVideoId(string path) { await using TvContext context = _db.CreateContext(); @@ -377,7 +523,14 @@ public class JellyfinMusicVideoLibraryScannerTests }; } - private static MusicVideo BuildIncoming(string path, string artistName, string title) => + private static MusicVideo BuildIncoming( + string path, + string artistName, + string title, + IEnumerable genres = null, + IEnumerable tags = null, + IEnumerable studios = null, + IEnumerable artists = null) => new() { MediaVersions = new List @@ -385,7 +538,8 @@ public class JellyfinMusicVideoLibraryScannerTests new() { MediaFiles = new List { new() { Path = path } }, - Streams = new List() + Streams = new List(), + Chapters = new List() } }, MusicVideoMetadata = new List @@ -393,7 +547,11 @@ public class JellyfinMusicVideoLibraryScannerTests new() { Title = title, - Artists = new List { new() { Name = artistName } } + Genres = (genres ?? Enumerable.Empty()).Select(g => new Genre { Name = g }).ToList(), + Tags = (tags ?? Enumerable.Empty()).Select(t => new Tag { Name = t }).ToList(), + Studios = (studios ?? Enumerable.Empty()).Select(s => new Studio { Name = s }).ToList(), + Artists = (artists ?? new[] { artistName }) + .Select(a => new MusicVideoArtist { Name = a }).ToList() } } };