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>
This commit is contained in:
@@ -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<ITempFilePool>();
|
||||
_tempFilePool.GetNextTempFile(Arg.Any<TempFileCategory>()).Returns(_tempSubtitleFile);
|
||||
|
||||
_imageCache = Substitute.For<IImageCache>();
|
||||
_imageCache.GetPathForImage(Arg.Any<string>(), Arg.Any<ArtworkKind>(), Arg.Any<Option<int>>())
|
||||
.Returns("/fake/watermark.png");
|
||||
|
||||
_ffmpegProcessService = Substitute.For<IFFmpegProcessService>();
|
||||
_ffmpegProcessService.GenerateSongImage(
|
||||
Arg.Any<string>(),
|
||||
Arg.Any<string>(),
|
||||
Arg.Any<Option<string>>(),
|
||||
Arg.Any<Channel>(),
|
||||
Arg.Any<MediaVersion>(),
|
||||
Arg.Any<string>(),
|
||||
Arg.Any<bool>(),
|
||||
Arg.Any<Option<string>>(),
|
||||
Arg.Any<WatermarkLocation>(),
|
||||
Arg.Any<int>(),
|
||||
Arg.Any<int>(),
|
||||
Arg.Any<int>(),
|
||||
Arg.Any<CancellationToken>())
|
||||
.Returns(Either<BaseError, string>.Right("/fake/song-image.png"));
|
||||
|
||||
_localFileSystem = Substitute.For<ILocalFileSystem>();
|
||||
_localFileSystem.GetCustomOrDefaultFile(Arg.Any<string>(), Arg.Any<string>())
|
||||
.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<string, MediaVersion> result = await _songVideoGenerator.GenerateSongVideo(
|
||||
song,
|
||||
channel,
|
||||
"/usr/bin/ffmpeg",
|
||||
"/usr/bin/ffprobe",
|
||||
CancellationToken.None);
|
||||
|
||||
result.ShouldNotBeNull();
|
||||
result.Item1.ShouldBe("/fake/song-image.png");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<string> _artists;
|
||||
private IList<string> _albumArtists;
|
||||
|
||||
public string Album { get; set; }
|
||||
public IList<string> Artists { get; set; }
|
||||
public IList<string> AlbumArtists { get; set; }
|
||||
|
||||
public IList<string> Artists
|
||||
{
|
||||
get => _artists ??= [];
|
||||
set => _artists = value;
|
||||
}
|
||||
|
||||
public IList<string> AlbumArtists
|
||||
{
|
||||
get => _albumArtists ??= [];
|
||||
set => _albumArtists = value;
|
||||
}
|
||||
|
||||
public string Track { get; set; }
|
||||
public string Comment { get; set; }
|
||||
public int SongId { get; set; }
|
||||
|
||||
Reference in New Issue
Block a user