fix(691): revert entity-level null guard, guard read sites instead
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 16s
PR Gates / Docs update reminder (pull_request) Successful in 59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m54s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m4s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 11s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
review-verdict/h10 Review-verdict: MERGEABLE @ dd7b582 (base: main)
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 22m12s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Review verdict / Set review-verdict status (pull_request) Successful in 5s
PR Gates / decisions lifecycle (pull_request) Successful in 13s
PR Gates / Script tests (pytest) (pull_request) Successful in 46s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 16s
PR Gates / Docs update reminder (pull_request) Successful in 59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m54s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m4s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 11s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
review-verdict/h10 Review-verdict: MERGEABLE @ dd7b582 (base: main)
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 22m12s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Review verdict / Set review-verdict status (pull_request) Successful in 5s
PR Gates / decisions lifecycle (pull_request) Successful in 13s
PR Gates / Script tests (pytest) (pull_request) Successful in 46s
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>
This commit is contained in:
@@ -2,30 +2,9 @@ namespace ErsatzTV.Core.Domain;
|
||||
|
||||
public class SongMetadata : Metadata
|
||||
{
|
||||
// backing fields so EF Core's default PreferField access mode reads/writes the raw
|
||||
// (possibly-null) value during materialization/change-tracking, while every other
|
||||
// caller goes through the property getter and always sees a non-null list. This is
|
||||
// the single guarded boundary for the nullable `Artists`/`AlbumArtists` primitive
|
||||
// collections (ersatztv#691) -- untagged songs persist these columns as NULL
|
||||
// (`FallbackMetadataProvider.GetSongMetadata` never assigns them), so every reader
|
||||
// needs the same `?? []` guard `LibraryBrowseItemMapper` already applied by hand.
|
||||
private IList<string> _artists;
|
||||
private IList<string> _albumArtists;
|
||||
|
||||
public string Album { get; set; }
|
||||
|
||||
public IList<string> Artists
|
||||
{
|
||||
get => _artists ??= [];
|
||||
set => _artists = value;
|
||||
}
|
||||
|
||||
public IList<string> AlbumArtists
|
||||
{
|
||||
get => _albumArtists ??= [];
|
||||
set => _albumArtists = value;
|
||||
}
|
||||
|
||||
public IList<string> Artists { get; set; }
|
||||
public IList<string> AlbumArtists { get; set; }
|
||||
public string Track { get; set; }
|
||||
public string Comment { get; set; }
|
||||
public int SongId { get; set; }
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
using System.Globalization;
|
||||
using System.Globalization;
|
||||
using System.Text;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.FFmpeg;
|
||||
@@ -85,6 +85,9 @@ public class SongVideoGenerator : ISongVideoGenerator
|
||||
|
||||
var sb = new StringBuilder();
|
||||
|
||||
List<string> artists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> albumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
if (detailsStyle)
|
||||
{
|
||||
if (!string.IsNullOrWhiteSpace(metadata.Title))
|
||||
@@ -92,17 +95,17 @@ public class SongVideoGenerator : ISongVideoGenerator
|
||||
sb.Append(CultureInfo.InvariantCulture, $"{{\\fs{largeFontSize}}}{metadata.Title}");
|
||||
}
|
||||
|
||||
if (metadata.Artists.Count > 0)
|
||||
if (artists.Count > 0)
|
||||
{
|
||||
var allArtists = string.Join(", ", metadata.Artists);
|
||||
var allArtists = string.Join(", ", artists);
|
||||
sb.Append(CultureInfo.InvariantCulture, $"\\N{{\\fs{fontSize}}}{allArtists}");
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
if (metadata.Artists.Count > 0)
|
||||
if (artists.Count > 0)
|
||||
{
|
||||
var allArtists = string.Join(", ", metadata.Artists);
|
||||
var allArtists = string.Join(", ", artists);
|
||||
sb.Append(allArtists);
|
||||
}
|
||||
|
||||
@@ -111,11 +114,11 @@ public class SongVideoGenerator : ISongVideoGenerator
|
||||
sb.Append(CultureInfo.InvariantCulture, $"\\N\"{metadata.Title}\"");
|
||||
}
|
||||
|
||||
if (metadata.AlbumArtists.Count > 0)
|
||||
if (albumArtists.Count > 0)
|
||||
{
|
||||
var allAlbumArtists = string.Join(
|
||||
", ",
|
||||
metadata.AlbumArtists.Filter(aa => !metadata.Artists.Contains(aa)));
|
||||
albumArtists.Filter(aa => !artists.Contains(aa)));
|
||||
sb.Append(CultureInfo.InvariantCulture, $"\\N{allAlbumArtists}");
|
||||
}
|
||||
|
||||
|
||||
@@ -1144,7 +1144,7 @@ public class MediaCollectionRepository : IMediaCollectionRepository
|
||||
|
||||
var allArtists = items.OfType<Song>()
|
||||
.SelectMany(s => s.SongMetadata)
|
||||
.Map(sm => sm.AlbumArtists.HeadOrNone().Match(aa => aa, string.Empty))
|
||||
.Map(sm => Optional(sm.AlbumArtists).Flatten().HeadOrNone().Match(aa => aa, string.Empty))
|
||||
.Distinct()
|
||||
.ToList();
|
||||
|
||||
@@ -1157,7 +1157,7 @@ public class MediaCollectionRepository : IMediaCollectionRepository
|
||||
foreach (Song song in items.OfType<Song>())
|
||||
{
|
||||
string firstArtist = song.SongMetadata
|
||||
.SelectMany(sm => sm.AlbumArtists)
|
||||
.SelectMany(sm => Optional(sm.AlbumArtists).Flatten())
|
||||
.HeadOrNone()
|
||||
.Match(aa => aa, string.Empty);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user