fix(671): review round 4 -- fix the chapter-title entity interpolation
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 22s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 29s
PR Gates / Docs update reminder (pull_request) Successful in 41s
PR Gates / decisions lifecycle (pull_request) Successful in 41s
Review verdict / Set review-verdict status (pull_request) Successful in 15s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m17s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 20m1s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 22m49s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 25m35s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ 2249a80 (base: main)
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 22s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 29s
PR Gates / Docs update reminder (pull_request) Successful in 41s
PR Gates / decisions lifecycle (pull_request) Successful in 41s
Review verdict / Set review-verdict status (pull_request) Successful in 15s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m17s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m11s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 20m1s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 22m49s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 25m35s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ 2249a80 (base: main)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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]");
|
||||
|
||||
@@ -58,4 +58,39 @@ public class PlayoutMapperDisplayTitleTests
|
||||
|
||||
title.ShouldBe("A, B - Tagged");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The chapter branch interpolated the `case Song s` ENTITY rather than the composed title, and
|
||||
/// <see cref="Song" /> has no <c>ToString()</c> 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.
|
||||
/// </summary>
|
||||
[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<string>.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<string>.Some("Chapter 2"));
|
||||
|
||||
title.ShouldBe("Untagged (Chapter 2)");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 `<Aggregate>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.
|
||||
|
||||
Reference in New Issue
Block a user