From 8523088ceb83afbf71a38127c1f1133800269aac Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 18:48:24 +0200 Subject: [PATCH 1/5] fix(671): resolve rerun-collection selections through one shared include chain The paged list handler eager-loaded nothing, so `ProjectToViewModel` read four unloaded navigations and every row of every collection type projected a null selection. Because the selected id and the display name are read off the SAME navigation, this dropped the id too -- the harm is not an unlabelled badge but an editor that round-trips a null and clears the user's stored selection. The by-id handler loaded metadata for only four of the ten selectable media types: Song/OtherVideo/Image/RemoteStream returned a null-ish selection and Episode/MusicVideo threw an NRE that surfaced as a 500. Fixed at the boundary rather than per call site: - `RerunCollectionQueryExtensions.IncludeSelectionDetails()` is now the single include chain, called by both handlers, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` precedent (#229). Artwork legs are deliberately omitted -- this projection reads only ids and titles. - The media-item switch was duplicated verbatim for RerunCollection and PlaylistItem; both now call one `ProjectMediaItemToViewModel`, which handles `RemoteStream` (via a new `ProjectToNamedViewModel`, since the existing `ProjectToViewModel(RemoteStream)` returns an unrelated type) and never falls through to null -- an unknown subtype keeps its id and takes a conspicuous name, because throwing would fail a whole paged GET over one bad row. - Every metadata navigation in `MediaItems.Mapper` is now read through `Optional(...).Flatten()`, so an un-included nav degrades to "???" instead of being a latent 500 for whichever caller loads least. Tests enumerate all 13 supported CollectionTypes for both handlers, with the matrix derived from `IsSupportedSelectionType` so a newly-supported type joins it automatically, plus a completeness guard on the set. Each mechanism was removed in turn and confirmed red first. fixes #671 Co-Authored-By: Claude Opus 5 (1M context) --- .../MediaCollections/Mapper.cs | 60 ++-- .../GetPagedRerunCollectionsHandler.cs | 6 +- .../Queries/GetRerunCollectionByIdHandler.cs | 17 +- .../RerunCollectionQueryExtensions.cs | 57 ++++ ErsatzTV.Application/MediaItems/Mapper.cs | 53 +++- .../RerunCollectionQueryHandlerTests.cs | 278 ++++++++++++++++++ docs/api-conventions.md | 28 ++ docs/decisions/README.md | 1 + .../api/selection-projection-include-chain.md | 53 ++++ 9 files changed, 495 insertions(+), 58 deletions(-) create mode 100644 ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs create mode 100644 ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs create mode 100644 docs/decisions/records/api/selection-projection-include-chain.md diff --git a/ErsatzTV.Application/MediaCollections/Mapper.cs b/ErsatzTV.Application/MediaCollections/Mapper.cs index b59029c2a..7038d6523 100644 --- a/ErsatzTV.Application/MediaCollections/Mapper.cs +++ b/ErsatzTV.Application/MediaCollections/Mapper.cs @@ -37,23 +37,43 @@ internal static class Mapper collection.Collection is not null ? ProjectToViewModel(collection.Collection) : null, collection.MultiCollection is not null ? ProjectToViewModel(collection.MultiCollection) : null, collection.SmartCollection is not null ? ProjectToViewModel(collection.SmartCollection) : null, - collection.MediaItem switch - { - Show show => MediaItems.Mapper.ProjectToViewModel(show), - Season season => MediaItems.Mapper.ProjectToViewModel(season), - Artist artist => MediaItems.Mapper.ProjectToViewModel(artist), - Movie movie => MediaItems.Mapper.ProjectToViewModel(movie), - Episode episode => MediaItems.Mapper.ProjectToViewModel(episode), - MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo), - OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo), - Song song => MediaItems.Mapper.ProjectToViewModel(song), - Image image => MediaItems.Mapper.ProjectToViewModel(image), - _ => null - }, + ProjectMediaItemToViewModel(collection.MediaItem), collection.FirstRunPlaybackOrder, collection.RerunPlaybackOrder, collection.Version); + /// + /// Flattens the half of a selection tagged union to a named view model. + /// Shared by and , which select from an + /// identical set of media types; one copy is what stops the two drifting apart again (issue #671 + /// — the same rationale as ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails + /// on the query side). + /// A null is the legitimate "this selection is not a media item" + /// case (the selection is a Collection/MultiCollection/SmartCollection instead) and maps to null. + /// An unrecognized non-null subtype keeps its id and takes a deliberately conspicuous name rather + /// than falling through to null: the id is what the editor round-trips, so returning null there + /// silently clears the user's stored selection — while throwing would fail an entire paged GET + /// over one unreadable row. + /// + private static MediaItems.NamedMediaItemViewModel ProjectMediaItemToViewModel(MediaItem mediaItem) => + mediaItem switch + { + null => null, + Show show => MediaItems.Mapper.ProjectToViewModel(show), + Season season => MediaItems.Mapper.ProjectToViewModel(season), + Artist artist => MediaItems.Mapper.ProjectToViewModel(artist), + Movie movie => MediaItems.Mapper.ProjectToViewModel(movie), + Episode episode => MediaItems.Mapper.ProjectToViewModel(episode), + MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo), + OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo), + Song song => MediaItems.Mapper.ProjectToViewModel(song), + Image image => MediaItems.Mapper.ProjectToViewModel(image), + RemoteStream remoteStream => MediaItems.Mapper.ProjectToNamedViewModel(remoteStream), + _ => new MediaItems.NamedMediaItemViewModel( + mediaItem.Id, + $"[unsupported media type: {mediaItem.GetType().Name}]") + }; + internal static TraktListViewModel ProjectToViewModel(TraktList traktList) => new( traktList.Id, @@ -108,19 +128,7 @@ internal static class Mapper playlistItem.SmartCollection is not null ? ProjectToViewModel(playlistItem.SmartCollection) : null, - playlistItem.MediaItem switch - { - Show show => MediaItems.Mapper.ProjectToViewModel(show), - Season season => MediaItems.Mapper.ProjectToViewModel(season), - Artist artist => MediaItems.Mapper.ProjectToViewModel(artist), - Movie movie => MediaItems.Mapper.ProjectToViewModel(movie), - Episode episode => MediaItems.Mapper.ProjectToViewModel(episode), - MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo), - OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo), - Song song => MediaItems.Mapper.ProjectToViewModel(song), - Image image => MediaItems.Mapper.ProjectToViewModel(image), - _ => null - }, + ProjectMediaItemToViewModel(playlistItem.MediaItem), playlistItem.PlaybackOrder, playlistItem.Count, playlistItem.PlayAll, diff --git a/ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs b/ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs index 00b1df517..cfdf08378 100644 --- a/ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs @@ -1,4 +1,4 @@ -using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain; using ErsatzTV.Infrastructure.Data; using Microsoft.EntityFrameworkCore; using static ErsatzTV.Application.MediaCollections.Mapper; @@ -15,13 +15,15 @@ public class GetPagedRerunCollectionsHandler(IDbContextFactory dbCont await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); int count = await dbContext.RerunCollections.CountAsync(cancellationToken); - IQueryable query = dbContext.RerunCollections.AsNoTracking(); + IQueryable query = dbContext.RerunCollections.AsNoTracking().IncludeSelectionDetails(); if (!string.IsNullOrWhiteSpace(request.Query)) { query = query.Where(rc => EF.Functions.Like(rc.Name, $"%{request.Query}%")); } + // EF applies the includes to the paged subquery, so the selection graph is loaded for at most + // PageSize rows — the per-request cost is bounded by the page, not by the table (issue #671). List page = await query .OrderBy(rc => rc.Name) .Skip(request.PageNum * request.PageSize) diff --git a/ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs b/ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs index 224cf6b96..46d68d9f3 100644 --- a/ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs @@ -1,4 +1,4 @@ -using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Extensions; using Microsoft.EntityFrameworkCore; @@ -16,20 +16,7 @@ public class GetRerunCollectionByIdHandler(IDbContextFactory dbContex await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); return await dbContext.RerunCollections .AsNoTracking() - .Include(c => c.Collection) - .Include(c => c.MultiCollection) - .Include(c => c.SmartCollection) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Movie).MovieMetadata) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Season).SeasonMetadata) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Season).Show) - .ThenInclude(s => s.ShowMetadata) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Show).ShowMetadata) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Artist).ArtistMetadata) + .IncludeSelectionDetails() .SelectOneAsync(c => c.Id, c => c.Id == request.Id, cancellationToken) .MapT(ProjectToViewModel); } diff --git a/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs b/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs new file mode 100644 index 000000000..3a33d1919 --- /dev/null +++ b/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs @@ -0,0 +1,57 @@ +using ErsatzTV.Core.Domain; +using Microsoft.EntityFrameworkCore; + +namespace ErsatzTV.Application.MediaCollections; + +internal static class RerunCollectionQueryExtensions +{ + /// + /// The single source of truth for the navigation graph a needs before it + /// can be projected via . Both the paged-list + /// and by-id handlers reload through this chain so the two cannot drift apart again (see #671 — the list + /// handler had no includes at all, so every row projected a null selection, while the by-id handler + /// covered only Movie/Season/Show/Artist and so returned a null selection for Song/OtherVideo/Image and + /// a 500 for Episode/MusicVideo). + /// Because the id and the display name are both read off these navigations, an un-included type does not + /// merely lose its label — it loses the selected id too, which is what silently cleared a stored + /// selection in the editor. + /// Deliberately narrower than the analogous playlist-item chain in GetPlaylistItemsHandler: the + /// rerun projection reads only each selection's id and title, never its artwork, so the + /// .ThenInclude(… => …Artwork) legs are omitted rather than paid for on every page. + /// + public static IQueryable IncludeSelectionDetails(this IQueryable query) => + query + .Include(c => c.Collection) + .Include(c => c.MultiCollection) + .Include(c => c.SmartCollection) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Movie).MovieMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Show).ShowMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Season).SeasonMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Season).Show) + .ThenInclude(s => s.ShowMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Artist).ArtistMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Episode).EpisodeMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Episode).Season) + .ThenInclude(s => s.Show) + .ThenInclude(s => s.ShowMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as MusicVideo).MusicVideoMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as MusicVideo).Artist) + .ThenInclude(a => a.ArtistMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as OtherVideo).OtherVideoMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Song).SongMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as Image).ImageMetadata) + .Include(c => c.MediaItem) + .ThenInclude(i => (i as RemoteStream).RemoteStreamMetadata); +} diff --git a/ErsatzTV.Application/MediaItems/Mapper.cs b/ErsatzTV.Application/MediaItems/Mapper.cs index 49f55b5eb..57760b12b 100644 --- a/ErsatzTV.Application/MediaItems/Mapper.cs +++ b/ErsatzTV.Application/MediaItems/Mapper.cs @@ -1,18 +1,24 @@ -using System.Globalization; +using System.Globalization; using ErsatzTV.Core.Domain; namespace ErsatzTV.Application.MediaItems; internal static class Mapper { + // Every metadata navigation below is read through Optional(...).Flatten() rather than a bare + // dereference: these projections are reached from several handlers whose Include chains differ, + // and an un-included navigation must degrade to the "???" placeholder instead of throwing an + // NRE that surfaces as a 500 on a GET (issue #671). internal static NamedMediaItemViewModel ProjectToViewModel(Show show) => - new(show.Id, show.ShowMetadata.HeadOrNone().Map(sm => $"{sm?.Title} ({sm?.Year})").IfNone("???")); + new( + show.Id, + Optional(show.ShowMetadata).Flatten().HeadOrNone().Map(sm => $"{sm?.Title} ({sm?.Year})").IfNone("???")); internal static NamedMediaItemViewModel ProjectToViewModel(Season season) => new(season.Id, $"{ShowTitle(season)} - {SeasonDescription(season)}"); internal static NamedMediaItemViewModel ProjectToViewModel(Artist artist) => - new(artist.Id, artist.ArtistMetadata.HeadOrNone().Match(am => am.Title, () => "???")); + new(artist.Id, Optional(artist.ArtistMetadata).Flatten().HeadOrNone().Match(am => am.Title, () => "???")); internal static NamedMediaItemViewModel ProjectToViewModel(Movie movie) => new(movie.Id, MovieTitle(movie)); @@ -24,23 +30,37 @@ internal static class Mapper new(musicVideo.Id, MusicVideoTitle(musicVideo)); internal static NamedMediaItemViewModel ProjectToViewModel(OtherVideo otherVideo) => - new(otherVideo.Id, otherVideo.OtherVideoMetadata.HeadOrNone().Match(ov => ov.Title, () => "???")); + new( + otherVideo.Id, + Optional(otherVideo.OtherVideoMetadata).Flatten().HeadOrNone().Match(ov => ov.Title, () => "???")); internal static NamedMediaItemViewModel ProjectToViewModel(Song song) => new(song.Id, SongTitle(song)); internal static NamedMediaItemViewModel ProjectToViewModel(Image image) => - new(image.Id, image.ImageMetadata.HeadOrNone().Match(i => i.Title, () => "???")); + new(image.Id, Optional(image.ImageMetadata).Flatten().HeadOrNone().Match(i => i.Title, () => "???")); internal static RemoteStreamViewModel ProjectToViewModel(RemoteStream remoteStream) => new(remoteStream.Id, remoteStream.Url, remoteStream.Script); + /// + /// The named projection for a . This cannot be an overload of + /// — that one already exists and returns a + /// , and C# will not overload on return type alone. Its + /// absence is why every selection-flattening switch dropped RemoteStream through a + /// _ => null arm (issue #671). + /// + internal static NamedMediaItemViewModel ProjectToNamedViewModel(RemoteStream remoteStream) => + new( + remoteStream.Id, + Optional(remoteStream.RemoteStreamMetadata).Flatten().HeadOrNone().Match(rsm => rsm.Title, () => "???")); + private static string MovieTitle(Movie movie) { var title = "???"; var year = "???"; - foreach (MovieMetadata movieMetadata in movie.MovieMetadata.HeadOrNone()) + foreach (MovieMetadata movieMetadata in Optional(movie.MovieMetadata).Flatten().HeadOrNone()) { title = movieMetadata.Title; foreach (int y in Optional(movieMetadata.Year)) @@ -57,7 +77,10 @@ internal static class Mapper var title = "???"; var year = "???"; - foreach (ShowMetadata show in season.Show.ShowMetadata.HeadOrNone()) + // Season.Show and Show.ShowMetadata are only populated when the caller eager-loaded them. + // An un-included navigation must degrade to the "???" placeholder these helpers already + // produce for missing metadata — never an NRE, which surfaced as a 500 (issue #671). + foreach (ShowMetadata show in Optional(season.Show?.ShowMetadata).Flatten().HeadOrNone()) { title = show.Title; foreach (int y in Optional(show.Year)) @@ -74,10 +97,10 @@ internal static class Mapper private static string EpisodeTitle(Episode e) { - string showTitle = e.Season.Show.ShowMetadata.HeadOrNone() + string showTitle = Optional(e.Season?.Show?.ShowMetadata).Flatten().HeadOrNone() .Map(sm => $"{sm.Title} - ").IfNone(string.Empty); - var episodeNumbers = e.EpisodeMetadata.Map(em => em.EpisodeNumber).ToList(); - var episodeTitles = e.EpisodeMetadata.Map(em => em.Title).ToList(); + var episodeNumbers = Optional(e.EpisodeMetadata).Flatten().Map(em => em.EpisodeNumber).ToList(); + var episodeTitles = Optional(e.EpisodeMetadata).Flatten().Map(em => em.Title).ToList(); if (episodeNumbers.Count == 0 || episodeTitles.Count == 0) { return "[unknown episode]"; @@ -86,24 +109,24 @@ internal static class Mapper var numbersString = $"e{string.Join('e', episodeNumbers.Map(n => $"{n:00}"))}"; var titlesString = $"{string.Join('/', episodeTitles)}"; - return $"{showTitle}s{e.Season.SeasonNumber:00}{numbersString} - {titlesString}"; + return $"{showTitle}s{e.Season?.SeasonNumber ?? 0:00}{numbersString} - {titlesString}"; } private static string MusicVideoTitle(MusicVideo mv) { - string artistName = mv.Artist.ArtistMetadata.HeadOrNone() + string artistName = Optional(mv.Artist?.ArtistMetadata).Flatten().HeadOrNone() .Map(am => $"{am.Title} - ").IfNone(string.Empty); - return mv.MusicVideoMetadata.HeadOrNone() + return Optional(mv.MusicVideoMetadata).Flatten().HeadOrNone() .Map(mvm => $"{artistName}{mvm.Title}") .IfNone("[unknown music video]"); } private static string SongTitle(Song s) { - string songArtist = s.SongMetadata.HeadOrNone() + string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone() .Map(sm => $"{string.Join(", ", sm.Artists)} - ") .IfNone(string.Empty); - return s.SongMetadata.HeadOrNone() + return Optional(s.SongMetadata).Flatten().HeadOrNone() .Map(sm => $"{songArtist}{sm.Title ?? string.Empty}") .IfNone("[unknown song]"); } diff --git a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs new file mode 100644 index 000000000..be00bcaad --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs @@ -0,0 +1,278 @@ +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Controllers.Api.Requests; +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.MediaCollections; + +/// +/// Read-path coverage for the two rerun-collection query handlers (issue #671). The defect was +/// precisely that nobody enumerated the selection types: the list handler eager-loaded nothing, and +/// the by-id handler loaded metadata for only four of the ten media types. So the matrix here is +/// derived from the production predicate rather than hand-listed — a newly-supported +/// joins it automatically and fails loudly in SeedSelection +/// until someone teaches the suite how to seed it. +/// +[TestFixture] +public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase +{ + private const int SelectedId = 42; + + private static IEnumerable SupportedSelectionTypes => + Enum.GetValues().Where(RerunCollectionRequestMapping.IsSupportedSelectionType); + + /// + /// Completeness guard. Without it, a change that narrowed IsSupportedSelectionType would + /// shrink the matrix silently and every remaining case would still pass — the "filters on the + /// property it asserts" failure mode. + /// + [Test] + public void Supported_Selection_Types_Should_Be_The_Full_Documented_Set() + { + SupportedSelectionTypes.ShouldBe( + [ + CollectionType.Collection, + CollectionType.TelevisionShow, + CollectionType.TelevisionSeason, + CollectionType.Artist, + CollectionType.MultiCollection, + CollectionType.SmartCollection, + CollectionType.Movie, + CollectionType.Episode, + CollectionType.MusicVideo, + CollectionType.OtherVideo, + CollectionType.Song, + CollectionType.Image, + CollectionType.RemoteStream + ], + ignoreOrder: true); + } + + [TestCaseSource(nameof(SupportedSelectionTypes))] + public async Task GetById_Should_Resolve_The_Selection(CollectionType collectionType) + { + await SeedSelection(collectionType); + await SeedRerunCollection(1, collectionType); + + var handler = new GetRerunCollectionByIdHandler(Db.Factory); + + Option result = + await handler.Handle(new GetRerunCollectionById(1), CancellationToken.None); + + RerunCollectionViewModel vm = result.IfNone(() => throw new AssertionException("Expected a result")); + AssertSelectionResolved(vm, collectionType); + } + + [TestCaseSource(nameof(SupportedSelectionTypes))] + public async Task GetPaged_Should_Resolve_The_Selection(CollectionType collectionType) + { + await SeedSelection(collectionType); + await SeedRerunCollection(1, collectionType); + + var handler = new GetPagedRerunCollectionsHandler(Db.Factory); + + PagedRerunCollectionsViewModel result = await handler.Handle( + new GetPagedRerunCollections(string.Empty, 0, 10), + CancellationToken.None); + + result.Page.Count.ShouldBe(1); + AssertSelectionResolved(result.Page[0], collectionType); + } + + /// + /// Mirrors RerunCollectionController.ProjectToResponseModel, which flattens the tagged + /// union to the single selectedId / selectedName pair the SPA consumes. The id is + /// the load-bearing half: the editor round-trips it, so a null there silently clears the user's + /// stored selection. + /// + private static void AssertSelectionResolved(RerunCollectionViewModel vm, CollectionType collectionType) + { + int? selectedId = vm.Collection?.Id + ?? vm.MultiCollection?.Id + ?? vm.SmartCollection?.Id + ?? vm.MediaItem?.MediaItemId; + + string selectedName = vm.Collection?.Name + ?? vm.MultiCollection?.Name + ?? vm.SmartCollection?.Name + ?? vm.MediaItem?.Name; + + selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id"); + + selectedName.ShouldNotBeNullOrWhiteSpace($"{collectionType} lost its selected name"); + + // The placeholders the mappers emit when metadata is missing. Asserting merely "not null" would + // pass on "???" — i.e. on a selection whose navigation was never loaded. + selectedName.ShouldNotContain("???", customMessage: $"{collectionType} resolved to a placeholder name"); + selectedName.ShouldNotStartWith("[unknown", customMessage: $"{collectionType} resolved to a placeholder"); + selectedName.ShouldNotStartWith( + "[unsupported media type", + customMessage: $"{collectionType} fell through the media-item switch"); + } + + private async Task SeedSelection(CollectionType collectionType) + { + await using TvContext context = Db.CreateContext(); + + switch (collectionType) + { + case CollectionType.Collection: + context.Collections.Add(new Collection + { + Id = SelectedId, + Name = "Selected collection", + MediaItems = [] + }); + break; + case CollectionType.MultiCollection: + context.MultiCollections.Add(new MultiCollection + { + Id = SelectedId, + Name = "Selected multi collection" + }); + break; + case CollectionType.SmartCollection: + context.SmartCollections.Add(new SmartCollection + { + Id = SelectedId, + Name = "Selected smart collection", + Query = "tag:family" + }); + break; + case CollectionType.TelevisionShow: + context.Shows.Add(new Show + { + Id = SelectedId, + ShowMetadata = [new ShowMetadata { Title = "Selected show", Year = 2020 }] + }); + break; + case CollectionType.TelevisionSeason: + context.Seasons.Add(new Season + { + Id = SelectedId, + SeasonNumber = 3, + Show = new Show + { + Id = 900, + ShowMetadata = [new ShowMetadata { Title = "Parent show", Year = 2020 }] + } + }); + break; + case CollectionType.Artist: + context.Artists.Add(new Artist + { + Id = SelectedId, + ArtistMetadata = [new ArtistMetadata { Title = "Selected artist" }] + }); + break; + case CollectionType.Movie: + context.Movies.Add(new Movie + { + Id = SelectedId, + MovieMetadata = [new MovieMetadata { Title = "Selected movie", Year = 2019 }] + }); + break; + case CollectionType.Episode: + context.Episodes.Add(new Episode + { + Id = SelectedId, + EpisodeMetadata = [new EpisodeMetadata { Title = "Selected episode", EpisodeNumber = 4 }], + Season = new Season + { + Id = 901, + SeasonNumber = 2, + Show = new Show + { + Id = 902, + ShowMetadata = [new ShowMetadata { Title = "Episode's show", Year = 2018 }] + } + } + }); + break; + case CollectionType.MusicVideo: + context.MusicVideos.Add(new MusicVideo + { + Id = SelectedId, + MusicVideoMetadata = [new MusicVideoMetadata { Title = "Selected music video" }], + Artist = new Artist + { + Id = 903, + ArtistMetadata = [new ArtistMetadata { Title = "Video's artist" }] + } + }); + break; + case CollectionType.OtherVideo: + context.OtherVideos.Add(new OtherVideo + { + Id = SelectedId, + OtherVideoMetadata = [new OtherVideoMetadata { Title = "Selected other video" }] + }); + break; + case CollectionType.Song: + context.Songs.Add(new Song + { + Id = SelectedId, + SongMetadata = [new SongMetadata { Title = "Selected song", Artists = [] }] + }); + break; + case CollectionType.Image: + context.Images.Add(new Image + { + Id = SelectedId, + ImageMetadata = [new ImageMetadata { Title = "Selected image" }] + }); + break; + case CollectionType.RemoteStream: + context.RemoteStreams.Add(new RemoteStream + { + Id = SelectedId, + Url = "http://example.invalid/stream", + RemoteStreamMetadata = [new RemoteStreamMetadata { Title = "Selected remote stream" }] + }); + break; + default: + throw new AssertionException( + $"{collectionType} is a supported rerun-collection selection type but this suite does " + + "not know how to seed it — teach SeedSelection about it rather than narrowing the matrix."); + } + + await context.SaveChangesAsync(); + } + + private async Task SeedRerunCollection(int id, CollectionType collectionType) + { + await using TvContext context = Db.CreateContext(); + + var rerunCollection = new RerunCollection + { + Id = id, + Name = "Rerun", + CollectionType = collectionType, + FirstRunPlaybackOrder = PlaybackOrder.Chronological, + RerunPlaybackOrder = PlaybackOrder.Chronological + }; + + switch (collectionType) + { + case CollectionType.Collection: + rerunCollection.CollectionId = SelectedId; + break; + case CollectionType.MultiCollection: + rerunCollection.MultiCollectionId = SelectedId; + break; + case CollectionType.SmartCollection: + rerunCollection.SmartCollectionId = SelectedId; + break; + default: + rerunCollection.MediaItemId = SelectedId; + break; + } + + context.RerunCollections.Add(rerunCollection); + await context.SaveChangesAsync(); + } +} diff --git a/docs/api-conventions.md b/docs/api-conventions.md index f60514eb0..fabd7bc27 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -146,6 +146,34 @@ Exemplars: `Brief`. `Remediation.Kind` is a mapped **string** ("ExternalDoc"/"AppRoute"), not a wire enum — same pattern as `Status`. See `decisions.md` 2026-07-17 (#164). +### 2a. Flattening a tagged-union selection (read path) + +Several DTOs flatten a "exactly one of these navigations is populated" tagged union to a single +`selectedId` + `selectedName` pair (`RerunCollectionResponseModel`, and the playlist-item shape). +Two rules, both learned from #671, where the list endpoint returned a null selection for **every** +row and the detail GET 500'd for two of its media types: + +- **One include chain per projected aggregate, shared by every handler that projects it.** Put it in + a `QueryExtensions` extension method and call it from the list handler *and* the by-id + handler. Exemplars: `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, + `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()`. Two hand-maintained chains + drift, and the one that drifts is usually the paged list, whose rows are individually less + obviously wrong. Applying it before `Skip`/`Take` is fine — EF applies the includes to the paged + subquery, so the cost is bounded by `PageSize`, not by the table. +- **The id and the name must not share a single point of failure.** When both are read off the same + eager-loaded navigation, the id is only ever as available as the name — so an un-included type + doesn't merely render an unlabelled badge, it drops the selected id, and an editor that + round-trips that id silently clears the user's stored selection. Accordingly a media-item + flattening switch never ends in `_ => null`: an unrecognized subtype keeps its id and takes a + conspicuous `[unsupported media type: X]` name. Throwing is the wrong lever — it would fail an + entire paged GET over one unreadable row. The shared switch is + `MediaCollections.Mapper.ProjectMediaItemToViewModel`. + +Corollary for the mappers themselves: `MediaItems.Mapper`'s projections are reached from handlers +whose include chains differ, so every metadata navigation is read through `Optional(...).Flatten()` +and degrades to the `"???"` placeholder rather than throwing. A bare `x.Season.Show.ShowMetadata` +inside a projection is a latent 500 on some other caller's GET. + ## 3. Error mapping Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 4fafde0f7..ceab1df90 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -31,6 +31,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `api.search-field-values-sources` | `GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for a narrow allow-list of catalog fields (never the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source (`title`, `show_title` only); `limit` clamped to `[1, 50]` (default 50). The FINAL filter, dedup and ordering applied to the response are ORDINAL (`OrdinalIgnoreCase` / `StringComparer.Ordinal`), never current-culture, because `UseRequestLocalization` makes the culture caller-controlled — scoped to the in-memory stages on purpose: a field sourced by a plain EF query is filtered and truncated by the DATABASE collation first (SQLite's `LOWER()` is ASCII-only), which ordinal semantics downstream cannot undo (ersatztv#668). A field whose values live in an EF **primitive collection** (one JSON array per row in a single column: `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) is served, not 404'd, as bounded best-effort, and its rows are read by a keyset page carrying **NO RESIDUAL predicate** — `SELECT Id, AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch`, no `LIKE`, no `LOWER`, not even `IS NOT NULL`. The cursor is itself a predicate, but a SEEKABLE one on the ordering key: it positions the scan and never discards a row. A RESIDUAL predicate discards rows the engine already produced, and `LIMIT` truncates only the survivors — so with one present it bounds the OUTPUT rather than the row count. All selectivity is in memory. The guarantee is scoped: **at most 20,000 LOGICAL rows returned/materialized and at most 10 round trips (11 for `artist`)** — NOT bounded physical work and NOT bounded bytes, because MySQL traverses deleted-but-unpurged index records and the `TEXT`/`longtext` payload width is unrestricted. The walk pages 2,000 rows at a time, stopping on the first of enough distinct matches, a short page, or the ceiling. | 2026-07-26 | [link](records/api/search-field-values-sources.md) | | `api.search-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `show_genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`'s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite's `LOWER()` folds ASCII only (`lower('Édith')` is `'Édith'` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware, so `LOWER('Édith')` really is `'édith'` and the existing predicate reaches the row unaided. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT AS Value FROM WHERE [ AND] etv_upper() LIKE @Pattern ESCAPE '\' ORDER BY LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today's EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record's "Known limitation inherited, not introduced" clause; everything else it settles still holds. | 2026-07-27 | [link](records/api/search-field-values-unicode-fold.md) | | `api.search-paging-cap` | Search stays capped at 100 items per media kind; an overflowing kind's "See all" reuses library-browse paging instead of adding new API surface. | 2026-07-11 | [link](records/api/search-paging-cap.md) | +| `api.selection-projection-include-chain` | Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET. | 2026-07-28 | [link](records/api/selection-projection-include-chain.md) | | `api.versioning-v1` | The entire `/api` surface is versioned to `/api/v1` uniformly (no unversioned corner); legacy unversioned callers are rewritten in-pipeline (not redirected) with Deprecation/Link/Sunset headers, and post-freeze `/api/v1` is additive-only — a breaking change requires `/api/v2`. | 2026-07-13 | [link](records/api/versioning-v1.md) | | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | diff --git a/docs/decisions/records/api/selection-projection-include-chain.md b/docs/decisions/records/api/selection-projection-include-chain.md new file mode 100644 index 000000000..2754dea30 --- /dev/null +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -0,0 +1,53 @@ +--- +key: api.selection-projection-include-chain +title: '2026-07-28 — A tagged-union selection is projected through one shared include chain, and its flattening switch never falls through to null (#671)' +status: active +since: '2026-07-28' +supersedes: none +superseded-by: none +rule: 'Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET.' +signals: 'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection · paths: `ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues: #671, #651, #229' +mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`); `RerunCollectionRequestMapping.IsSupportedSelectionType`' +--- + +Applies the `#229` shared-include-chain remedy to the READ path. That record framed the rule as a +write-path concern (project the mutation response through the include chain the GET uses); #671 is +its mirror image, where the GET itself under-loaded. + +## The coupling that hid the bug + +The id and the display name are read off the SAME eager-loaded navigation, so the id is only ever as +available as the name. There is no state where the API knows WHICH item is selected but not what it +is called. That is why the client-side symptom looked like a naming problem: an unlabelled badge. +The real harm is one level down — the selected id is null too, and an editor that round-trips it +silently clears the stored selection. #651 added a client-side merge-instead-of-replace guard that +made this survivable; that guard stays, but it was a client-side patch over a server-side defect. + +The corollary is the rule: never let the id and the name share a single point of failure. Hence the +`_ => null` ban — an unrecognized subtype surrenders its NAME, never its ID. + +## Why fail-soft rather than a throw + +The Done-when asked for the unhandled case to be handled or made loud. A throw is loud but fails an +entire paged GET over one bad row, on a read path. A conspicuous placeholder name is loud in the UI, +greppable in logs, and preserves the id — the thing that actually matters. Recorded because a future +session could reasonably reach for `NotSupportedException` here, as the sibling +`ProgramSchedules.Mapper` item-type switch does; that one dispatches on the ITEM type, an internal +closed set, where a throw is correct. + +## Scope deliberately not widened + +Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five in +`Scheduling.Mapper`) and all handle only Show/Season/Artist. That is NOT the same oversight: those +call sites genuinely restrict selection to those three types, and their handlers load a matching +chain. Only the RerunCollection and PlaylistItem switches span the full set, which is why exactly +those two were merged. `GetPlaylistItemsHandler` already loaded the full graph, so PlaylistItem was +never broken in production — it shared the latent `RemoteStream` gap and now shares the fix. + +## Verification worth repeating + +Each mechanism was removed in turn and the suite quoted red before the fix was restored: stripping +the list include chain failed all 13 types with "lost its selected id"; restoring the original +four-type by-id chain failed exactly the six the issue named; and reverting only the three bare +dereferences reproduced the `NullReferenceException` for Episode and MusicVideo. A green new test +over a read path proves little until the mechanism it covers has been shown to fail without it. -- 2.47.3 From 017ef988d00b094c979b196cd501c395757d4731 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 19:00:08 +0200 Subject: [PATCH 2/5] fix(671): review round 1 -- pin exact names, complete the playlist include Cross-family (Codex) adversarial review of 8523088ce. All three findings taken: - The name assertion only checked "not a placeholder", so it could not see a missing NESTED include leg: dropping Episode -> Season -> Show still renders "s00e04 - Selected episode", which contains no placeholder marker and passed. Now every type pins its whole expected string; re-removing that leg fails, as verified before restoring it. - Widening the shared switch with a RemoteStream arm put `GetPlaylistItemsHandler` one include short -- it loaded metadata for the other nine types, so playlist RemoteStream names alone would have degraded to "???". - `?? 0` rendered an unloaded Season as "s00", which conventionally means Specials and so fabricated plausible-looking real data; it now renders "s??". Co-Authored-By: Claude Opus 5 (1M context) --- .../Queries/GetPlaylistItemsHandler.cs | 4 ++ ErsatzTV.Application/MediaItems/Mapper.cs | 6 ++- .../RerunCollectionQueryHandlerTests.cs | 40 ++++++++++++++----- .../api/selection-projection-include-chain.md | 14 ++++++- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/ErsatzTV.Application/MediaCollections/Queries/GetPlaylistItemsHandler.cs b/ErsatzTV.Application/MediaCollections/Queries/GetPlaylistItemsHandler.cs index e741ba2b1..60ea4c542 100644 --- a/ErsatzTV.Application/MediaCollections/Queries/GetPlaylistItemsHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Queries/GetPlaylistItemsHandler.cs @@ -55,6 +55,10 @@ public class GetPlaylistItemsHandler(IDbContextFactory dbContextFacto .Include(i => i.MediaItem) .ThenInclude(i => (i as Image).ImageMetadata) .ThenInclude(mm => mm.Artwork) + // RemoteStream is projected by the shared ProjectMediaItemToViewModel switch as of #671; + // without its metadata the name would degrade to "???" here while every sibling type resolves. + .Include(i => i.MediaItem) + .ThenInclude(i => (i as RemoteStream).RemoteStreamMetadata) .ToListAsync(cancellationToken); return allItems.Map(Mapper.ProjectToViewModel).ToList(); diff --git a/ErsatzTV.Application/MediaItems/Mapper.cs b/ErsatzTV.Application/MediaItems/Mapper.cs index 57760b12b..6b339d897 100644 --- a/ErsatzTV.Application/MediaItems/Mapper.cs +++ b/ErsatzTV.Application/MediaItems/Mapper.cs @@ -109,7 +109,11 @@ internal static class Mapper var numbersString = $"e{string.Join('e', episodeNumbers.Map(n => $"{n:00}"))}"; var titlesString = $"{string.Join('/', episodeTitles)}"; - return $"{showTitle}s{e.Season?.SeasonNumber ?? 0:00}{numbersString} - {titlesString}"; + // "s00" conventionally means Specials, so an unloaded Season must not borrow it — that would + // fabricate plausible-looking real data. Render the season as explicitly unknown instead. + string seasonNumber = e.Season is null ? "??" : $"{e.Season.SeasonNumber:00}"; + + return $"{showTitle}s{seasonNumber}{numbersString} - {titlesString}"; } private static string MusicVideoTitle(MusicVideo mv) diff --git a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs index be00bcaad..c928dfb64 100644 --- a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs +++ b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs @@ -83,6 +83,32 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase AssertSelectionResolved(result.Page[0], collectionType); } + /// + /// The exact projected name per type. Pinning the whole string — rather than merely asserting + /// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping + /// Episode → Season → Show still yields the non-placeholder "s00e04 - Selected episode", and + /// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a + /// looser assertion while having lost real data. + /// + private static string ExpectedName(CollectionType collectionType) => + collectionType switch + { + CollectionType.Collection => "Selected collection", + CollectionType.MultiCollection => "Selected multi collection", + CollectionType.SmartCollection => "Selected smart collection", + CollectionType.TelevisionShow => "Selected show (2020)", + CollectionType.TelevisionSeason => "Parent show (2020) - Season 3", + CollectionType.Artist => "Selected artist", + CollectionType.Movie => "Selected movie (2019)", + CollectionType.Episode => "Episode's show - s02e04 - Selected episode", + CollectionType.MusicVideo => "Video's artist - Selected music video", + CollectionType.OtherVideo => "Selected other video", + CollectionType.Song => "Song artist - Selected song", + CollectionType.Image => "Selected image", + CollectionType.RemoteStream => "Selected remote stream", + _ => throw new AssertionException($"No expected name pinned for {collectionType}") + }; + /// /// Mirrors RerunCollectionController.ProjectToResponseModel, which flattens the tagged /// union to the single selectedId / selectedName pair the SPA consumes. The id is @@ -102,16 +128,7 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase ?? vm.MediaItem?.Name; selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id"); - - selectedName.ShouldNotBeNullOrWhiteSpace($"{collectionType} lost its selected name"); - - // The placeholders the mappers emit when metadata is missing. Asserting merely "not null" would - // pass on "???" — i.e. on a selection whose navigation was never loaded. - selectedName.ShouldNotContain("???", customMessage: $"{collectionType} resolved to a placeholder name"); - selectedName.ShouldNotStartWith("[unknown", customMessage: $"{collectionType} resolved to a placeholder"); - selectedName.ShouldNotStartWith( - "[unsupported media type", - customMessage: $"{collectionType} fell through the media-item switch"); + selectedName.ShouldBe(ExpectedName(collectionType), $"{collectionType} projected the wrong name"); } private async Task SeedSelection(CollectionType collectionType) @@ -216,7 +233,8 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase context.Songs.Add(new Song { Id = SelectedId, - SongMetadata = [new SongMetadata { Title = "Selected song", Artists = [] }] + SongMetadata = + [new SongMetadata { Title = "Selected song", Artists = ["Song artist"] }] }); break; case CollectionType.Image: diff --git a/docs/decisions/records/api/selection-projection-include-chain.md b/docs/decisions/records/api/selection-projection-include-chain.md index 2754dea30..62e8eb1f5 100644 --- a/docs/decisions/records/api/selection-projection-include-chain.md +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -41,8 +41,12 @@ Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five `Scheduling.Mapper`) and all handle only Show/Season/Artist. That is NOT the same oversight: those call sites genuinely restrict selection to those three types, and their handlers load a matching chain. Only the RerunCollection and PlaylistItem switches span the full set, which is why exactly -those two were merged. `GetPlaylistItemsHandler` already loaded the full graph, so PlaylistItem was -never broken in production — it shared the latent `RemoteStream` gap and now shares the fix. +those two were merged. `GetPlaylistItemsHandler` already loaded the other nine types, so PlaylistItem +was never broken in production; adding the shared `RemoteStream` arm did oblige it to gain the one +matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`. + +**Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not +free: each consumer must be re-checked against the navigations the new arm dereferences. ## Verification worth repeating @@ -51,3 +55,9 @@ the list include chain failed all 13 types with "lost its selected id"; restorin four-type by-id chain failed exactly the six the issue named; and reverting only the three bare dereferences reproduced the `NullReferenceException` for Episode and MusicVideo. A green new test over a read path proves little until the mechanism it covers has been shown to fail without it. + +The per-type name assertion pins the WHOLE expected string, not merely "is not a placeholder". A +cross-family review caught that the looser form could not see a missing NESTED leg: drop +Episode → Season → Show and the projection still reads `s00e04 - Selected episode`, which contains +no placeholder marker and would have passed. Related: an absent Season renders `s??`, never `s00`, +since `s00` conventionally means Specials and would fabricate plausible-looking real data. -- 2.47.3 From 572737a29e57cdcfe5c655f486342d9352b27ba5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 19:33:52 +0200 Subject: [PATCH 3/5] fix(671): review round 2 -- guard Song.Artists, cover the second consumer Cold independent review of 017ef988d. Three findings taken, one filed out. The important one is a regression this branch INTRODUCED. `SongMetadata.Artists` is a nullable EF primitive collection (JSON in one column, not a navigation) that FallbackMetadataProvider leaves unassigned when a song's tags fail to read, and `string.Join` throws ArgumentNullException on a null sequence. The rerun list previously did not load SongMetadata at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would have failed the whole page. Confirmed by reverting the guard: ArgumentNullException, parameter 'values'. The file header claiming every member was guarded was false. The empty case is filtered too, so an artist-less song loses its bare " - ". Second: `GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator), so the RemoteStream include added last round was discharged by inspection -- the same method that produced #671. It now runs the same 13-type matrix via a shared SelectionSeedData; removing the include fails that matrix. Third: dropped the dead `(i as Season).SeasonMetadata` include leg -- the Season projection reads Show.ShowMetadata and the scalar SeasonNumber, never SeasonMetadata. Filed #690 for the pre-existing, out-of-scope finding: the paged TotalCount ignores the search query, so the SPA renders empty pages. Co-Authored-By: Claude Opus 5 (1M context) --- .../RerunCollectionQueryExtensions.cs | 4 +- ErsatzTV.Application/MediaItems/Mapper.cs | 8 +- .../GetPlaylistItemsHandlerTests.cs | 89 +++++++ .../RerunCollectionQueryHandlerTests.cs | 218 ++++-------------- ErsatzTV.Tests/Support/SelectionSeedData.cs | 207 +++++++++++++++++ docs/api-conventions.md | 7 + .../api/selection-projection-include-chain.md | 19 +- 7 files changed, 374 insertions(+), 178 deletions(-) create mode 100644 ErsatzTV.Tests/Application/MediaCollections/GetPlaylistItemsHandlerTests.cs create mode 100644 ErsatzTV.Tests/Support/SelectionSeedData.cs diff --git a/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs b/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs index 3a33d1919..da03c501a 100644 --- a/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs +++ b/ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs @@ -28,8 +28,8 @@ internal static class RerunCollectionQueryExtensions .ThenInclude(i => (i as Movie).MovieMetadata) .Include(c => c.MediaItem) .ThenInclude(i => (i as Show).ShowMetadata) - .Include(c => c.MediaItem) - .ThenInclude(i => (i as Season).SeasonMetadata) + // No (i as Season).SeasonMetadata leg on purpose: ProjectToViewModel(Season) builds its name + // from Show.ShowMetadata and the scalar SeasonNumber, and never reads SeasonMetadata. .Include(c => c.MediaItem) .ThenInclude(i => (i as Season).Show) .ThenInclude(s => s.ShowMetadata) diff --git a/ErsatzTV.Application/MediaItems/Mapper.cs b/ErsatzTV.Application/MediaItems/Mapper.cs index 6b339d897..e04c56608 100644 --- a/ErsatzTV.Application/MediaItems/Mapper.cs +++ b/ErsatzTV.Application/MediaItems/Mapper.cs @@ -127,8 +127,14 @@ internal static class Mapper private static string SongTitle(Song s) { + // Artists is a NULLABLE primitive collection, not a navigation: a song whose tags failed to read + // is persisted by FallbackMetadataProvider with Artists never assigned, and string.Join throws + // ArgumentNullException on a null sequence. Filtering the empty case too avoids prefixing an + // artist-less song with a bare " - ". string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone() - .Map(sm => $"{string.Join(", ", sm.Artists)} - ") + .Map(sm => Optional(sm.Artists).Flatten().ToList()) + .Filter(artists => artists.Count > 0) + .Map(artists => $"{string.Join(", ", artists)} - ") .IfNone(string.Empty); return Optional(s.SongMetadata).Flatten().HeadOrNone() .Map(sm => $"{songArtist}{sm.Title ?? string.Empty}") diff --git a/ErsatzTV.Tests/Application/MediaCollections/GetPlaylistItemsHandlerTests.cs b/ErsatzTV.Tests/Application/MediaCollections/GetPlaylistItemsHandlerTests.cs new file mode 100644 index 000000000..d806172c2 --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/GetPlaylistItemsHandlerTests.cs @@ -0,0 +1,89 @@ +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.MediaCollections; + +/// +/// The second consumer of the shared ProjectMediaItemToViewModel switch (issue #671). +/// GetPlaylistItemsHandler had no handler-level test — the controller tests stub the mediator +/// and never execute the query — so the only symptom of a missing include here was a silent "???" +/// name that nothing in the suite could see. Widening the shared switch with a RemoteStream arm +/// obliged this handler to gain a matching include; proving that by inspection would have repeated +/// the very method that produced #671, so it gets the same full matrix the rerun handlers get. +/// +[TestFixture] +public class GetPlaylistItemsHandlerTests : MediaCollectionHandlerTestBase +{ + private static IEnumerable SupportedSelectionTypes => SelectionSeedData.SupportedSelectionTypes; + + [TestCaseSource(nameof(SupportedSelectionTypes))] + public async Task GetPlaylistItems_Should_Resolve_The_Selection(CollectionType collectionType) + { + await SeedSelection(collectionType); + await SeedPlaylistItem(collectionType); + + var handler = new GetPlaylistItemsHandler(Db.Factory); + + List items = + await handler.Handle(new GetPlaylistItems(1), CancellationToken.None); + + items.Count.ShouldBe(1); + + PlaylistItemViewModel item = items[0]; + + int? selectedId = item.Collection?.Id + ?? item.MultiCollection?.Id + ?? item.SmartCollection?.Id + ?? item.MediaItem?.MediaItemId; + + string selectedName = item.Collection?.Name + ?? item.MultiCollection?.Name + ?? item.SmartCollection?.Name + ?? item.MediaItem?.Name; + + selectedId.ShouldBe(SelectionSeedData.SelectedId, $"{collectionType} lost its selected id"); + selectedName.ShouldBe( + SelectionSeedData.ExpectedName(collectionType), + $"{collectionType} projected the wrong name"); + } + + private async Task SeedSelection(CollectionType collectionType) + { + await using TvContext context = Db.CreateContext(); + await SelectionSeedData.SeedSelection(context, collectionType); + } + + private async Task SeedPlaylistItem(CollectionType collectionType) + { + await using TvContext context = Db.CreateContext(); + + var item = new PlaylistItem + { + Id = 1, + Index = 0, + PlaylistId = 1, + CollectionType = collectionType, + PlaybackOrder = PlaybackOrder.Chronological + }; + + SelectionSeedData.ApplySelection( + collectionType, + v => item.CollectionId = v, + v => item.MultiCollectionId = v, + v => item.SmartCollectionId = v, + v => item.MediaItemId = v); + + context.Playlists.Add(new Playlist + { + Id = 1, + Name = "Playlist", + Items = [item] + }); + + await context.SaveChangesAsync(); + } +} diff --git a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs index c928dfb64..612864b42 100644 --- a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs +++ b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs @@ -1,5 +1,4 @@ using ErsatzTV.Application.MediaCollections; -using ErsatzTV.Controllers.Api.Requests; using ErsatzTV.Core.Domain; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; @@ -12,23 +11,18 @@ namespace ErsatzTV.Tests.Application.MediaCollections; /// /// Read-path coverage for the two rerun-collection query handlers (issue #671). The defect was /// precisely that nobody enumerated the selection types: the list handler eager-loaded nothing, and -/// the by-id handler loaded metadata for only four of the ten media types. So the matrix here is -/// derived from the production predicate rather than hand-listed — a newly-supported -/// joins it automatically and fails loudly in SeedSelection -/// until someone teaches the suite how to seed it. +/// the by-id handler loaded metadata for only four of the ten media types. So the matrix is derived +/// from the production predicate (see ) rather than hand-listed. /// [TestFixture] public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase { - private const int SelectedId = 42; - - private static IEnumerable SupportedSelectionTypes => - Enum.GetValues().Where(RerunCollectionRequestMapping.IsSupportedSelectionType); + private static IEnumerable SupportedSelectionTypes => SelectionSeedData.SupportedSelectionTypes; /// /// Completeness guard. Without it, a change that narrowed IsSupportedSelectionType would /// shrink the matrix silently and every remaining case would still pass — the "filters on the - /// property it asserts" failure mode. + /// property it asserts" failure mode. Set equality, so it fails on widening too. /// [Test] public void Supported_Selection_Types_Should_Be_The_Full_Documented_Set() @@ -84,30 +78,37 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase } /// - /// The exact projected name per type. Pinning the whole string — rather than merely asserting - /// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping - /// Episode → Season → Show still yields the non-placeholder "s00e04 - Selected episode", and - /// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a - /// looser assertion while having lost real data. + /// SongMetadata.Artists is a NULLABLE primitive collection, and a song whose tags failed to + /// read is persisted with it never assigned. Before #671 the rerun list did not load SongMetadata + /// at all, so this was unreachable there; eager-loading it made a latent `string.Join` throw into a + /// live 500 that would take down the whole page. /// - private static string ExpectedName(CollectionType collectionType) => - collectionType switch + [TestCase(null, "Selected song", TestName = "GetById_Song_With_Null_Artists_Should_Not_Throw")] + [TestCase(new string[] { }, "Selected song", TestName = "GetById_Song_With_No_Artists_Should_Not_Prefix")] + public async Task GetById_Should_Tolerate_Song_Artists(string[] artists, string expectedName) + { + await using (TvContext context = Db.CreateContext()) { - CollectionType.Collection => "Selected collection", - CollectionType.MultiCollection => "Selected multi collection", - CollectionType.SmartCollection => "Selected smart collection", - CollectionType.TelevisionShow => "Selected show (2020)", - CollectionType.TelevisionSeason => "Parent show (2020) - Season 3", - CollectionType.Artist => "Selected artist", - CollectionType.Movie => "Selected movie (2019)", - CollectionType.Episode => "Episode's show - s02e04 - Selected episode", - CollectionType.MusicVideo => "Video's artist - Selected music video", - CollectionType.OtherVideo => "Selected other video", - CollectionType.Song => "Song artist - Selected song", - CollectionType.Image => "Selected image", - CollectionType.RemoteStream => "Selected remote stream", - _ => throw new AssertionException($"No expected name pinned for {collectionType}") - }; + context.Songs.Add(new Song + { + Id = SelectionSeedData.SelectedId, + SongMetadata = [new SongMetadata { Title = "Selected song", Artists = artists?.ToList() }] + }); + await context.SaveChangesAsync(); + } + + await SeedRerunCollection(1, CollectionType.Song); + + var handler = new GetRerunCollectionByIdHandler(Db.Factory); + + Option result = + await handler.Handle(new GetRerunCollectionById(1), CancellationToken.None); + + RerunCollectionViewModel vm = result.IfNone(() => throw new AssertionException("Expected a result")); + vm.MediaItem.ShouldNotBeNull(); + vm.MediaItem.MediaItemId.ShouldBe(SelectionSeedData.SelectedId); + vm.MediaItem.Name.ShouldBe(expectedName); + } /// /// Mirrors RerunCollectionController.ProjectToResponseModel, which flattens the tagged @@ -127,138 +128,16 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase ?? vm.SmartCollection?.Name ?? vm.MediaItem?.Name; - selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id"); - selectedName.ShouldBe(ExpectedName(collectionType), $"{collectionType} projected the wrong name"); + selectedId.ShouldBe(SelectionSeedData.SelectedId, $"{collectionType} lost its selected id"); + selectedName.ShouldBe( + SelectionSeedData.ExpectedName(collectionType), + $"{collectionType} projected the wrong name"); } private async Task SeedSelection(CollectionType collectionType) { await using TvContext context = Db.CreateContext(); - - switch (collectionType) - { - case CollectionType.Collection: - context.Collections.Add(new Collection - { - Id = SelectedId, - Name = "Selected collection", - MediaItems = [] - }); - break; - case CollectionType.MultiCollection: - context.MultiCollections.Add(new MultiCollection - { - Id = SelectedId, - Name = "Selected multi collection" - }); - break; - case CollectionType.SmartCollection: - context.SmartCollections.Add(new SmartCollection - { - Id = SelectedId, - Name = "Selected smart collection", - Query = "tag:family" - }); - break; - case CollectionType.TelevisionShow: - context.Shows.Add(new Show - { - Id = SelectedId, - ShowMetadata = [new ShowMetadata { Title = "Selected show", Year = 2020 }] - }); - break; - case CollectionType.TelevisionSeason: - context.Seasons.Add(new Season - { - Id = SelectedId, - SeasonNumber = 3, - Show = new Show - { - Id = 900, - ShowMetadata = [new ShowMetadata { Title = "Parent show", Year = 2020 }] - } - }); - break; - case CollectionType.Artist: - context.Artists.Add(new Artist - { - Id = SelectedId, - ArtistMetadata = [new ArtistMetadata { Title = "Selected artist" }] - }); - break; - case CollectionType.Movie: - context.Movies.Add(new Movie - { - Id = SelectedId, - MovieMetadata = [new MovieMetadata { Title = "Selected movie", Year = 2019 }] - }); - break; - case CollectionType.Episode: - context.Episodes.Add(new Episode - { - Id = SelectedId, - EpisodeMetadata = [new EpisodeMetadata { Title = "Selected episode", EpisodeNumber = 4 }], - Season = new Season - { - Id = 901, - SeasonNumber = 2, - Show = new Show - { - Id = 902, - ShowMetadata = [new ShowMetadata { Title = "Episode's show", Year = 2018 }] - } - } - }); - break; - case CollectionType.MusicVideo: - context.MusicVideos.Add(new MusicVideo - { - Id = SelectedId, - MusicVideoMetadata = [new MusicVideoMetadata { Title = "Selected music video" }], - Artist = new Artist - { - Id = 903, - ArtistMetadata = [new ArtistMetadata { Title = "Video's artist" }] - } - }); - break; - case CollectionType.OtherVideo: - context.OtherVideos.Add(new OtherVideo - { - Id = SelectedId, - OtherVideoMetadata = [new OtherVideoMetadata { Title = "Selected other video" }] - }); - break; - case CollectionType.Song: - context.Songs.Add(new Song - { - Id = SelectedId, - SongMetadata = - [new SongMetadata { Title = "Selected song", Artists = ["Song artist"] }] - }); - break; - case CollectionType.Image: - context.Images.Add(new Image - { - Id = SelectedId, - ImageMetadata = [new ImageMetadata { Title = "Selected image" }] - }); - break; - case CollectionType.RemoteStream: - context.RemoteStreams.Add(new RemoteStream - { - Id = SelectedId, - Url = "http://example.invalid/stream", - RemoteStreamMetadata = [new RemoteStreamMetadata { Title = "Selected remote stream" }] - }); - break; - default: - throw new AssertionException( - $"{collectionType} is a supported rerun-collection selection type but this suite does " + - "not know how to seed it — teach SeedSelection about it rather than narrowing the matrix."); - } - - await context.SaveChangesAsync(); + await SelectionSeedData.SeedSelection(context, collectionType); } private async Task SeedRerunCollection(int id, CollectionType collectionType) @@ -274,21 +153,12 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase RerunPlaybackOrder = PlaybackOrder.Chronological }; - switch (collectionType) - { - case CollectionType.Collection: - rerunCollection.CollectionId = SelectedId; - break; - case CollectionType.MultiCollection: - rerunCollection.MultiCollectionId = SelectedId; - break; - case CollectionType.SmartCollection: - rerunCollection.SmartCollectionId = SelectedId; - break; - default: - rerunCollection.MediaItemId = SelectedId; - break; - } + SelectionSeedData.ApplySelection( + collectionType, + v => rerunCollection.CollectionId = v, + v => rerunCollection.MultiCollectionId = v, + v => rerunCollection.SmartCollectionId = v, + v => rerunCollection.MediaItemId = v); context.RerunCollections.Add(rerunCollection); await context.SaveChangesAsync(); diff --git a/ErsatzTV.Tests/Support/SelectionSeedData.cs b/ErsatzTV.Tests/Support/SelectionSeedData.cs new file mode 100644 index 000000000..ddcbb7f79 --- /dev/null +++ b/ErsatzTV.Tests/Support/SelectionSeedData.cs @@ -0,0 +1,207 @@ +using ErsatzTV.Controllers.Api.Requests; +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using NUnit.Framework; + +namespace ErsatzTV.Tests.Support; + +/// +/// One selection-type matrix, shared by every fixture that exercises a tagged-union selection +/// (rerun collections and playlist items). Both consumers of +/// MediaCollections.Mapper.ProjectMediaItemToViewModel are proved against the SAME data, so +/// widening the shared switch cannot be discharged for the second consumer by inspection alone — +/// which is the method that produced #671 in the first place. +/// +internal static class SelectionSeedData +{ + public const int SelectedId = 42; + + /// + /// Derived from production rather than hand-listed, so a newly-supported type joins the matrix + /// automatically and trips the default: arms below until someone teaches them about it. + /// + public static IEnumerable SupportedSelectionTypes => + Enum.GetValues().Where(RerunCollectionRequestMapping.IsSupportedSelectionType); + + /// + /// The exact projected name per type. Pinning the whole string — rather than merely asserting + /// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping + /// Episode → Season → Show still yields the placeholder-free "s??e04 - Selected episode", and + /// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a + /// looser assertion while having lost real data. + /// + public static string ExpectedName(CollectionType collectionType) => + collectionType switch + { + CollectionType.Collection => "Selected collection", + CollectionType.MultiCollection => "Selected multi collection", + CollectionType.SmartCollection => "Selected smart collection", + CollectionType.TelevisionShow => "Selected show (2020)", + CollectionType.TelevisionSeason => "Parent show (2020) - Season 3", + CollectionType.Artist => "Selected artist", + CollectionType.Movie => "Selected movie (2019)", + CollectionType.Episode => "Episode's show - s02e04 - Selected episode", + CollectionType.MusicVideo => "Video's artist - Selected music video", + CollectionType.OtherVideo => "Selected other video", + CollectionType.Song => "Song artist - Selected song", + CollectionType.Image => "Selected image", + CollectionType.RemoteStream => "Selected remote stream", + _ => throw new AssertionException($"No expected name pinned for {collectionType}") + }; + + public static async Task SeedSelection(TvContext context, CollectionType collectionType) + { + switch (collectionType) + { + case CollectionType.Collection: + context.Collections.Add(new Collection + { + Id = SelectedId, + Name = "Selected collection", + MediaItems = [] + }); + break; + case CollectionType.MultiCollection: + context.MultiCollections.Add(new MultiCollection + { + Id = SelectedId, + Name = "Selected multi collection" + }); + break; + case CollectionType.SmartCollection: + context.SmartCollections.Add(new SmartCollection + { + Id = SelectedId, + Name = "Selected smart collection", + Query = "tag:family" + }); + break; + case CollectionType.TelevisionShow: + context.Shows.Add(new Show + { + Id = SelectedId, + ShowMetadata = [new ShowMetadata { Title = "Selected show", Year = 2020 }] + }); + break; + case CollectionType.TelevisionSeason: + context.Seasons.Add(new Season + { + Id = SelectedId, + SeasonNumber = 3, + Show = new Show + { + Id = 900, + ShowMetadata = [new ShowMetadata { Title = "Parent show", Year = 2020 }] + } + }); + break; + case CollectionType.Artist: + context.Artists.Add(new Artist + { + Id = SelectedId, + ArtistMetadata = [new ArtistMetadata { Title = "Selected artist" }] + }); + break; + case CollectionType.Movie: + context.Movies.Add(new Movie + { + Id = SelectedId, + MovieMetadata = [new MovieMetadata { Title = "Selected movie", Year = 2019 }] + }); + break; + case CollectionType.Episode: + context.Episodes.Add(new Episode + { + Id = SelectedId, + EpisodeMetadata = [new EpisodeMetadata { Title = "Selected episode", EpisodeNumber = 4 }], + Season = new Season + { + Id = 901, + SeasonNumber = 2, + Show = new Show + { + Id = 902, + ShowMetadata = [new ShowMetadata { Title = "Episode's show", Year = 2018 }] + } + } + }); + break; + case CollectionType.MusicVideo: + context.MusicVideos.Add(new MusicVideo + { + Id = SelectedId, + MusicVideoMetadata = [new MusicVideoMetadata { Title = "Selected music video" }], + Artist = new Artist + { + Id = 903, + ArtistMetadata = [new ArtistMetadata { Title = "Video's artist" }] + } + }); + break; + case CollectionType.OtherVideo: + context.OtherVideos.Add(new OtherVideo + { + Id = SelectedId, + OtherVideoMetadata = [new OtherVideoMetadata { Title = "Selected other video" }] + }); + break; + case CollectionType.Song: + context.Songs.Add(new Song + { + Id = SelectedId, + SongMetadata = + [new SongMetadata { Title = "Selected song", Artists = ["Song artist"] }] + }); + break; + case CollectionType.Image: + context.Images.Add(new Image + { + Id = SelectedId, + ImageMetadata = [new ImageMetadata { Title = "Selected image" }] + }); + break; + case CollectionType.RemoteStream: + context.RemoteStreams.Add(new RemoteStream + { + Id = SelectedId, + Url = "http://example.invalid/stream", + RemoteStreamMetadata = [new RemoteStreamMetadata { Title = "Selected remote stream" }] + }); + break; + default: + throw new AssertionException( + $"{collectionType} is a supported selection type but this suite does not know how " + + "to seed it — teach SeedSelection about it rather than narrowing the matrix."); + } + + await context.SaveChangesAsync(); + } + + /// + /// Assigns the one foreign key the tagged union uses for this type. Shared so the rerun and + /// playlist fixtures cannot disagree about which slot a type occupies. + /// + public static void ApplySelection( + CollectionType collectionType, + Action setCollectionId, + Action setMultiCollectionId, + Action setSmartCollectionId, + Action setMediaItemId) + { + switch (collectionType) + { + case CollectionType.Collection: + setCollectionId(SelectedId); + break; + case CollectionType.MultiCollection: + setMultiCollectionId(SelectedId); + break; + case CollectionType.SmartCollection: + setSmartCollectionId(SelectedId); + break; + default: + setMediaItemId(SelectedId); + break; + } + } +} diff --git a/docs/api-conventions.md b/docs/api-conventions.md index fabd7bc27..a02be0b11 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -174,6 +174,13 @@ whose include chains differ, so every metadata navigation is read through `Optio and degrades to the `"???"` placeholder rather than throwing. A bare `x.Season.Show.ShowMetadata` inside a projection is a latent 500 on some other caller's GET. +**And it is not only navigations.** `SongMetadata.Artists` is a nullable EF *primitive collection* +(a JSON array in one column), which `FallbackMetadataProvider` leaves unassigned for a song whose +tags failed to read — and `string.Join` throws `ArgumentNullException` on a null sequence, not a +`NullReferenceException`. Adding an include is therefore not automatically safe: it can promote a +latent throw on a previously-unloaded member into a live 500 that fails the whole page. When you +widen an include chain, audit what the newly-reachable projection dereferences. + ## 3. Error mapping Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of diff --git a/docs/decisions/records/api/selection-projection-include-chain.md b/docs/decisions/records/api/selection-projection-include-chain.md index 62e8eb1f5..0318809db 100644 --- a/docs/decisions/records/api/selection-projection-include-chain.md +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -46,7 +46,24 @@ was never broken in production; adding the shared `RemoteStream` arm did oblige matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`. **Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not -free: each consumer must be re-checked against the navigations the new arm dereferences. +free: each consumer must be re-checked against the navigations the new arm dereferences — and +discharged by a TEST, not by inspection, since inspection is the method that produced this bug. +`GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator), +so it gained the same 13-type matrix via the shared `SelectionSeedData`. + +## Adding an include can CREATE a 500 + +Eager-loading is not automatically safe. `SongMetadata.Artists` is a nullable EF **primitive +collection** — a JSON array in one column, not a navigation — that `FallbackMetadataProvider` leaves +unassigned whenever a song's tags fail to read, and `string.Join` throws `ArgumentNullException` (not +`NullReferenceException`) on a null sequence. The rerun list previously did not load `SongMetadata` +at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would +have failed the entire page. The guard is `Optional(sm.Artists).Flatten()`, and the empty case is +filtered too so an artist-less song is not prefixed with a bare `" - "`. + +The general rule: when you widen an include chain, audit what the newly-reachable projection +dereferences. A "null navigation" audit is not enough — nullable primitive collections throw a +different exception type and will not turn up in a grep for `NullReferenceException`. ## Verification worth repeating -- 2.47.3 From 34eee753b248568c776a15dea223f44bdb708c7a Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 20:09:07 +0200 Subject: [PATCH 4/5] fix(671): review round 3 -- sweep Artists by FIELD, correct the archaeology Cold review of 572737a29. Findings taken; two are corrections to my own claims. CORRECTION: "a regression this branch INTRODUCED" was wrong, and I verified the reviewer's counter-claim against origin/main before accepting it. That handler already included SongMetadata AND already routed Song there, so GET /api/v1/playlists/{id}/items was ALREADY a live 500 for a null-Artists song. This branch only made the same throw reachable on a second path. The record said so twice; both are fixed, because a wrong explanation outlives a wrong line. SWEEP: fixing one site left the mirror standing -- Playouts/Mapper.GetDisplayTitle had the identical unguarded join on a path that also eager-loads SongMetadata, so it too was live, feeding the playout guide, troubleshooting, media-item info and channel states. Guarded, with a unit test; reverting it reproduces ArgumentNullException. LibraryBrowseItemMapper already wrote `Artists ?? []`, so the nullability was known in-tree and these sites were simply unswept. Filed #691 for the remaining SongVideoGenerator dereferences on the playback path. Also: documented that the shared matrix is the RERUN predicate used as a superset for playlists (the playlist write path rejects RemoteStream today); noted the third, inert consumer ReplacePlaylistItemsHandler; added the new mechanisms to the record's `mechanics:` field; and trimmed the record under the 60-line prose ceiling -- it was tipping the corpus p90 above the ceiling and reddening the calibration test in scripts/tests, which passes again. Co-Authored-By: Claude Opus 5 (1M context) --- ErsatzTV.Application/Playouts/Mapper.cs | 8 +- .../PlayoutMapperDisplayTitleTests.cs | 61 +++++++++++ ErsatzTV.Tests/Support/SelectionSeedData.cs | 5 + .../api/selection-projection-include-chain.md | 102 ++++++++---------- 4 files changed, 120 insertions(+), 56 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index 2b9ef0969..276eb5a94 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -102,8 +102,14 @@ internal static class Mapper : $"{s} ({chapterTitle})") .IfNone("[unknown video]"); case Song s: + // SongMetadata.Artists is a NULLABLE primitive collection (FallbackMetadataProvider never + // assigns it for a song whose tags failed to read) and string.Join throws + // ArgumentNullException on a null sequence. SongMetadata IS eager-loaded on this path, so + // this was a LIVE 500 on the playout guide, not a latent one (issue #671). string songArtist = s.SongMetadata.HeadOrNone() - .Map(sm => $"{string.Join(", ", sm.Artists)} - ") + .Map(sm => Optional(sm.Artists).Flatten().ToList()) + .Filter(artists => artists.Count > 0) + .Map(artists => $"{string.Join(", ", artists)} - ") .IfNone(string.Empty); return s.SongMetadata.HeadOrNone() .Map(sm => $"{songArtist}{sm.Title ?? string.Empty}") diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs new file mode 100644 index 000000000..669d17f08 --- /dev/null +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs @@ -0,0 +1,61 @@ +using ErsatzTV.Core.Domain; +using LanguageExt; +using NUnit.Framework; +using Shouldly; +using Mapper = ErsatzTV.Application.Playouts.Mapper; + +namespace ErsatzTV.Tests.Application.Playouts; + +/// +/// SongMetadata.Artists is a nullable EF primitive collection that +/// FallbackMetadataProvider leaves unassigned for a song whose tags failed to read, and +/// string.Join throws on a null sequence. Because +/// SongMetadata IS eager-loaded on the playout paths, this was a LIVE 500 rather than a +/// latent one — and GetDisplayTitle feeds the playout guide, troubleshooting, media-item +/// info and channel states alike (issue #671). +/// +[TestFixture] +public class PlayoutMapperDisplayTitleTests +{ + [Test] + public void GetDisplayTitle_Should_Not_Throw_When_Song_Artists_Is_Null() + { + var song = new Song + { + Id = 1, + SongMetadata = [new SongMetadata { Title = "Untagged", Artists = null }] + }; + + string title = Mapper.GetDisplayTitle(song, Option.None); + + title.ShouldBe("Untagged"); + } + + [Test] + public void GetDisplayTitle_Should_Not_Prefix_When_Song_Has_No_Artists() + { + var song = new Song + { + Id = 1, + SongMetadata = [new SongMetadata { Title = "Untagged", Artists = [] }] + }; + + string title = Mapper.GetDisplayTitle(song, Option.None); + + title.ShouldBe("Untagged"); + } + + [Test] + public void GetDisplayTitle_Should_Prefix_The_Artists_When_Present() + { + var song = new Song + { + Id = 1, + SongMetadata = [new SongMetadata { Title = "Tagged", Artists = ["A", "B"] }] + }; + + string title = Mapper.GetDisplayTitle(song, Option.None); + + title.ShouldBe("A, B - Tagged"); + } +} diff --git a/ErsatzTV.Tests/Support/SelectionSeedData.cs b/ErsatzTV.Tests/Support/SelectionSeedData.cs index ddcbb7f79..5933a6cd7 100644 --- a/ErsatzTV.Tests/Support/SelectionSeedData.cs +++ b/ErsatzTV.Tests/Support/SelectionSeedData.cs @@ -19,6 +19,11 @@ internal static class SelectionSeedData /// /// Derived from production rather than hand-listed, so a newly-supported type joins the matrix /// automatically and trips the default: arms below until someone teaches them about it. + /// Note this is the RERUN-COLLECTION predicate, used for playlist items as a deliberate + /// SUPERSET: ReplacePlaylistItemsHandler.CollectionTypeMustBeValid has no + /// RemoteStream case, so a RemoteStream playlist item cannot be created through the write + /// API today and the playlist fixture seeds that row directly. Covering it is forward-looking, + /// not a claim that the two sets are equivalent — split this if they ever legitimately diverge. /// public static IEnumerable SupportedSelectionTypes => Enum.GetValues().Where(RerunCollectionRequestMapping.IsSupportedSelectionType); diff --git a/docs/decisions/records/api/selection-projection-include-chain.md b/docs/decisions/records/api/selection-projection-include-chain.md index 0318809db..e4fe99751 100644 --- a/docs/decisions/records/api/selection-projection-include-chain.md +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -7,74 +7,66 @@ supersedes: none superseded-by: none rule: 'Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET.' signals: 'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection · paths: `ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues: #671, #651, #229' -mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`); `RerunCollectionRequestMapping.IsSupportedSelectionType`' +mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `SelectionSeedData` (`SupportedSelectionTypes`, `ExpectedName`, `SeedSelection`, `ApplySelection`); `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`, `GetById_Should_Tolerate_Song_Artists`); `GetPlaylistItemsHandlerTests`; `RerunCollectionRequestMapping.IsSupportedSelectionType`' --- -Applies the `#229` shared-include-chain remedy to the READ path. That record framed the rule as a -write-path concern (project the mutation response through the include chain the GET uses); #671 is -its mirror image, where the GET itself under-loaded. +Applies the `#229` shared-include-chain remedy to the READ path — that record framed it as a +write-path concern (project the mutation response through the GET's chain); this is its mirror image, +where the GET itself under-loaded. ## The coupling that hid the bug -The id and the display name are read off the SAME eager-loaded navigation, so the id is only ever as -available as the name. There is no state where the API knows WHICH item is selected but not what it -is called. That is why the client-side symptom looked like a naming problem: an unlabelled badge. -The real harm is one level down — the selected id is null too, and an editor that round-trips it -silently clears the stored selection. #651 added a client-side merge-instead-of-replace guard that -made this survivable; that guard stays, but it was a client-side patch over a server-side defect. - -The corollary is the rule: never let the id and the name share a single point of failure. Hence the -`_ => null` ban — an unrecognized subtype surrenders its NAME, never its ID. - -## Why fail-soft rather than a throw - -The Done-when asked for the unhandled case to be handled or made loud. A throw is loud but fails an -entire paged GET over one bad row, on a read path. A conspicuous placeholder name is loud in the UI, -greppable in logs, and preserves the id — the thing that actually matters. Recorded because a future -session could reasonably reach for `NotSupportedException` here, as the sibling -`ProgramSchedules.Mapper` item-type switch does; that one dispatches on the ITEM type, an internal -closed set, where a throw is correct. +The id and the display name are read off the SAME navigation, so the id is only ever as available as +the name — the API never knows WHICH item is selected but not what it is called. Hence the symptom +looked like a naming problem (an unlabelled badge) when the real harm is one level down: the selected +id is null too, and an editor that round-trips it clears the stored selection. #651's client-side +merge-instead-of-replace guard made this survivable and stays, but it patched a server defect from +the client. The rule: never let the id and the name share a single point of failure — hence the +`_ => null` ban, where an unrecognized subtype surrenders its NAME, never its ID. Fail-soft, not a +throw, since a throw fails a whole paged GET over one bad row. (`ProgramSchedules.Mapper`'s switch +does throw, correctly — it dispatches on the ITEM type, an internal closed set.) ## Scope deliberately not widened -Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five in -`Scheduling.Mapper`) and all handle only Show/Season/Artist. That is NOT the same oversight: those -call sites genuinely restrict selection to those three types, and their handlers load a matching -chain. Only the RerunCollection and PlaylistItem switches span the full set, which is why exactly -those two were merged. `GetPlaylistItemsHandler` already loaded the other nine types, so PlaylistItem -was never broken in production; adding the shared `RemoteStream` arm did oblige it to gain the one -matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`. +Nine further media-item switches (`ProgramSchedules.Mapper` ×4, `Scheduling.Mapper` ×5) handle only +Show/Season/Artist. Not the same oversight: those call sites genuinely restrict selection to those +three and their handlers load a matching chain. Only the RerunCollection and PlaylistItem switches +span the full set, so exactly those two were merged. A THIRD consumer, `ReplacePlaylistItemsHandler`, +projects items whose navigations are never loaded — inert only because the controller discards the +result and re-queries; a dead projection, not a working one. -**Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not -free: each consumer must be re-checked against the navigations the new arm dereferences — and -discharged by a TEST, not by inspection, since inspection is the method that produced this bug. -`GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator), -so it gained the same 13-type matrix via the shared `SelectionSeedData`. +**Widening a shared switch incurs a debt in every caller loading for it**, discharged by a TEST, not +by inspection — inspection is the method that produced this bug. `GetPlaylistItemsHandler` had no +handler-level test at all (its controller tests stub the mediator), so it gained the same 13-type +matrix via the shared `SelectionSeedData`. -## Adding an include can CREATE a 500 +## `Artists` is a nullable PRIMITIVE COLLECTION, and the sweep must follow the field -Eager-loading is not automatically safe. `SongMetadata.Artists` is a nullable EF **primitive -collection** — a JSON array in one column, not a navigation — that `FallbackMetadataProvider` leaves -unassigned whenever a song's tags fail to read, and `string.Join` throws `ArgumentNullException` (not -`NullReferenceException`) on a null sequence. The rerun list previously did not load `SongMetadata` -at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would -have failed the entire page. The guard is `Optional(sm.Artists).Flatten()`, and the empty case is -filtered too so an artist-less song is not prefixed with a bare `" - "`. +`SongMetadata.Artists` is a nullable EF primitive collection — a JSON array in one column, **not a +navigation** — left unassigned by `FallbackMetadataProvider` when a song's tags fail to read, and +`string.Join` throws `ArgumentNullException`, not `NullReferenceException`, on a null sequence. So a +"null navigation" audit misses it and so does a grep for `NullReferenceException`. -The general rule: when you widen an include chain, audit what the newly-reachable projection -dereferences. A "null navigation" audit is not enough — nullable primitive collections throw a -different exception type and will not turn up in a grep for `NullReferenceException`. +Two corrections, because a wrong explanation outlives a wrong line. It was **not** introduced here: +`GetPlaylistItemsHandler` already included `SongMetadata` on `origin/main` and already routed `Song`, +so `GET /api/v1/playlists/{id}/items` was ALREADY a live 500 — this branch only made the same throw +reachable on a second path. And fixing the rerun site alone left the mirror standing: `Playouts/Mapper` +had the identical unguarded join on a path that also eager-loads `SongMetadata`, likewise live, swept +here. `LibraryBrowseItemMapper` already wrote `Artists ?? []`, so the codebase knew. + +Filed separately: `SongVideoGenerator` dereferences `Artists.Count`/`.Contains` unguarded on the +playback path. Sweep by FIELD, not by the one call site the issue named. ## Verification worth repeating -Each mechanism was removed in turn and the suite quoted red before the fix was restored: stripping -the list include chain failed all 13 types with "lost its selected id"; restoring the original -four-type by-id chain failed exactly the six the issue named; and reverting only the three bare -dereferences reproduced the `NullReferenceException` for Episode and MusicVideo. A green new test -over a read path proves little until the mechanism it covers has been shown to fail without it. +Every mechanism was removed in turn and the suite quoted red before restoring it: stripping the list +include chain failed all 13 types on "lost its selected id"; the original four-type by-id chain failed +exactly the six the issue named; reverting the bare dereferences reproduced the `NullReferenceException` +for Episode and MusicVideo; and reverting either `Artists` guard reproduced `ArgumentNullException`. +A green new test over a read path proves little until it has been shown to fail without its mechanism. -The per-type name assertion pins the WHOLE expected string, not merely "is not a placeholder". A -cross-family review caught that the looser form could not see a missing NESTED leg: drop -Episode → Season → Show and the projection still reads `s00e04 - Selected episode`, which contains -no placeholder marker and would have passed. Related: an absent Season renders `s??`, never `s00`, -since `s00` conventionally means Specials and would fabricate plausible-looking real data. +The per-type assertion pins the WHOLE expected string, not merely "is not a placeholder", because a +review caught that the looser form cannot see a missing NESTED leg: drop Episode → Season → Show and +the projection still reads `s00e04 - Selected episode`, placeholder-free, and passes. Relatedly an +absent Season renders `s??`, never `s00`, which conventionally means Specials and would fabricate +plausible-looking real data. -- 2.47.3 From 2249a806c964380ef249a8350cb58aa165645b0f Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 20:27:08 +0200 Subject: [PATCH 5/5] fix(671): review round 4 -- fix the chapter-title entity interpolation Final cold review of 34eee753b: the commit's own changes were confirmed correct, but it flagged a real pre-existing bug in the exact block I had just edited, and I was adding the first-ever tests for that method without covering it. `Playouts/Mapper.GetDisplayTitle`'s Song arm interpolated `{s}` -- the `case Song s` ENTITY -- into its chapter-title branch instead of `{t}`, the composed title. Song has no ToString() override, so a chaptered song rendered as the literal "ErsatzTV.Core.Domain.Song (Chapter 3)" in the playout guide, troubleshooting, media-item info and channel states. The sibling MusicVideo and OtherVideo arms are correct only because they happen to name their lambda `s`. Pre-existing on main; fixed here because it is one token inside the block this branch already touches. Two tests pin it; reverting renders the type name. Also: completed the guard on that arm (`Optional(s.SongMetadata).Flatten()`, the other half of the sibling pattern I claimed to have copied), added the new mechanism to the record's `mechanics:`, added the symptom tokens a future session would actually search for (ArgumentNullException, Artists, primitive collection, chaptered song) to `signals:`, restored the remedy sentence an earlier trim dropped, and trimmed to 59 prose lines for margin under the 60-line ceiling. Co-Authored-By: Claude Opus 5 (1M context) --- ErsatzTV.Application/Playouts/Mapper.cs | 10 +++- .../PlayoutMapperDisplayTitleTests.cs | 35 ++++++++++++ .../api/selection-projection-include-chain.md | 55 +++++++++---------- 3 files changed, 69 insertions(+), 31 deletions(-) diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index 276eb5a94..c869aaddb 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -106,16 +106,20 @@ internal static class Mapper // assigns it for a song whose tags failed to read) and string.Join throws // ArgumentNullException on a null sequence. SongMetadata IS eager-loaded on this path, so // this was a LIVE 500 on the playout guide, not a latent one (issue #671). - string songArtist = s.SongMetadata.HeadOrNone() + string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone() .Map(sm => Optional(sm.Artists).Flatten().ToList()) .Filter(artists => artists.Count > 0) .Map(artists => $"{string.Join(", ", artists)} - ") .IfNone(string.Empty); - return s.SongMetadata.HeadOrNone() + return Optional(s.SongMetadata).Flatten().HeadOrNone() .Map(sm => $"{songArtist}{sm.Title ?? string.Empty}") .Map(t => string.IsNullOrWhiteSpace(chapterTitle) + // interpolate the composed title `t`, NOT the `case Song s` entity — Song has no + // ToString() override, so `{s}` rendered a chaptered song as the literal type name + // "ErsatzTV.Core.Domain.Song (Chapter 3)". The MusicVideo/OtherVideo arms above are + // correct only because they happen to name their lambda parameter `s`. ? t - : $"{s} ({chapterTitle})") + : $"{t} ({chapterTitle})") .IfNone("[unknown song]"); case Image i: return i.ImageMetadata.HeadOrNone().Map(im => im.Title ?? string.Empty).IfNone("[unknown image]"); diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs index 669d17f08..dc13df445 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs @@ -58,4 +58,39 @@ public class PlayoutMapperDisplayTitleTests title.ShouldBe("A, B - Tagged"); } + + /// + /// The chapter branch interpolated the `case Song s` ENTITY rather than the composed title, and + /// has no ToString() override — so a chaptered song rendered as the + /// literal "ErsatzTV.Core.Domain.Song (Chapter 1)". Pre-existing; the sibling MusicVideo and + /// OtherVideo arms are correct only because they name their lambda parameter `s` too. + /// + [Test] + public void GetDisplayTitle_Should_Compose_The_Title_Not_The_Entity_When_Chaptered() + { + var song = new Song + { + Id = 1, + SongMetadata = [new SongMetadata { Title = "Tagged", Artists = ["A"] }] + }; + + string title = Mapper.GetDisplayTitle(song, Option.Some("Chapter 1")); + + title.ShouldBe("A - Tagged (Chapter 1)"); + title.ShouldNotContain("ErsatzTV.Core.Domain"); + } + + [Test] + public void GetDisplayTitle_Should_Not_Throw_When_Chaptered_Song_Has_Null_Artists() + { + var song = new Song + { + Id = 1, + SongMetadata = [new SongMetadata { Title = "Untagged", Artists = null }] + }; + + string title = Mapper.GetDisplayTitle(song, Option.Some("Chapter 2")); + + title.ShouldBe("Untagged (Chapter 2)"); + } } diff --git a/docs/decisions/records/api/selection-projection-include-chain.md b/docs/decisions/records/api/selection-projection-include-chain.md index e4fe99751..28e983102 100644 --- a/docs/decisions/records/api/selection-projection-include-chain.md +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -6,13 +6,12 @@ since: '2026-07-28' supersedes: none superseded-by: none rule: 'Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET.' -signals: 'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection · paths: `ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues: #671, #651, #229' -mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `SelectionSeedData` (`SupportedSelectionTypes`, `ExpectedName`, `SeedSelection`, `ApplySelection`); `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`, `GetById_Should_Tolerate_Song_Artists`); `GetPlaylistItemsHandlerTests`; `RerunCollectionRequestMapping.IsSupportedSelectionType`' +signals: 'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection, ArgumentNullException value cannot be null parameter values, string.Join on null sequence, SongMetadata Artists is null, nullable primitive collection not a navigation, untagged song fallback metadata, playout guide 500 on a song, song artist prefix bare dash, chaptered song renders ErsatzTV.Core.Domain.Song, GetDisplayTitle interpolates the entity not the title · paths: `ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues: #671, #651, #229' +mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `SelectionSeedData` (`SupportedSelectionTypes`, `ExpectedName`, `SeedSelection`, `ApplySelection`); `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`, `GetById_Should_Tolerate_Song_Artists`); `GetPlaylistItemsHandlerTests`; `Playouts.Mapper.GetDisplayTitle` + `PlayoutMapperDisplayTitleTests`; `RerunCollectionRequestMapping.IsSupportedSelectionType`' --- Applies the `#229` shared-include-chain remedy to the READ path — that record framed it as a -write-path concern (project the mutation response through the GET's chain); this is its mirror image, -where the GET itself under-loaded. +write-path concern; this is its mirror image, where the GET itself under-loaded. ## The coupling that hid the bug @@ -20,20 +19,19 @@ The id and the display name are read off the SAME navigation, so the id is only the name — the API never knows WHICH item is selected but not what it is called. Hence the symptom looked like a naming problem (an unlabelled badge) when the real harm is one level down: the selected id is null too, and an editor that round-trips it clears the stored selection. #651's client-side -merge-instead-of-replace guard made this survivable and stays, but it patched a server defect from -the client. The rule: never let the id and the name share a single point of failure — hence the +merge-instead-of-replace guard made this survivable and stays, but patched a server defect from the +client. The rule: never let the id and the name share a single point of failure — hence the `_ => null` ban, where an unrecognized subtype surrenders its NAME, never its ID. Fail-soft, not a -throw, since a throw fails a whole paged GET over one bad row. (`ProgramSchedules.Mapper`'s switch -does throw, correctly — it dispatches on the ITEM type, an internal closed set.) +throw, which would fail a whole paged GET over one bad row. (`ProgramSchedules.Mapper`'s switch does +throw, correctly — it dispatches on the ITEM type, an internal closed set.) ## Scope deliberately not widened Nine further media-item switches (`ProgramSchedules.Mapper` ×4, `Scheduling.Mapper` ×5) handle only -Show/Season/Artist. Not the same oversight: those call sites genuinely restrict selection to those -three and their handlers load a matching chain. Only the RerunCollection and PlaylistItem switches -span the full set, so exactly those two were merged. A THIRD consumer, `ReplacePlaylistItemsHandler`, -projects items whose navigations are never loaded — inert only because the controller discards the -result and re-queries; a dead projection, not a working one. +Show/Season/Artist — not the same oversight, since those call sites genuinely restrict selection to +those three and load a matching chain. Only RerunCollection and PlaylistItem span the full set, so +exactly those two were merged. A THIRD consumer, `ReplacePlaylistItemsHandler`, projects items whose +navigations are never loaded — inert only because the controller discards the result and re-queries. **Widening a shared switch incurs a debt in every caller loading for it**, discharged by a TEST, not by inspection — inspection is the method that produced this bug. `GetPlaylistItemsHandler` had no @@ -44,29 +42,30 @@ matrix via the shared `SelectionSeedData`. `SongMetadata.Artists` is a nullable EF primitive collection — a JSON array in one column, **not a navigation** — left unassigned by `FallbackMetadataProvider` when a song's tags fail to read, and -`string.Join` throws `ArgumentNullException`, not `NullReferenceException`, on a null sequence. So a -"null navigation" audit misses it and so does a grep for `NullReferenceException`. +`string.Join` throws `ArgumentNullException`, not `NullReferenceException`. So a "null navigation" +audit misses it and so does a grep for `NullReferenceException`. The guard is +`Optional(sm.Artists).Flatten()`, empty filtered too so an artist-less song loses its bare `" - "`. Two corrections, because a wrong explanation outlives a wrong line. It was **not** introduced here: `GetPlaylistItemsHandler` already included `SongMetadata` on `origin/main` and already routed `Song`, so `GET /api/v1/playlists/{id}/items` was ALREADY a live 500 — this branch only made the same throw reachable on a second path. And fixing the rerun site alone left the mirror standing: `Playouts/Mapper` had the identical unguarded join on a path that also eager-loads `SongMetadata`, likewise live, swept -here. `LibraryBrowseItemMapper` already wrote `Artists ?? []`, so the codebase knew. +here. `LibraryBrowseItemMapper` already wrote `Artists ?? []`, so the codebase knew. Filed separately: +`SongVideoGenerator` dereferences `Artists.Count`/`.Contains` on the playback path. Sweep by FIELD. -Filed separately: `SongVideoGenerator` dereferences `Artists.Count`/`.Contains` unguarded on the -playback path. Sweep by FIELD, not by the one call site the issue named. +Adjacent, same review, fixed here: that Song arm interpolated the `case Song s` ENTITY into its +chapter branch, rendering a chaptered song as the literal `ErsatzTV.Core.Domain.Song (Chapter 3)`. ## Verification worth repeating -Every mechanism was removed in turn and the suite quoted red before restoring it: stripping the list -include chain failed all 13 types on "lost its selected id"; the original four-type by-id chain failed -exactly the six the issue named; reverting the bare dereferences reproduced the `NullReferenceException` -for Episode and MusicVideo; and reverting either `Artists` guard reproduced `ArgumentNullException`. -A green new test over a read path proves little until it has been shown to fail without its mechanism. +Every mechanism was removed in turn and quoted red before restoring it: stripping the list include +chain failed all 13 types on "lost its selected id"; the original four-type by-id chain failed exactly +the six the issue named; reverting the bare dereferences reproduced `NullReferenceException` for +Episode and MusicVideo; reverting either `Artists` guard reproduced `ArgumentNullException`; and +reverting the chapter fix rendered the type name. A green test proves little until shown to fail. -The per-type assertion pins the WHOLE expected string, not merely "is not a placeholder", because a -review caught that the looser form cannot see a missing NESTED leg: drop Episode → Season → Show and -the projection still reads `s00e04 - Selected episode`, placeholder-free, and passes. Relatedly an -absent Season renders `s??`, never `s00`, which conventionally means Specials and would fabricate -plausible-looking real data. +The per-type assertion pins the WHOLE expected string, not merely "is not a placeholder", because the +looser form cannot see a missing NESTED leg: drop Episode → Season → Show and the projection still +reads `s00e04 - Selected episode`, placeholder-free, and passes. Relatedly an absent Season renders +`s??`, never `s00`, which means Specials and would fabricate plausible-looking real data. -- 2.47.3