fix(701): guard SongMetadata's nullable primitive collections at the read site
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 8s
PR Gates / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 19s
review-verdict/h10 Review-verdict: MERGEABLE @ b5dee26 (base: main)
PR Gates / decisions lifecycle (pull_request) Successful in 20s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 15s
Review verdict / Set review-verdict status (pull_request_target) Successful in 6s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 4m32s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m44s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m18s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 5m53s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 8s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 6s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 8s
PR Gates / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 19s
review-verdict/h10 Review-verdict: MERGEABLE @ b5dee26 (base: main)
PR Gates / decisions lifecycle (pull_request) Successful in 20s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 15s
Review verdict / Set review-verdict status (pull_request_target) Successful in 6s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 4m32s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m44s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m18s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 5m53s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 8s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 6s
Both search indexers opened UpdateSong with
metadata.AlbumArtists ??= [];
metadata.Artists ??= [];
Artists/AlbumArtists hold the whole list in ONE COLUMN rather than
being navigations. So unlike the same `??= []` idiom on
Genres/Tags/Artwork all around them, the property IS the column value:
assigning it on a TRACKED entity flips the entry to Modified and the
next SaveChanges writes [] over a NULL column. This is the mechanism an
adversarial review demonstrated in #691, which is why that issue's
entity-level guard was reverted in favour of guarding at the read site.
Measured rather than reasoned about, per the issue's first done-when
box. Restoring ONLY the `??= []` clause (the real predecessor lines,
not a hand-written mutant) reddens the new fixture on
`metadata.Artists should be null but was []`; a probe variant with the
first two assertions replaced by prints reports STATE=Modified and the
raw column moving from NULL to "[]". Today's two feeds are both
AsNoTracking (SearchRepository.GetItemToIndex and GetAllSongs), so no
shipped caller loses data -- but that is a property of two callers, not
of the indexer, and #691 already recorded it as a loaded gun. The
fixture pins the indexer's own contract instead.
Removing the assignment is not sufficient alone: it was load-bearing
for the four reads below it, and deleting it by itself converts a
silent write into a live throw on every untagged song. Measured by
deleting only those two lines from the real predecessor file:
NullReferenceException, thrown at the foreach (cited by symbol: a line
number in a mutant that exists in no committed tree is unreproducible
by construction). The
exception type follows the read FORM, not the field -- foreach yields
NRE, string.Join/ToList yield ArgumentNullException -- and this PR
contains two of each, which is why no single exception-name grep
characterises the class. So each site moves together with its reads:
- LuceneSearchIndex.UpdateSong / ElasticSearchIndex.UpdateSong: hoist
Optional(...).Flatten().ToList() locals and read those.
- RefreshChannelDataHandler: the Scriban context took the raw nullable
lists (the issue's second item). The shipped _song.sbntxt only does
array.join, but a custom template is free to do anything.
The population was derived from the MODEL rather than from the issue's
file list, and the obvious derivation is wrong: "the IList<string>
properties under ErsatzTV.Core/Domain" returns two of eight. It misses
the six value-converted collections (ProgramScheduleAlternate and
PlayoutTemplate each carrying DaysOfMonth, MonthsOfYear, DaysOfWeek),
declared as plain ICollection<T> and made single columns only in
Data/Configurations -- and their storage differs (comma-separated text
for the int converter, JSON for the enum one), so the shared property
is "one scalar column", not the serialization. No site applies `??=`
to any of the six, so this defect has no instance there; whether a null
can REACH one at runtime is unverified and is filed as #823 rather than
asserted either way. Only the SongMetadata pair is left NULL in
practice, by FallbackMetadataProvider. Every site touching either field
was then swept; the remaining readers were already guarded by #691.
The fixture carries two anti-vacuity guards, both witnessed:
- A POSITIVE CONTROL (`writer.NumDocs.ShouldBe(1)`). Every other
assertion says something did NOT happen, so all of them hold
vacuously if UpdateSong never runs -- and it silently stops running
if a future refactor gates UpdateItems on `_initialized`, which this
fixture bypasses by injecting the writer. Verified BOTH directions:
with that gate added the control fails `NumDocs should be 1 but was
0`, and with the control removed the whole test PASSES while the code
under test is unreachable.
- A capturing logger, because UpdateSong wraps its body in a catch that
assigns metadata.Song = null -- severing a required relationship and
cascading the metadata to Deleted. Without it the probe silently
measures the error path; on the first run it did exactly that (a bare
ILanguageCodeService substitute NPEs inside AddLanguages). The raw
column helper also fails loudly on a missing row, since ExecuteScalar
returns CLR null for both "NULL column" and "no such row".
ElasticSearchIndex has no equivalent fixture -- it needs a stubbed
transport -- so its change is by inspection against the Lucene one, and
the gap is filed as #824 rather than covered by a source-text guard.
The whitespace-only churn in ElasticSearchIndex.cs is the #311
fix-as-you-touch format gate: it scopes to whole changed FILES.
`git diff -w` over that file shows only the two hunks above.
Local gate: ErsatzTV.Tests 2006 passed / 4 pre-existing skips,
Core.Tests 685/1 skip, Infrastructure.Tests 114, Architecture.Tests 7,
Scanner.Tests 1504 -- 0 failures in each. scripts/tests 874 passed / 2
skipped. dotnet format whitespace --verify-no-changes clean on the four
touched files, no BOM on any. decisions_validate OK.
Fixes #701
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -440,64 +440,64 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
Season season)
|
||||
{
|
||||
foreach (SeasonMetadata metadata in season.SeasonMetadata.HeadOrNone())
|
||||
foreach (ShowMetadata showMetadata in season.Show.ShowMetadata.HeadOrNone())
|
||||
{
|
||||
try
|
||||
foreach (ShowMetadata showMetadata in season.Show.ShowMetadata.HeadOrNone())
|
||||
{
|
||||
var seasonTitle = $"{showMetadata.Title} - S{season.SeasonNumber}";
|
||||
string sortTitle = $"{showMetadata.SortTitle}_{season.SeasonNumber:0000}"
|
||||
.ToLowerInvariant();
|
||||
string titleAndYear = $"{showMetadata.Title}_{showMetadata.Year}_{season.SeasonNumber}"
|
||||
.ToLowerInvariant();
|
||||
|
||||
var doc = new ElasticSearchItem
|
||||
try
|
||||
{
|
||||
Id = season.Id,
|
||||
Type = LuceneSearchIndex.SeasonType,
|
||||
Title = seasonTitle,
|
||||
SortTitle = sortTitle,
|
||||
LibraryName = season.LibraryPath.Library.Name,
|
||||
LibraryId = season.LibraryPath.Library.Id,
|
||||
TitleAndYear = titleAndYear,
|
||||
TitleAndYearSearch = LuceneSearchIndex.GetTitleAndYearSearch(metadata),
|
||||
JumpLetter = LuceneSearchIndex.GetJumpLetter(showMetadata),
|
||||
State = season.State.ToString(),
|
||||
SeasonNumber = season.SeasonNumber,
|
||||
ShowTitle = showMetadata.Title,
|
||||
ShowGenre = showMetadata.Genres.Map(g => g.Name).ToList(),
|
||||
ShowTag = showMetadata.Tags.Map(t => t.Name).ToList(),
|
||||
ShowStudio = showMetadata.Studios.Map(s => s.Name).ToList(),
|
||||
ShowContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
Language = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetLanguagesForSeason(season)),
|
||||
LanguageTag = await searchRepository.GetLanguagesForSeason(season),
|
||||
SubLanguage = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetSubLanguagesForSeason(season)),
|
||||
SubLanguageTag = await searchRepository.GetSubLanguagesForSeason(season),
|
||||
ContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
ReleaseDate = GetReleaseDate(metadata.ReleaseDate),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
TraktList = season.TraktListItems
|
||||
.Map(t => t.TraktList.TraktId.ToString(CultureInfo.InvariantCulture)).ToList(),
|
||||
Tag = metadata.Tags.Map(a => a.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
};
|
||||
var seasonTitle = $"{showMetadata.Title} - S{season.SeasonNumber}";
|
||||
string sortTitle = $"{showMetadata.SortTitle}_{season.SeasonNumber:0000}"
|
||||
.ToLowerInvariant();
|
||||
string titleAndYear = $"{showMetadata.Title}_{showMetadata.Year}_{season.SeasonNumber}"
|
||||
.ToLowerInvariant();
|
||||
|
||||
foreach ((string key, List<string> value) in GetMetadataGuids(metadata))
|
||||
{
|
||||
doc.AdditionalProperties.Add(key, value);
|
||||
var doc = new ElasticSearchItem
|
||||
{
|
||||
Id = season.Id,
|
||||
Type = LuceneSearchIndex.SeasonType,
|
||||
Title = seasonTitle,
|
||||
SortTitle = sortTitle,
|
||||
LibraryName = season.LibraryPath.Library.Name,
|
||||
LibraryId = season.LibraryPath.Library.Id,
|
||||
TitleAndYear = titleAndYear,
|
||||
TitleAndYearSearch = LuceneSearchIndex.GetTitleAndYearSearch(metadata),
|
||||
JumpLetter = LuceneSearchIndex.GetJumpLetter(showMetadata),
|
||||
State = season.State.ToString(),
|
||||
SeasonNumber = season.SeasonNumber,
|
||||
ShowTitle = showMetadata.Title,
|
||||
ShowGenre = showMetadata.Genres.Map(g => g.Name).ToList(),
|
||||
ShowTag = showMetadata.Tags.Map(t => t.Name).ToList(),
|
||||
ShowStudio = showMetadata.Studios.Map(s => s.Name).ToList(),
|
||||
ShowContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
Language = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetLanguagesForSeason(season)),
|
||||
LanguageTag = await searchRepository.GetLanguagesForSeason(season),
|
||||
SubLanguage = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetSubLanguagesForSeason(season)),
|
||||
SubLanguageTag = await searchRepository.GetSubLanguagesForSeason(season),
|
||||
ContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
ReleaseDate = GetReleaseDate(metadata.ReleaseDate),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
TraktList = season.TraktListItems
|
||||
.Map(t => t.TraktList.TraktId.ToString(CultureInfo.InvariantCulture)).ToList(),
|
||||
Tag = metadata.Tags.Map(a => a.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
};
|
||||
|
||||
foreach ((string key, List<string> value) in GetMetadataGuids(metadata))
|
||||
{
|
||||
doc.AdditionalProperties.Add(key, value);
|
||||
}
|
||||
|
||||
await _client.IndexAsync(doc, IndexName, ES.Id.From(doc));
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
metadata.Season = null;
|
||||
_logger.LogWarning(ex, "Error indexing season with metadata {@Metadata}", metadata);
|
||||
}
|
||||
|
||||
await _client.IndexAsync(doc, IndexName, ES.Id.From(doc));
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
metadata.Season = null;
|
||||
_logger.LogWarning(ex, "Error indexing season with metadata {@Metadata}", metadata);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private async Task UpdateArtist(
|
||||
@@ -763,8 +763,10 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
{
|
||||
try
|
||||
{
|
||||
metadata.AlbumArtists ??= [];
|
||||
metadata.Artists ??= [];
|
||||
// Guard the two NULLABLE primitive collections at the READ SITE, never by assigning back onto
|
||||
// `metadata` (ersatztv#701) -- see the matching comment in LuceneSearchIndex.UpdateSong.
|
||||
List<string> artists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> albumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
var doc = new ElasticSearchItem
|
||||
{
|
||||
@@ -785,8 +787,8 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
SubLanguageTag = GetSubLanguageTags(song.MediaVersions),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
Album = metadata.Album ?? string.Empty,
|
||||
Artist = metadata.Artists.ToList(),
|
||||
AlbumArtist = metadata.AlbumArtists.ToList(),
|
||||
Artist = artists,
|
||||
AlbumArtist = albumArtists,
|
||||
Genre = metadata.Genres.Map(g => g.Name).ToList(),
|
||||
Tag = metadata.Tags.Map(t => t.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
using System.Globalization;
|
||||
using System.Globalization;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Metadata;
|
||||
@@ -145,7 +145,7 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
_directory = FSDirectory.Open(FileSystemLayout.SearchIndexFolder);
|
||||
Analyzer analyzer = SearchQueryParser.AnalyzerWrapper();
|
||||
var indexConfig = new IndexWriterConfig(AppLuceneVersion, analyzer)
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
_writer = new IndexWriter(_directory, indexConfig);
|
||||
_initialized = true;
|
||||
}
|
||||
@@ -328,7 +328,7 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
using (Analyzer analyzer = SearchQueryParser.AnalyzerWrapper())
|
||||
{
|
||||
var indexConfig = new IndexWriterConfig(AppLuceneVersion, analyzer)
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
using (var w = new IndexWriter(d, indexConfig))
|
||||
{
|
||||
using (DirectoryReader _ = w.GetReader(true))
|
||||
@@ -1318,8 +1318,13 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
{
|
||||
try
|
||||
{
|
||||
metadata.AlbumArtists ??= [];
|
||||
metadata.Artists ??= [];
|
||||
// Guard the two NULLABLE primitive collections at the READ SITE, never by assigning back onto
|
||||
// `metadata` (ersatztv#701). The entity reaching here may be TRACKED, and Artists/AlbumArtists
|
||||
// are scalar JSON-array columns rather than navigations -- so `??= []` flips the entity to
|
||||
// Modified and the next SaveChanges writes `[]` over a NULL column. Same convention as
|
||||
// SongVideoGenerator and MediaCollectionRepository (ersatztv#691).
|
||||
List<string> artists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> albumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
var doc = new Document
|
||||
{
|
||||
@@ -1355,12 +1360,12 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
doc.Add(new TextField(AlbumField, metadata.Album, Field.Store.NO));
|
||||
}
|
||||
|
||||
foreach (string artist in metadata.Artists)
|
||||
foreach (string artist in artists)
|
||||
{
|
||||
doc.Add(new TextField(ArtistField, artist, Field.Store.NO));
|
||||
}
|
||||
|
||||
foreach (string albumArtist in metadata.AlbumArtists)
|
||||
foreach (string albumArtist in albumArtists)
|
||||
{
|
||||
doc.Add(new TextField(AlbumArtistField, albumArtist, Field.Store.NO));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user