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

2 Commits

Author	SHA1	Message	Date
timothyandClaude Opus 5	dd7b58232c	fix(691): revert entity-level null guard, guard read sites instead ... 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>	2026-07-28 22:39:52 +02:00
timothyandClaude Opus 5	a4700185b2	fix(691): guard SongMetadata.Artists/AlbumArtists at the domain boundary ... 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>	2026-07-28 21:30:06 +02:00