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/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/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..da03c501a --- /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) + // 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) + .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..e04c56608 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,34 @@ 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}"; + // "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) { - 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() - .Map(sm => $"{string.Join(", ", sm.Artists)} - ") + // 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 => 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}") .IfNone("[unknown song]"); } diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index 2b9ef0969..c869aaddb 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -102,14 +102,24 @@ internal static class Mapper : $"{s} ({chapterTitle})") .IfNone("[unknown video]"); case Song s: - string songArtist = s.SongMetadata.HeadOrNone() - .Map(sm => $"{string.Join(", ", sm.Artists)} - ") + // 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 = 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/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 new file mode 100644 index 000000000..612864b42 --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs @@ -0,0 +1,166 @@ +using ErsatzTV.Application.MediaCollections; +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 is derived +/// from the production predicate (see ) rather than hand-listed. +/// +[TestFixture] +public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase +{ + 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. Set equality, so it fails on widening too. + /// + [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); + } + + /// + /// 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. + /// + [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()) + { + 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 + /// 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(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 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 + }; + + 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/Application/Playouts/PlayoutMapperDisplayTitleTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs new file mode 100644 index 000000000..dc13df445 --- /dev/null +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutMapperDisplayTitleTests.cs @@ -0,0 +1,96 @@ +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"); + } + + /// + /// 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/ErsatzTV.Tests/Support/SelectionSeedData.cs b/ErsatzTV.Tests/Support/SelectionSeedData.cs new file mode 100644 index 000000000..5933a6cd7 --- /dev/null +++ b/ErsatzTV.Tests/Support/SelectionSeedData.cs @@ -0,0 +1,212 @@ +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. + /// 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); + + /// + /// 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 f60514eb0..a02be0b11 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -146,6 +146,41 @@ 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. + +**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/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..28e983102 --- /dev/null +++ b/docs/decisions/records/api/selection-projection-include-chain.md @@ -0,0 +1,71 @@ +--- +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, 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; 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 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 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, 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, 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 +handler-level test at all (its controller tests stub the mediator), so it gained the same 13-type +matrix via the shared `SelectionSeedData`. + +## `Artists` is a nullable PRIMITIVE COLLECTION, and the sweep must follow the field + +`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`. 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. Filed separately: +`SongVideoGenerator` dereferences `Artists.Count`/`.Contains` on the playback path. Sweep by FIELD. + +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 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 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.