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).