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.