From b5dee26202722fd721d019cb71fcb0a50d3d5279 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 22 Aug 2026 23:53:57 +0200 Subject: [PATCH] fix(701): guard SongMetadata's nullable primitive collections at the read site 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 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 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) --- .../Commands/RefreshChannelDataHandler.cs | 11 +- .../Search/ElasticSearchIndex.cs | 116 ++++----- .../Search/LuceneSearchIndex.cs | 19 +- .../SongIndexerMetadataMutationTests.cs | 230 ++++++++++++++++++ docs/decisions/README.md | 1 + .../nullable-primitive-collection-mutation.md | 68 ++++++ 6 files changed, 379 insertions(+), 66 deletions(-) create mode 100644 ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs create mode 100644 docs/decisions/records/media/nullable-primitive-collection-mutation.md diff --git a/ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs b/ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs index 9d50f1201..0c683daf1 100644 --- a/ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs @@ -595,6 +595,13 @@ public class RefreshChannelDataHandler : IRequestHandler metadata.Genres ??= []; metadata.Studios ??= []; + // Artists/AlbumArtists are NULLABLE primitive collections, so they are guarded at the read site + // rather than assigned back onto `metadata` like the navigations above (ersatztv#701/#691): they + // are scalar JSON-array columns, so `??= []` on a tracked entity would persist `[]` over NULL. + // The shipped `_song.sbntxt` only does `array.join`, but a user template is free to do anything. + List songArtists = Optional(metadata.Artists).Flatten().ToList(); + List songAlbumArtists = Optional(metadata.AlbumArtists).Flatten().ToList(); + string artworkPath = GetPrioritizedArtworkPath(metadata); var data = new @@ -607,8 +614,8 @@ public class RefreshChannelDataHandler : IRequestHandler HasCustomTitle = hasCustomTitle, displayItem.CustomTitle, SongTitle = subtitle, - SongArtists = metadata.Artists, - SongAlbumArtists = metadata.AlbumArtists, + SongArtists = songArtists, + SongAlbumArtists = songAlbumArtists, SongHasYear = metadata.Year.HasValue, SongYear = metadata.Year, SongGenres = metadata.Genres.Map(g => g.Name).OrderBy(n => n), diff --git a/ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs b/ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs index 657498174..592878a71 100644 --- a/ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs +++ b/ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs @@ -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 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 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 artists = Optional(metadata.Artists).Flatten().ToList(); + List 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() diff --git a/ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs b/ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs index 41e9fb856..11bc42ace 100644 --- a/ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs +++ b/ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs @@ -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 artists = Optional(metadata.Artists).Flatten().ToList(); + List 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)); } diff --git a/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs new file mode 100644 index 000000000..b22b43662 --- /dev/null +++ b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs @@ -0,0 +1,230 @@ +using System.Reflection; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Metadata; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Search; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.Extensions; +using ErsatzTV.Infrastructure.Search; +using ErsatzTV.Tests.Support; +using Lucene.Net.Analysis.Standard; +using Lucene.Net.Index; +using Lucene.Net.Store; +using Lucene.Net.Util; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#701, EXECUTED against a real on SQLite. +/// +/// The defect. LuceneSearchIndex.UpdateSong opened with +/// metadata.AlbumArtists ??= []; metadata.Artists ??= [];. Unlike the navigation +/// collections guarded the same way all around them, these two are SCALAR COLUMNS: the whole +/// list lives in one column, so the property IS the column value. They are two of EIGHT such +/// columns in the model — the population is derived from the MODEL CONFIGURATION (EF-native +/// primitive collections plus the six HasConversion<*CollectionValueConverter> +/// columns on ProgramScheduleAlternate/PlayoutTemplate), NOT by grepping the +/// domain classes for IList<string>, which finds only two of the eight. See +/// the decision record media.nullable-primitive-collection-mutation. +/// Assigning one on a TRACKED entity flips it to , and the next +/// SaveChanges writes [] over what the database held as NULL — the exact +/// mechanism an adversarial review demonstrated in ersatztv#691, which is why that issue's +/// entity-level guard was reverted in favour of guarding at the READ SITE. +/// +/// +/// Why the fixture loads the song TRACKED even though production does not. Both feeds into +/// the indexer are AsNoTracking() today — SearchRepository.GetItemToIndex and +/// SearchRepository.GetAllSongs — so no shipped caller loses data. That is a property of +/// today's two callers, not of the indexer, and it is exactly what ersatztv#691 recorded as "a +/// loaded gun". This fixture therefore pins the INDEXER's own contract: handed a tracked entity it +/// must not mutate it. Run against the real pre-fix file, the FIRST of the numbered assertions +/// below fails (metadata.Artists should be null but was []) and the run stops there; +/// reaching the persistence half needs a probe variant with assertions 1 and 2 replaced by +/// prints, which reports Modified and the column moving from NULL to []. +/// Each was separately shown discriminating. A future caller that drops AsNoTracking +/// therefore cannot reintroduce the data loss silently. +/// +/// +/// The Lucene is injected into the private field rather than obtained via +/// Initialize, because Initialize writes to FileSystemLayout.SearchIndexFolder — +/// a process-wide static resolved once from ETV_CONFIG_FOLDER, i.e. the developer's real +/// application data folder. Letting the writer throw instead is NOT an option here: the +/// catch in UpdateSong assigns metadata.Song = null, which would itself dirty +/// the entity under test and make the probe report the wrong cause. +/// +/// +[TestFixture] +public class SongIndexerMetadataMutationTests +{ + [Test] + public async Task UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity() + { + await using var harness = await InMemoryTvContext.CreateAsync(); + + int metadataId; + await using (TvContext context = harness.CreateContext()) + { + var library = new LocalLibrary { Name = "Music", MediaKind = LibraryMediaKind.Songs }; + context.Add(library); + await context.SaveChangesAsync(); + + var libraryPath = new LibraryPath { Path = "/music", LibraryId = library.Id }; + context.Add(libraryPath); + await context.SaveChangesAsync(); + + var song = new Song + { + LibraryPathId = libraryPath.Id, + MediaVersions = [], + SongMetadata = + [ + new SongMetadata + { + MetadataKind = MetadataKind.Fallback, + Title = "Untagged Track", + SortTitle = "untagged track", + DateAdded = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc), + + // The shape FallbackMetadataProvider.GetSongMetadata leaves behind: it never + // assigns either primitive collection, so both columns persist as NULL. + Artists = null!, + AlbumArtists = null!, + + Genres = [], + Tags = [], + Studios = [], + Actors = [], + Artwork = [], + Guids = [] + } + ] + }; + + context.Add(song); + await context.SaveChangesAsync(); + metadataId = song.SongMetadata[0].Id; + } + + // The seed must actually have produced NULL columns, or every assertion below is vacuous. + (await ReadRawArtists(harness, metadataId)).ShouldBeNull(); + + await using (TvContext context = harness.CreateContext()) + { + // Deliberately TRACKED -- see the fixture docstring. + Song tracked = await context.Songs + .IncludeForSearch() + .AsSplitQuery() + .SingleAsync(); + + SongMetadata metadata = tracked.SongMetadata[0]; + metadata.Artists.ShouldBeNull("EF must materialize the NULL column as null, not as an empty list"); + + // UpdateSong wraps its whole body in a catch that logs a warning and assigns + // `metadata.Song = null` -- which severs a required relationship and cascades the metadata to + // Deleted. A silently-exercised catch would therefore make every assertion below report the + // wrong cause, so the logger fails the test instead of swallowing. + var logger = new ThrowOnWarningLogger(); + var index = new LuceneSearchIndex( + new SearchQueryParser( + Substitute.For(), + Substitute.For>()), + logger); + + using var directory = new RAMDirectory(); + using var writer = new IndexWriter( + directory, + new IndexWriterConfig(LuceneVersion.LUCENE_48, new StandardAnalyzer(LuceneVersion.LUCENE_48))); + + typeof(LuceneSearchIndex) + .GetField("_writer", BindingFlags.NonPublic | BindingFlags.Instance)! + .SetValue(index, writer); + + // A bare substitute returns null from GetAllLanguageCodes, which NPEs inside AddLanguages and + // would divert the run into the catch above. + var languageCodeService = Substitute.For(); + languageCodeService.GetAllLanguageCodes(Arg.Any>()).Returns([]); + languageCodeService.GetAllLanguageCodes(Arg.Any()).Returns([]); + + await index.UpdateItems( + Substitute.For(), + Substitute.For(), + languageCodeService, + [tracked]); + + logger.Failure.ShouldBeNull("UpdateSong threw and its catch ran, so this probe measured the " + + "error path rather than the indexing path"); + + // POSITIVE CONTROL. Every assertion below asserts that something did NOT happen, so all of + // them hold vacuously if UpdateSong never ran at all -- and it silently stops running if a + // future refactor gates UpdateItems on `_initialized`, which this fixture deliberately + // bypasses by injecting the writer. Verified BOTH ways by adding + // `if (!_initialized) { return Unit.Default; }` to UpdateItems (a bare `return;` does not + // compile there -- CS0126): with this line present it is the only failure, and with it + // removed the whole test PASSES while the code under test is unreachable. + // NumDocs == 1 proves the song-indexing path ran; it does NOT prove the artist loops + // specifically ran, which would need a second seeded song asserting ArtistField. + writer.NumDocs.ShouldBe(1, "UpdateSong did not index the song, so the assertions below " + + "would pass without exercising the code under test"); + + // 1. The indexer left the entity alone. + metadata.Artists.ShouldBeNull(); + metadata.AlbumArtists.ShouldBeNull(); + + // 2. ...so EF has nothing to persist. This is the assertion that fails loudly the day the + // mutation returns, even if a later refactor stopped the value from being observable above. + context.Entry(metadata).State.ShouldBe(EntityState.Unchanged); + + // 3. And the save that a real caller would go on to make does not rewrite the column. + await context.SaveChangesAsync(); + } + + (await ReadRawArtists(harness, metadataId)).ShouldBeNull(); + } + + private sealed class ThrowOnWarningLogger : ILogger + { + public Exception? Failure { get; private set; } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (logLevel >= LogLevel.Warning) + { + Failure ??= exception ?? new InvalidOperationException(formatter(state, exception)); + } + } + } + + private static async Task ReadRawArtists(InMemoryTvContext harness, int metadataId) + { + await using TvContext context = harness.CreateContext(); + await using var command = context.Database.GetDbConnection().CreateCommand(); + command.CommandText = $"SELECT Artists FROM SongMetadata WHERE Id = {metadataId}"; + object? value = await command.ExecuteScalarAsync(); + + // ExecuteScalar returns CLR null both for "the column is NULL" and for "there is no such row", + // and the second is reachable: UpdateSong's catch assigns metadata.Song = null, which severs a + // required relationship and cascades the row to Deleted, so a SaveChanges on the error path + // DELETES it and a plain null check would pass for the wrong reason. + if (value is null) + { + Assert.Fail($"SongMetadata row {metadataId} no longer exists, so its Artists column cannot " + + "be read -- the probe measured a deleted row rather than a preserved NULL."); + } + + return value is DBNull ? null : value; + } +} diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 762800a84..6e834b1e4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -102,6 +102,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | | `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint's query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | +| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn 'DaysOfMonth ??=\\|MonthsOfYear ??=\\|DaysOfWeek ??=' --include='*.cs' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | | `media.source-mgmt-write-api` | Media-source management (local/Plex/Jellyfin/Emby) is a REST write API + SPA under `/app/libraries/*`, wrapping existing MediatR commands 1:1 with no new commands or DB migration; connection GETs never leak a stored `apiKey`, and each PUT-replace family's identity contract is documented per-family (not assumed uniform). | 2026-07-11 | [link](records/media/source-mgmt-write-api.md) | diff --git a/docs/decisions/records/media/nullable-primitive-collection-mutation.md b/docs/decisions/records/media/nullable-primitive-collection-mutation.md new file mode 100644 index 000000000..5989a0593 --- /dev/null +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -0,0 +1,68 @@ +--- +key: media.nullable-primitive-collection-mutation +title: '2026-08-22 — A nullable primitive collection is guarded at the READ SITE and never assigned back onto a possibly-tracked entity (#701)' +status: active +since: '2026-08-22' +supersedes: none +superseded-by: none +rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn ''DaysOfMonth ??=\|MonthsOfYear ??=\|DaysOfWeek ??='' --include=''*.cs'' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.' +signals: 'Artists ??= [] on a tracked entity · nullable primitive collection not a navigation · JSON array in one column · value converter is the same hazard as a primitive collection · IntCollectionValueConverter EnumCollectionJsonValueConverter · derive the collection-column population from the model configuration · SaveChanges writes empty array over NULL · entity flips to Modified on a read guard · AsNoTracking today is not a safety argument · untagged song loses its NULL artists · foreach over null throws NRE while string.Join throws ArgumentNullException · sweep by FIELD not by file · Optional Flatten hoisted local · paths: `ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs`, `ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs`, `ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs`, `ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs` · issues: #701, #691, #671, #823, #824' +mechanics: 'Pinned by `SongIndexerMetadataMutationTests.UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity`, which drives the real `LuceneSearchIndex` against a real `TvContext` on SQLite with a deliberately TRACKED song. That fixture covers LUCENE ONLY — `ElasticSearchIndex` holds an independent copy of the same code and needs a stubbed transport, so an Elastic-only reintroduction stays green (#824). No repo-wide detector: the `??=` idiom is correct on navigations and appears 92 times across the app projects (`grep -rn ''??= '' --include=''*.cs'' ErsatzTV ErsatzTV.Core ErsatzTV.Application ErsatzTV.Infrastructure ErsatzTV.Scanner | grep -v ''/obj/\|/bin/'' | grep -c ''''`, 2026-08-22), so a grep for it would be noise — the eight-column population is small enough to sweep by field instead.' +--- + +**Deriving this population from the domain classes gives the wrong answer.** "The `IList` +properties under `ErsatzTV.Core/Domain`" is the derivation that looks obviously right, and it is +REJECTED: it returns two of the eight. It is blind to the six value-converted collections, which are +declared as ordinary `ICollection` / `ICollection` and become single columns only in +`ErsatzTV.Infrastructure/Data/Configurations`, and blind to enum collections entirely. The +authoritative source is the model configuration — `HasConversion<*CollectionValueConverter, …>` plus +EF's native primitive-collection mapping. This is `testing.guard-derives-population-from-source` +applied to a sweep rather than to a guard. + +**The idiom is right almost everywhere it appears, which is what makes this hard to see.** `??= []` +on `metadata.Genres`, `metadata.Tags`, `metadata.Artwork` and their kin is harmless: those are +navigation collections, and setting a null navigation to an empty list is not a scalar property +change, so EF has nothing to persist. The reader who wrote `metadata.Artists ??= []` two lines below +`metadata.Genres ??= []` was following the surrounding code correctly. The difference is invisible at +the call site and lives in the model: `Artists` is a primitive collection — one column holding the +whole list. + +**Why the "it is `AsNoTracking` today" argument is banned rather than merely weak.** Both feeds into +the search indexer — `SearchRepository.GetItemToIndex` and `SearchRepository.GetAllSongs` — are +`AsNoTracking`, so no shipped caller loses data, and that was true when #691 looked at it too. It is +a fact about two callers. Nothing in the indexer requires it, nothing tests for it, and a future +caller that drops `AsNoTracking` to reuse an existing context reintroduces silent data loss with no +diff anywhere near the indexer. Writing the observation down as a justification is what converts a +latent bug into a checked decision that talks the next reader out of verifying. + +**The measurement, so it is not re-argued.** Restoring only the `??= []` clause — the real predecessor +lines, not a hand-written mutant — and re-running the fixture reports +`metadata.Artists should be null but was []`, and stops there: the first assertion short-circuits. +The persistence half needs a probe VARIANT with assertions 1 and 2 replaced by prints, which reports +`STATE=Modified` and the raw column moving from `NULL` to `"[]"`. Both halves were executed. The +recipe is spelled out to that level of detail because the short version is not runnable as stated: +following it produces only the first failure, which reads as the record overstating itself. + +**The fixture's two anti-vacuity guards.** A POSITIVE CONTROL (`writer.NumDocs.ShouldBe(1)`) is +required because every other assertion says something did NOT happen, so all of them hold vacuously +if `UpdateSong` never runs. Measured: gate `UpdateItems` on `_initialized` — which this fixture +bypasses by injecting the writer, so it is a plausible refactor — and with the control removed the +test PASSES with the code under test unreachable. Separately, the fixture fails loudly if +`UpdateSong` throws, because that method wraps its whole body in a `catch` that assigns +`metadata.Song = null` — which severs a required relationship and cascades the metadata to +`Deleted`. Without that check a probe silently measures the error path and reports the wrong cause, +and the raw-column helper likewise fails on a MISSING row, since `ExecuteScalar` returns CLR null +both for a NULL column and for no such row. + +**Removing the assignment is not sufficient on its own.** The `??= []` was load-bearing for the four +reads below it (`foreach (string artist in metadata.Artists)`, `metadata.Artists.ToList()`). Deleting +it alone 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 deliberately: 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. That is the same trap #691 hit from the other direction, and it is why the rule pairs the +removal with the read-site guard rather than stating them separately. + +Related: `api.selection-projection-include-chain` (#671) records the read-site guard itself and the +sweep-by-FIELD instruction; this record covers the write half it does not address.