From d3db2f6af103563946da0d98d2917e0025de1fd0 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 20 Jul 2026 00:58:49 +0200 Subject: [PATCH] fix(488): resolve LibraryFolder from DB so Jellyfin music-video scans stop NRE'ing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetOrAddFolder read libraryPath.LibraryFolders, a navigation collection only eager-loaded on the local scan path (via GetLibrary). The Jellyfin remote sync path takes its LibraryPath off the JellyfinLibrary entity, where LibraryFolders is null, so .Filter(null) threw ArgumentNullException('source') on the first item of every Jellyfin music-video scan — a feature that had therefore never run in prod, CI, or locally. Look the folder up from the DB by (LibraryPathId, Path) instead, removing the implicit eager-load contract entirely (correct for all nine callers) and documenting it on ILibraryRepository. null != empty is preserved so a re-scan does not insert duplicate LibraryFolder rows. No new hot-path cost: local scanners already query GetParentFolderId per folder just before this call. Tests: - LibraryRepositoryTests: GetOrAddFolder with a null LibraryFolders (the exact remote-path shape) creates the folder, is idempotent on re-scan, and persists a supplied ParentId. - JellyfinMusicVideoLibraryScannerTests: an end-to-end scan of one synthetic music video, wiring the REAL LibraryRepository/ArtistRepository/ MusicVideoRepository against in-memory SQLite (the existing MediaServer* scanner tests mock every repo, which is why the bug escaped), asserts the scan completes and creates Artist + MusicVideo rows with a real LibraryFolder. Both proven non-vacuous against the reverted fix (each reproduces the issue's ArgumentNullException). decisions.md entry added. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Repositories/ILibraryRepository.cs | 13 +- .../Data/Repositories/LibraryRepository.cs | 17 +- .../JellyfinMusicVideoLibraryScannerTests.cs | 156 ++++++++++++++++++ .../Integration/LibraryRepositoryTests.cs | 116 +++++++++++++ docs/decisions.md | 32 ++++ 5 files changed, 327 insertions(+), 7 deletions(-) create mode 100644 ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs create mode 100644 ErsatzTV.Tests/Integration/LibraryRepositoryTests.cs diff --git a/ErsatzTV.Core/Interfaces/Repositories/ILibraryRepository.cs b/ErsatzTV.Core/Interfaces/Repositories/ILibraryRepository.cs index afa8038de..01328d33a 100644 --- a/ErsatzTV.Core/Interfaces/Repositories/ILibraryRepository.cs +++ b/ErsatzTV.Core/Interfaces/Repositories/ILibraryRepository.cs @@ -1,4 +1,4 @@ -using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain; namespace ErsatzTV.Core.Interfaces.Repositories; @@ -23,6 +23,17 @@ public interface ILibraryRepository Task SetEtag(LibraryPath libraryPath, Option knownFolder, string path, string etag); Task CleanEtagsForLibraryPath(LibraryPath libraryPath); Task> GetParentFolderId(LibraryPath libraryPath, string folder, CancellationToken cancellationToken); + + /// + /// Returns the at under + /// , creating it if it does not yet exist. + /// + /// + /// The existing folder is looked up from the database by (LibraryPathId, Path). Callers do + /// not need to eager-load — the remote (Jellyfin) + /// sync path never does, and relying on that navigation collection here previously NRE'd every + /// Jellyfin music-video scan (ersatztv#488). + /// Task GetOrAddFolder(LibraryPath libraryPath, Option maybeParentFolder, string folder); Task UpdateLibraryFolderId(MediaFile mediaFile, int libraryFolderId); Task UpdatePath(LibraryPath libraryPath, string normalizedLibraryPath); diff --git a/ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs b/ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs index 64eab6b38..a296a4f17 100644 --- a/ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs +++ b/ErsatzTV.Infrastructure/Data/Repositories/LibraryRepository.cs @@ -1,4 +1,4 @@ -using System.IO.Abstractions; +using System.IO.Abstractions; using Dapper; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Interfaces.Repositories; @@ -169,11 +169,16 @@ public class LibraryRepository(IFileSystem fileSystem, IDbContextFactory f.Path == folder && f.LibraryPathId == libraryPath.Id) - .HeadOrNone() - .IfNoneAsync(CreateNewFolder(libraryPath, maybeParentFolder, folder)); + // load from db or create new folder. Look the folder up by (LibraryPathId, Path) rather than + // reading libraryPath.LibraryFolders: that navigation collection is only eager-loaded on the + // local scan path (via GetLibrary) and is null on the remote (Jellyfin) sync path, which used + // to NRE every Jellyfin music-video scan here (ersatztv#488). The local scanners already hit + // the db once per folder via GetParentFolderId, so this adds no new query pattern. + LibraryFolder knownFolder = await dbContext.LibraryFolders + .AsNoTracking() + .Filter(f => f.LibraryPathId == libraryPath.Id && f.Path == folder) + .FirstOrDefaultAsync() + ?? CreateNewFolder(libraryPath, maybeParentFolder, folder); // update parent folder if not present foreach (int parentFolder in maybeParentFolder) diff --git a/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs b/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs new file mode 100644 index 000000000..cc6da681e --- /dev/null +++ b/ErsatzTV.Tests/Integration/JellyfinMusicVideoLibraryScannerTests.cs @@ -0,0 +1,156 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Jellyfin; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Jellyfin; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Scanner.Core.Interfaces; +using ErsatzTV.Scanner.Core.Jellyfin; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using IFileSystem = System.IO.Abstractions.IFileSystem; +using Unit = LanguageExt.Unit; + +namespace ErsatzTV.Tests.Integration; + +// End-to-end regression for ersatztv#488. Unlike the existing MediaServer*LibraryScanner tests (which +// substitute every repository, and therefore could never have exhibited the null-navigation bug — see the +// mocked GetOrAddFolder in MovieFolderScannerTests), this test wires the REAL LibraryRepository / +// ArtistRepository / MusicVideoRepository against in-memory SQLite so the actual crash path runs. The +// deviation from the mock-and-verify house style is deliberate and required: a substituted +// ILibraryRepository cannot exhibit the defect this issue is about. +[TestFixture] +public class JellyfinMusicVideoLibraryScannerTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task ScanLibrary_Should_Complete_And_Create_Artist_And_MusicVideo_Rows() + { + int libraryPathId = await SeedLibraryPath("/data/music"); + + // the remote-path shape that used to crash: Paths is populated, LibraryFolders is null + var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null }; + var library = new JellyfinLibrary + { + Id = 42, + MediaSourceId = 1, + ItemId = "lib15", + ShouldSyncItems = true, + Paths = new List { libraryPath } + }; + + const string VideoPath = "/data/music/artist1/song1.mkv"; + MusicVideo incoming = BuildIncoming(VideoPath, artistName: "Artist 1", title: "Song 1"); + + var apiClient = Substitute.For(); + apiClient.GetMusicVideoLibraryItems(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(OneItem(incoming)); + + 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); + + var scanner = new JellyfinMusicVideoLibraryScanner( + scannerProxy, + apiClient, + pathReplacement, + mediaSourceRepository, + new ArtistRepository(_db.Factory), + new MusicVideoRepository(_db.Factory), + new LibraryRepository(Substitute.For(), _db.Factory), + Substitute.For(), + NullLogger.Instance); + + var connectionParameters = new JellyfinConnectionParameters("http://jellyfin", "api-key", 1); + + Either result = + await scanner.ScanLibrary(connectionParameters, library, deepScan: false, CancellationToken.None); + + // the scan runs to completion instead of crashing on GetOrAddFolder + result.IsRight.ShouldBeTrue(result.Match(Right: _ => "", Left: e => e.Value)); + + await using TvContext context = _db.CreateContext(); + + // Artist row created from the incoming metadata + List artists = await context.Artists + .Include(a => a.ArtistMetadata) + .Where(a => a.LibraryPathId == libraryPathId) + .ToListAsync(); + artists.Count.ShouldBe(1); + artists[0].ArtistMetadata.Single().Title.ShouldBe("Artist 1"); + + // MusicVideo row created, wired to a real LibraryFolder (proving GetOrAddFolder returned a persisted row) + List musicVideos = await context.MusicVideos + .Include(mv => mv.MediaVersions) + .ThenInclude(v => v.MediaFiles) + .Where(mv => mv.LibraryPathId == libraryPathId) + .ToListAsync(); + musicVideos.Count.ShouldBe(1); + MediaFile file = musicVideos[0].MediaVersions.Single().MediaFiles.Single(); + file.Path.ShouldBe(VideoPath); + file.LibraryFolderId.ShouldNotBeNull(); + + // the folder was created by the fixed GetOrAddFolder + LibraryFolder folder = await context.LibraryFolders.SingleAsync(f => f.Id == file.LibraryFolderId); + folder.Path.ShouldBe("/data/music/artist1"); + folder.LibraryPathId.ShouldBe(libraryPathId); + } + + private static MusicVideo BuildIncoming(string path, string artistName, string title) => + new() + { + MediaVersions = new List + { + new() + { + MediaFiles = new List { new() { Path = path } }, + Streams = new List() + } + }, + MusicVideoMetadata = new List + { + new() + { + Title = title, + Artists = new List { new() { Name = artistName } } + } + } + }; + + private static async IAsyncEnumerable> OneItem(MusicVideo musicVideo) + { + yield return new Tuple(musicVideo, 1); + await Task.CompletedTask; + } + + private async Task SeedLibraryPath(string path) + { + await using TvContext context = _db.CreateContext(); + var libraryPath = new LibraryPath { Path = path }; + await context.LibraryPaths.AddAsync(libraryPath); + await context.SaveChangesAsync(); + return libraryPath.Id; + } +} diff --git a/ErsatzTV.Tests/Integration/LibraryRepositoryTests.cs b/ErsatzTV.Tests/Integration/LibraryRepositoryTests.cs new file mode 100644 index 000000000..e2dcb36d9 --- /dev/null +++ b/ErsatzTV.Tests/Integration/LibraryRepositoryTests.cs @@ -0,0 +1,116 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using IFileSystem = System.IO.Abstractions.IFileSystem; + +namespace ErsatzTV.Tests.Integration; + +[TestFixture] +public class LibraryRepositoryTests +{ + private InMemoryTvContext _db = null!; + private LibraryRepository _repository = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _repository = new LibraryRepository(Substitute.For(), _db.Factory); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + // Regression for ersatztv#488: the Jellyfin (remote) sync path takes its LibraryPath straight off the + // JellyfinLibrary entity, so LibraryPath.LibraryFolders is never eager-loaded (null). GetOrAddFolder + // used to read that navigation collection directly and threw ArgumentNullException on the very first + // item of every Jellyfin music-video scan. The repository must resolve the folder from the database + // instead, so an unloaded collection is not a precondition. + [Test] + public async Task GetOrAddFolder_Should_Create_Folder_When_LibraryFolders_Not_Loaded() + { + int libraryPathId = await SeedLibraryPath("/data/music"); + + // mimic the Jellyfin path: Paths is populated, but LibraryFolders was never included + var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null }; + + LibraryFolder result = + await _repository.GetOrAddFolder(libraryPath, Option.None, "/data/music/artist1"); + + result.ShouldNotBeNull(); + result.Id.ShouldBeGreaterThan(0); + result.Path.ShouldBe("/data/music/artist1"); + result.LibraryPathId.ShouldBe(libraryPathId); + + await using TvContext context = _db.CreateContext(); + List folders = await context.LibraryFolders + .Where(f => f.LibraryPathId == libraryPathId) + .ToListAsync(); + folders.Count.ShouldBe(1); + folders[0].Path.ShouldBe("/data/music/artist1"); + } + + // Re-scanning must be idempotent: a second GetOrAddFolder for the same path returns the existing row + // rather than inserting a duplicate LibraryFolder (there is no unique constraint behind it). + [Test] + public async Task GetOrAddFolder_Should_Be_Idempotent_On_Rescan() + { + int libraryPathId = await SeedLibraryPath("/data/music"); + var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null }; + + LibraryFolder first = + await _repository.GetOrAddFolder(libraryPath, Option.None, "/data/music/artist1"); + LibraryFolder second = + await _repository.GetOrAddFolder(libraryPath, Option.None, "/data/music/artist1"); + + second.Id.ShouldBe(first.Id); + + await using TvContext context = _db.CreateContext(); + int count = await context.LibraryFolders.CountAsync(f => f.LibraryPathId == libraryPathId); + count.ShouldBe(1); + } + + // Covers the maybeParentFolder = Some(...) branch: on a folder already in the db, the parent id is + // persisted through the raw Dapper UPDATE against the no-tracking entity (not change tracking), and the + // returned object reflects it. Guards the AsNoTracking + raw-UPDATE interaction the DB-lookup fix relies on. + [Test] + public async Task GetOrAddFolder_Should_Persist_ParentId_On_Existing_Folder() + { + int libraryPathId = await SeedLibraryPath("/data/music"); + var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null }; + + LibraryFolder parent = + await _repository.GetOrAddFolder(libraryPath, Option.None, "/data/music"); + // first pass creates the child with no parent + LibraryFolder child = + await _repository.GetOrAddFolder(libraryPath, Option.None, "/data/music/artist1"); + child.ParentId.ShouldBeNull(); + + // second pass supplies the parent — the existing row must be updated, not duplicated + LibraryFolder updated = + await _repository.GetOrAddFolder(libraryPath, Option.Some(parent.Id), "/data/music/artist1"); + updated.Id.ShouldBe(child.Id); + updated.ParentId.ShouldBe(parent.Id); + + await using TvContext context = _db.CreateContext(); + LibraryFolder persisted = await context.LibraryFolders.SingleAsync(f => f.Id == child.Id); + persisted.ParentId.ShouldBe(parent.Id); + int count = await context.LibraryFolders.CountAsync(f => f.Path == "/data/music/artist1"); + count.ShouldBe(1); + } + + private async Task SeedLibraryPath(string path) + { + await using TvContext context = _db.CreateContext(); + var libraryPath = new LibraryPath { Path = path }; + await context.LibraryPaths.AddAsync(libraryPath); + await context.SaveChangesAsync(); + return libraryPath.Id; + } +} diff --git a/docs/decisions.md b/docs/decisions.md index 0d5104a6f..9caa80296 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2221,3 +2221,35 @@ for its whole slot. This closes that gap. `PlayoutItemNotAvailableFromMediaServer` and no metadata call; available → `Right` with the `/media/plex` URL). The fail-open contract itself stays pinned by `HttpRemoteStreamProberTests`. Proven non-vacuous by neutralizing the probe guard and watching the unavailable assertion flip to `Right`. + +## 2026-07-20 — `ILibraryRepository.GetOrAddFolder` resolves the folder from the DB, not the caller's `LibraryPath.LibraryFolders` navigation (#488) + +`GetOrAddFolder` looked the existing folder up by reading `libraryPath.LibraryFolders` in memory. That +navigation collection is only eager-loaded on the **local** scan path — `LibraryRepository.GetLibrary` +`.Include(l => l.Paths).ThenInclude(p => p.LibraryFolders)` — which every `*FolderScanner` goes through. The +**remote** (Jellyfin) sync path takes its `LibraryPath` straight off the `JellyfinLibrary` entity, whose +`Paths[].LibraryFolders` is **null**, so `.Filter(...)` on it hit `Enumerable.Where(null, …)` and threw +`ArgumentNullException('source')` on the very first item of every Jellyfin music-video scan. +`JellyfinMusicVideoLibraryScanner` is the only *remote* caller of `GetOrAddFolder` (it does not derive from the +`MediaServer*LibraryScanner` base that the movie/TV/other-video remote scanners share), which is why no other +remote scanner tripped it, and the feature had never run in prod, CI, or locally. + +- **Fix the repository, not the caller.** The lookup now queries `dbContext.LibraryFolders` by + `(LibraryPathId, Path)` (the method already opened a `dbContext` for the insert). This removes the *implicit, + undocumented loading contract* entirely — correct for all nine callers — rather than the caller-side option + (eager-load `LibraryFolders` on the Jellyfin path), which would have left the contract intact for the next + remote scanner to trip. The contract change is now **stated** on `ILibraryRepository.GetOrAddFolder`. +- **`null` ≠ empty — do not coerce.** A `null` navigation meant *unknown*, not *known-absent*; treating it as an + empty collection would fall through to `CreateNewFolder` and insert a duplicate `LibraryFolder` for a path + that already exists (there is no unique constraint behind it). Querying the DB distinguishes the two. +- **No new hot-path cost.** Every local scanner already calls `GetParentFolderId` (a DB query with the same + `(LibraryPathId, …)` shape) once per folder immediately before `GetOrAddFolder`, so the folder granularity was + never served purely from memory; this adds one indexed lookup per folder, not a per-file query. As a bonus the + DB lookup also sees folders created earlier in the *same* scan, which the load-time in-memory snapshot could + not. +- **Tests.** `LibraryRepositoryTests` (real in-memory-SQLite `TvContext`) drives `GetOrAddFolder` with + `LibraryPath.LibraryFolders == null` — the exact remote-path shape — and asserts it creates the folder and is + idempotent on re-scan (no duplicate row). Proven non-vacuous by restoring the navigation-collection lookup and + watching both tests fail with the issue's `ArgumentNullException('source')`. Note: `CleanEtagsForLibraryPath` + still reads `libraryPath.LibraryFolders` directly, but it is only reached on the local (eager-loaded) path, so + it is not affected; left as-is (out of #488 scope). -- 2.47.3