From a4700185b26e2edc22f1f71dc138f9e5dabce798 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 21:30:06 +0200 Subject: [PATCH 1/2] 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) --- .../FFmpeg/SongVideoGeneratorTests.cs | 129 ++++++++++++++++++ ErsatzTV.Core/Domain/Metadata/SongMetadata.cs | 27 +++- 2 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 ErsatzTV.Core.Tests/FFmpeg/SongVideoGeneratorTests.cs diff --git a/ErsatzTV.Core.Tests/FFmpeg/SongVideoGeneratorTests.cs b/ErsatzTV.Core.Tests/FFmpeg/SongVideoGeneratorTests.cs new file mode 100644 index 000000000..e0c10c2c3 --- /dev/null +++ b/ErsatzTV.Core.Tests/FFmpeg/SongVideoGeneratorTests.cs @@ -0,0 +1,129 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.FFmpeg; +using ErsatzTV.Core.Interfaces.FFmpeg; +using ErsatzTV.Core.Interfaces.Images; +using ErsatzTV.Core.Interfaces.Metadata; +using ErsatzTV.FFmpeg.State; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Core.Tests.FFmpeg; + +[TestFixture] +public class SongVideoGeneratorTests +{ + private ITempFilePool _tempFilePool; + private IImageCache _imageCache; + private IFFmpegProcessService _ffmpegProcessService; + private ILocalFileSystem _localFileSystem; + private SongVideoGenerator _songVideoGenerator; + private string _tempSubtitleFile; + + [SetUp] + public void SetUp() + { + _tempSubtitleFile = Path.Combine(Path.GetTempPath(), $"{Guid.NewGuid()}.ass"); + + _tempFilePool = Substitute.For(); + _tempFilePool.GetNextTempFile(Arg.Any()).Returns(_tempSubtitleFile); + + _imageCache = Substitute.For(); + _imageCache.GetPathForImage(Arg.Any(), Arg.Any(), Arg.Any>()) + .Returns("/fake/watermark.png"); + + _ffmpegProcessService = Substitute.For(); + _ffmpegProcessService.GenerateSongImage( + Arg.Any(), + Arg.Any(), + Arg.Any>(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(Either.Right("/fake/song-image.png")); + + _localFileSystem = Substitute.For(); + _localFileSystem.GetCustomOrDefaultFile(Arg.Any(), Arg.Any()) + .Returns("/fake/background.png"); + + _songVideoGenerator = new SongVideoGenerator( + _tempFilePool, + _imageCache, + _ffmpegProcessService, + _localFileSystem); + } + + [TearDown] + public void TearDown() + { + if (_tempSubtitleFile is not null && File.Exists(_tempSubtitleFile)) + { + File.Delete(_tempSubtitleFile); + } + } + + private static Channel BuildChannel() + { + var resolution = new Resolution { Width = 1920, Height = 1080 }; + FFmpegProfile ffmpegProfile = FFmpegProfile.New("test", resolution); + + return new Channel(Guid.NewGuid()) + { + Number = "1", + Name = "Test Channel", + FFmpegProfile = ffmpegProfile, + SongVideoMode = ChannelSongVideoMode.Default + }; + } + + private static Song BuildUntaggedSong() + { + // an untagged song: FallbackMetadataProvider.GetSongMetadata never assigns + // Artists/AlbumArtists, so they persist (and materialize) as null (ersatztv#691) + var metadata = new SongMetadata + { + MetadataKind = MetadataKind.Fallback, + Title = "Untagged Song", + Artwork = [], + Artists = null, + AlbumArtists = null + }; + + return new Song + { + SongMetadata = [metadata], + MediaVersions = [] + }; + } + + [Test] + public async Task GenerateSongVideo_should_not_throw_when_artists_and_album_artists_are_null() + { + Song song = BuildUntaggedSong(); + Channel channel = BuildChannel(); + + // SongVideoGenerator randomly picks between two rendering styles (and dereferences + // metadata.Artists/AlbumArtists differently in each); loop enough times that both + // branches -- including the AlbumArtists.Filter(... Artists.Contains ...) branch -- + // are exercised with overwhelming probability, so the null guard is proven on both. + for (var i = 0; i < 25; i++) + { + Tuple result = await _songVideoGenerator.GenerateSongVideo( + song, + channel, + "/usr/bin/ffmpeg", + "/usr/bin/ffprobe", + CancellationToken.None); + + result.ShouldNotBeNull(); + result.Item1.ShouldBe("/fake/song-image.png"); + } + } +} diff --git a/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs b/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs index 435502bfe..747c9d45c 100644 --- a/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs +++ b/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs @@ -1,10 +1,31 @@ -namespace ErsatzTV.Core.Domain; +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 _artists; + private IList _albumArtists; + public string Album { get; set; } - public IList Artists { get; set; } - public IList AlbumArtists { get; set; } + + public IList Artists + { + get => _artists ??= []; + set => _artists = value; + } + + public IList AlbumArtists + { + get => _albumArtists ??= []; + set => _albumArtists = value; + } + public string Track { get; set; } public string Comment { get; set; } public int SongId { get; set; } -- 2.47.3 From dd7b58232c981268474ebaa7013e10788e4c6ddc Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 28 Jul 2026 22:39:35 +0200 Subject: [PATCH 2/2] 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 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) --- ErsatzTV.Core/Domain/Metadata/SongMetadata.cs | 25 ++----------------- ErsatzTV.Core/FFmpeg/SongVideoGenerator.cs | 17 +++++++------ .../Repositories/MediaCollectionRepository.cs | 4 +-- 3 files changed, 14 insertions(+), 32 deletions(-) diff --git a/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs b/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs index 747c9d45c..853eb84e0 100644 --- a/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs +++ b/ErsatzTV.Core/Domain/Metadata/SongMetadata.cs @@ -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 _artists; - private IList _albumArtists; - public string Album { get; set; } - - public IList Artists - { - get => _artists ??= []; - set => _artists = value; - } - - public IList AlbumArtists - { - get => _albumArtists ??= []; - set => _albumArtists = value; - } - + public IList Artists { get; set; } + public IList AlbumArtists { get; set; } public string Track { get; set; } public string Comment { get; set; } public int SongId { get; set; } diff --git a/ErsatzTV.Core/FFmpeg/SongVideoGenerator.cs b/ErsatzTV.Core/FFmpeg/SongVideoGenerator.cs index e2a45d880..8d330dc41 100644 --- a/ErsatzTV.Core/FFmpeg/SongVideoGenerator.cs +++ b/ErsatzTV.Core/FFmpeg/SongVideoGenerator.cs @@ -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 artists = Optional(metadata.Artists).Flatten().ToList(); + List 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}"); } diff --git a/ErsatzTV.Infrastructure/Data/Repositories/MediaCollectionRepository.cs b/ErsatzTV.Infrastructure/Data/Repositories/MediaCollectionRepository.cs index 47b2b9d16..56ea46160 100644 --- a/ErsatzTV.Infrastructure/Data/Repositories/MediaCollectionRepository.cs +++ b/ErsatzTV.Infrastructure/Data/Repositories/MediaCollectionRepository.cs @@ -1144,7 +1144,7 @@ public class MediaCollectionRepository : IMediaCollectionRepository var allArtists = items.OfType() .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()) { string firstArtist = song.SongMetadata - .SelectMany(sm => sm.AlbumArtists) + .SelectMany(sm => Optional(sm.AlbumArtists).Flatten()) .HeadOrNone() .Match(aa => aa, string.Empty); -- 2.47.3