fix(691): guard the nullable SongMetadata.Artists/AlbumArtists at their read sites #700

Merged

timothy merged 2 commits from fix/691-song-artists-null-guard into main 2026-07-28 23:43:30 +02:00

Conversation 1 Commits 2 Files Changed 4

+142 -10

timothy commented 2026-07-28 22:42:53 +02:00

Owner

Copy Link

Copy Source

fixes #691

SongMetadata.Artists / .AlbumArtists are nullable EF primitive collections that FallbackMetadataProvider never assigns, so untagged songs persist them as NULL. SongVideoGenerator dereferenced them unguarded — a throw on the playback path.

Two commits on purpose

The first commit made the entity coalesce on read (get => _artists ??= [];). Adversarial review rejected that design, empirically. It probed the real TvContext on SQLite: with EF's default PreferField access mode, a single read of .Artists on a tracked entity mutates the backing field, flips the entity to Modified, and the next SaveChanges writes [] over the NULL column — the opposite of what that commit's own comment claimed. No live corruption today only because every current reader happens to be AsNoTracking; it was a loaded gun aimed at the next author.

It also silently reversed an active decision record — api.selection-projection-include-chain (#671) prescribes Optional(sm.Artists).Flatten() and says "Sweep by FIELD". #691 is that filed follow-up.

The second commit reverts the entity to plain auto-properties and follows the recorded convention instead.

What landed

• SongVideoGenerator.cs — Optional(...).Flatten().ToList() hoisted once into artists / albumArtists locals, used at all six former sites including the Contains at the album-artist filter.
• MediaCollectionRepository.cs — both GroupIntoFakeCollections sites (AlbumArtists.HeadOrNone(), SelectMany(sm => sm.AlbumArtists)). The issue never named these; the field sweep found them. Reverting the entity fix without them would have traded one bug for two.
• The six existing guards elsewhere (MediaItems/Mapper, Playouts/Mapper, LibraryBrowseItemMapper, Lucene/Elastic index) are correct and load-bearing under this design and are left alone.

Verification

Both guards proven independently load-bearing by mutating only their own clause:

• artists local unguarded → ArgumentNullException at SongVideoGenerator.cs:88
• album-artists local unguarded → ArgumentNullException at SongVideoGenerator.cs:89

(ArgumentNullException from Enumerable.ToList's null-source check rather than NullReferenceException — the throw site moved with the hoist; same defect.)

Green: ErsatzTV.Core.Tests 685 passed / 1 skipped; ErsatzTV.Tests 1996 passed / 4 skipped. has-pending-model-changes → no changes (no migration needed; the column stays nullable deliberately, so existing NULL rows are untouched and no dual-provider migration is required).

Deliberately not done

Making the column non-nullable. It needs dual-provider migrations plus a prod backfill for a problem the read-site guards already close, and the review agreed. Not filed as follow-up work; noted here so the choice is explicit rather than implicit.

fixes #691 `SongMetadata.Artists` / `.AlbumArtists` are nullable EF primitive collections that `FallbackMetadataProvider` never assigns, so untagged songs persist them as NULL. `SongVideoGenerator` dereferenced them unguarded — a throw on the playback path. ## Two commits on purpose The first commit made the entity coalesce on read (`get => _artists ??= [];`). **Adversarial review rejected that design, empirically.** It probed the real `TvContext` on SQLite: with EF's default `PreferField` access mode, a single read of `.Artists` on a *tracked* entity mutates the backing field, flips the entity to `Modified`, and the next `SaveChanges` writes `[]` over the NULL column — the opposite of what that commit's own comment claimed. No live corruption today only because every current reader happens to be `AsNoTracking`; it was a loaded gun aimed at the next author. It also silently reversed an active decision record — `api.selection-projection-include-chain` (#671) prescribes `Optional(sm.Artists).Flatten()` and says *"Sweep by FIELD"*. #691 **is** that filed follow-up. The second commit reverts the entity to plain auto-properties and follows the recorded convention instead. ## What landed - `SongVideoGenerator.cs` — `Optional(...).Flatten().ToList()` hoisted **once** into `artists` / `albumArtists` locals, used at all six former sites including the `Contains` at the album-artist filter. - `MediaCollectionRepository.cs` — both `GroupIntoFakeCollections` sites (`AlbumArtists.HeadOrNone()`, `SelectMany(sm => sm.AlbumArtists)`). **The issue never named these**; the field sweep found them. Reverting the entity fix without them would have traded one bug for two. - The six existing guards elsewhere (`MediaItems/Mapper`, `Playouts/Mapper`, `LibraryBrowseItemMapper`, `Lucene`/`Elastic` index) are correct and load-bearing under this design and are left alone. ## Verification Both guards proven independently load-bearing by mutating **only** their own clause: - artists local unguarded → `ArgumentNullException` at `SongVideoGenerator.cs:88` - album-artists local unguarded → `ArgumentNullException` at `SongVideoGenerator.cs:89` (`ArgumentNullException` from `Enumerable.ToList`'s null-source check rather than `NullReferenceException` — the throw site moved with the hoist; same defect.) Green: `ErsatzTV.Core.Tests` 685 passed / 1 skipped; `ErsatzTV.Tests` 1996 passed / 4 skipped. `has-pending-model-changes` → no changes (no migration needed; the column stays nullable deliberately, so existing NULL rows are untouched and no dual-provider migration is required). ## Deliberately not done Making the column non-nullable. It needs dual-provider migrations plus a prod backfill for a problem the read-site guards already close, and the review agreed. Not filed as follow-up work; noted here so the choice is explicit rather than implicit.

timothy added 2 commits 2026-07-28 22:42:55 +02:00

fix(691): guard SongMetadata.Artists/AlbumArtists at the domain boundary ... a4700185b2

SongMetadata.Artists and .AlbumArtists are nullable EF primitive
collections that FallbackMetadataProvider.GetSongMetadata never
assigns, so untagged songs persist them as NULL. SongVideoGenerator
dereferenced both unguarded (metadata.Artists.Count, string.Join,
AlbumArtists.Filter(...Artists.Contains...)), throwing NRE/ANE during
song-video generation on the playback path.

Rather than enumerating and guarding each read site (the same mistake
that left these unswept after #671), add backing fields to the two
properties whose getters coalesce null to an empty list. EF Core's
default PreferField access mode reads/writes the raw backing field
during materialization and change-tracking (confirmed by running the
full ErsatzTV.Tests suite, including SongMetadata round-trip tests,
unchanged), while every other caller -- SongVideoGenerator,
MediaCollectionRepository's rerun-collection artist grouping, and any
future reader -- goes through the property getter and always sees a
non-null list. This subsumes the ad hoc `metadata.Artists ??= []`
guards already hand-applied in LuceneSearchIndex/ElasticSearchIndex
and the `?? []` in LibraryBrowseItemMapper, which remain but are now
redundant.

Adds SongVideoGeneratorTests covering an untagged song (null Artists/
AlbumArtists) through GenerateSongVideo; verified RED (NRE at
SongMetadata.cs's Artists getter) by reverting only the `??= []`
clause, not the file.

Strips the pre-existing UTF-8 BOM from SongMetadata.cs per the #311
formatting gate (touching a legacy-BOM file makes stripping it ours
to do).

Refs #691

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(691): revert entity-level null guard, guard read sites instead ... dd7b58232c

The prior commit (a4700185b) made SongMetadata.Artists/AlbumArtists
coalesce null to [] via backing-field getters, reasoning that EF
Core's PreferField access mode never observes the getter. Adversarial
review disproved this on real TvContext/SQLite: a single read of
.Artists on a TRACKED entity mutates the backing field through the
getter, flips the entity to Modified, and the next SaveChanges writes
[] over what was a NULL column -- silent data loss waiting on the
first tracked reader (today all readers happen to be AsNoTracking).

This also reversed docs/decisions/records/api/selection-projection-include-chain.md
(#671) without the doc update CLAUDE.md requires; #691 is that
record's own "sweep by FIELD" follow-up, so it should follow the
record, not contradict it.

Revert SongMetadata.cs to plain auto-properties (byte-identical to
origin/main, BOM still stripped per the #311 gate). Guard the read
sites instead, per the #671 convention (Optional(...).Flatten(),
matching Playouts/Mapper.cs and MediaItems/Mapper.cs):

- SongVideoGenerator.cs: hoist `artists`/`albumArtists` locals once
  near the top of the metadata loop instead of repeating the guard at
  each of the six former call sites.
- MediaCollectionRepository.cs (GroupIntoFakeCollections): guard the
  two AlbumArtists reads at lines ~1147/~1160 that #691 never named --
  dropping the entity-level fix without these would trade one bug for
  two.

Verified RED per guard by removing only the Optional(...).Flatten()
clause (not the whole file): the artists local throws
ArgumentNullException at SongVideoGenerator.cs:88, the albumArtists
local at :89 (List.ToList() on a null IList<string> source -- same
loaded-gun shape the review demonstrated, precise exception type is
ArgumentNullException rather than NullReferenceException since the
throw site is Enumerable.ToList's null-source check). Restored both;
existing SongVideoGeneratorTests still pass. Full ErsatzTV.Core.Tests:
685 passed (1 pre-existing skip), ErsatzTV.Tests: 1996 passed (4
pre-existing skips), 0 failures in each. No EF model drift
(`dotnet ef migrations has-pending-model-changes` reports none).
`dotnet format --verify-no-changes` on the three touched files exits
0.

Refs #691

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy commented 2026-07-28 23:05:43 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ dd7b582

Independent cold re-review of the fix commit dd7b58232 (worktree-isolated, no implementation role): all 8 verification items PASS, both guards proven load-bearing by single-clause mutation (ArgumentNullException at SongVideoGenerator.cs:88 and :89), no EF model drift, 685 + 1996 tests green. Two INFO findings are pre-existing and filed separately. Author-orchestrator posting on own PR; the review itself was performed by an independent agent.

Review-verdict: MERGEABLE @ dd7b582 Independent cold re-review of the fix commit dd7b58232 (worktree-isolated, no implementation role): all 8 verification items PASS, both guards proven load-bearing by single-clause mutation (ArgumentNullException at SongVideoGenerator.cs:88 and :89), no EF model drift, 685 + 1996 tests green. Two INFO findings are pre-existing and filed separately. Author-orchestrator posting on own PR; the review itself was performed by an independent agent.

timothy referenced this pull request 2026-07-28 23:06:05 +02:00

Search indexers mutate `SongMetadata.Artists` on a possibly-tracked entity (`??= []`) — same shape rejected in #691 #701

timothy referenced this pull request 2026-07-28 23:14:19 +02:00

`SongVideoGenerator` dereferences the nullable `SongMetadata.Artists` unguarded — NRE on the playback path #691

timothy merged commit 4e2ea61674 into main 2026-07-28 23:43:30 +02:00

timothy referenced this issue from a commit 2026-07-28 23:43:30 +02:00

Merge pull request 'fix(691): guard the nullable SongMetadata.Artists/AlbumArtists at their read sites' (#700) from fix/691-song-artists-null-guard into main

timothy deleted branch fix/691-song-artists-null-guard 2026-07-28 23:43:31 +02:00

timothy referenced this pull request 2026-07-28 23:43:56 +02:00

`SongVideoGenerator` dereferences the nullable `SongMetadata.Artists` unguarded — NRE on the playback path #691

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#700