From 95b2700f094f579e49fabe659a758592fa22751f Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 19:16:09 +0200 Subject: [PATCH 1/5] fix(823,824): a scheduling NULL collection reads as UNRESTRICTED and is guarded at both read sites; the Elastic indexer gets its own mutation proof Both issues are #701 deferrals, and they land together because both rewrite the same decision record. #823 -- can a null reach one of the six collection-valued scalar columns? MEASURED against a real TvContext on BOTH providers (SQLite, and MySQL 8.4 on an ephemeral server), because the reasoning available beforehand pointed the wrong way. The two converters differ on their read side -- IntCollectionValueConverter maps null-or-blank to Array.Empty(), while EnumCollectionJsonValueConverter would dereference the result of JsonConvert.DeserializeObject -- so the expectation was that a NULL row behaves differently per column. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all. All six materialize as CLR null, the int converter's null-to-empty branch is dead on this path, and unguarded each .Contains in AlternateScheduleSelector throws NullReferenceException. A NULL reads as UNRESTRICTED -- the All*() sets -- not as empty. This is the whole semantic question and the first draft got it backwards. It is decided by the one NULL reachable WITHOUT any code writing one: Sqlite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth adds the column with nullable:true and NO defaultValue, so a PlayoutTemplate row inserted before it holds NULL and by construction had no day-of-month restriction. Reading that as empty INVERTS the row's meaning and silently stops the template applying at all. All*() preserves it, and is how "no restriction recorded" is already represented (GetPlayoutAlternateSchedulesHandler, PreviewBlockPlayoutHandler). What does NOT decide it, and was wrongly cited in the first draft: the API request records normalize an omitted field with `?? []`, but that is a client omitting a field on a WRITE and says nothing about what a legacy database NULL meant. Two read sites, not one. Guarding only the selector would have left the entity->DTO mappers unguarded, and those feed the SPA: PlayoutScheduleEditors spreads the collection (`[...template.daysOfMonth]` -> TypeError on a JSON null) and playoutTemplateCalendar's appliesToDate -- an exact port of GetScheduleForDate -- calls .includes on it. Both mappers now substitute the SAME defaults, so the preview agrees with what is actually scheduled. Neither guard is assigned back onto the entity, which is the media.nullable-primitive-collection-mutation mechanism. Reachability, stated precisely rather than overclaimed. All six are nullable:true on both providers, but a nullable column does not produce a NULL row: five of the six were present at CreateTable, so a NULL there still needs code to write one, and on MySQL there is NO code-path-free NULL for any of the six. The write path ACCEPTS a null (SaveChanges succeeds, stores SQL NULL) but no caller supplies one today -- every production construction of the two commands goes through the request records. That is a property of the code, not a live caller; claiming otherwise would be the banned "it's AsNoTracking today" argument pointed the other way. #824 -- ElasticSearchIndex.UpdateSong had no regression test Issue option 1 (a non-network transport) shipped, and needed no new package: Elastic.Transport.InMemoryRequestInvoker is public in the pinned version and ElasticsearchClientSettings(NodePool, IRequestInvoker) accepts it, injected into the private _client the way #701 injects the Lucene IndexWriter. UpdateItems never runs `_client ??= CreateClient()`, so the injected instance is the one used. Two traps there are load-bearing, both measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client's product check throws UnsupportedProductException INTO UpdateSong's catch, and an empty body fails to deserialize the same way. Either turns the fixture into a green measurement of the error path -- which is how it first failed here, caught by the ThrowOnWarningLogger. The document id is asserted as the LAST PATH SEGMENT, not by substring: the index name carries digits, so ShouldContain would stop discriminating for a song whose id collided with one. Six mutations executed, each disarming ITS OWN clause alone: - `??=` restored in ElasticSearchIndex only -> the Elastic fixture reddens on "metadata.Artists should be null but was []" while the LUCENE fixture stays GREEN. The #824 hole demonstrated, not described. - DaysOfWeek guard disarmed in the selector -> 4 red, 3 green (DaysOfMonth and MonthsOfYear unaffected). Each clause is independently load-bearing. - DaysOfMonth guard disarmed in Playouts.Mapper -> 1 red, 2 green. - Elastic dropped from the covered set / mapped to the SAME fixture as Lucene / mapped to a class with no [Test] -> SearchIndexMutationCoverageTests reddens on each. That coverage guard is the boundary fix the issue asked for: the covered set is compared against an ISearchIndex population DERIVED FROM THE ASSEMBLY. Its claim stops where the check does -- no static check can establish that a named fixture actually DRIVES its indexer, so it forces a human to look rather than proving coverage. ThrowOnWarningLogger moved to ErsatzTV.Tests/Support so both fixtures share it; the Lucene fixture's assertions are otherwise untouched, since it is a witnessed proof artifact. No production change in ElasticSearchIndex.cs -- #824 is coverage only. Docs: testing.md gains a "Provider-parity fixtures" section naming all THREE opt-in-MySQL fixtures and recording that CI runs none of them (#627); docs/README.md gains the matching task signal; guard-inventory.md's hand-written C# guard list goes from five files to six. Scheduling/Mapper.cs loses the UTF-8 BOM it inherited, per #311 fix-as-you-touch. Local gate (with the MySQL lane armed): ErsatzTV.Tests 2096 passed / 0 skipped, Core.Tests 693/1, Infrastructure.Tests 114, Architecture.Tests 7, Scanner.Tests 1504 -- 0 failures in each. scripts/tests 1228 passed / 2 skipped. dotnet format whitespace --verify-no-changes clean; BOM check over the touched set with the population COUNT asserted, because a bare zsh loop silently checks one concatenated filename. decisions_validate OK. Fixes #823 Fixes #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- ErsatzTV.Application/Playouts/Mapper.cs | 13 +- ErsatzTV.Application/Scheduling/Mapper.cs | 15 +- .../AlternateScheduleSelectorTests.cs | 145 +++++++ .../Scheduling/AlternateScheduleSelector.cs | 33 +- .../RecurrenceLimitsMapperNullTests.cs | 120 ++++++ ...ElasticSongIndexerMetadataMutationTests.cs | 299 ++++++++++++++ .../SchedulingCollectionColumnNullTests.cs | 388 ++++++++++++++++++ .../SearchIndexMutationCoverageTests.cs | 89 ++++ .../SongIndexerMetadataMutationTests.cs | 22 - .../Support/ThrowOnWarningLogger.cs | 37 ++ docs/README.md | 1 + docs/decisions/README.md | 2 +- .../nullable-primitive-collection-mutation.md | 4 +- docs/guard-inventory.md | 5 +- docs/testing.md | 27 ++ 15 files changed, 1164 insertions(+), 36 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs create mode 100644 ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs create mode 100644 ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs create mode 100644 ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs create mode 100644 ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index c869aaddb..876b8a11d 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -1,5 +1,6 @@ using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Application.Playouts; @@ -40,9 +41,15 @@ internal static class Mapper programScheduleAlternate.Id, programScheduleAlternate.Index, programScheduleAlternate.ProgramScheduleId, - programScheduleAlternate.DaysOfWeek, - programScheduleAlternate.DaysOfMonth, - programScheduleAlternate.MonthsOfYear, + // ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the + // SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the + // SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts + // `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and + // throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the + // entity (`media.nullable-primitive-collection-mutation`). + programScheduleAlternate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + programScheduleAlternate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + programScheduleAlternate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), programScheduleAlternate.LimitToDateRange, programScheduleAlternate.StartMonth, programScheduleAlternate.StartDay, diff --git a/ErsatzTV.Application/Scheduling/Mapper.cs b/ErsatzTV.Application/Scheduling/Mapper.cs index 91e6356b5..4ae54728c 100644 --- a/ErsatzTV.Application/Scheduling/Mapper.cs +++ b/ErsatzTV.Application/Scheduling/Mapper.cs @@ -1,6 +1,7 @@ -using ErsatzTV.Application.Tree; +using ErsatzTV.Application.Tree; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Application.Scheduling; @@ -190,9 +191,15 @@ internal static class Mapper ProjectToViewModel(playoutTemplate.Template), ProjectToViewModel(playoutTemplate.DecoTemplate), playoutTemplate.Index, - playoutTemplate.DaysOfWeek, - playoutTemplate.DaysOfMonth, - playoutTemplate.MonthsOfYear, + // ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the + // SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the + // SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts + // `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and + // throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the + // entity (`media.nullable-primitive-collection-mutation`). + playoutTemplate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + playoutTemplate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + playoutTemplate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), playoutTemplate.LimitToDateRange, playoutTemplate.StartMonth, playoutTemplate.StartDay, diff --git a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs index 34af2a3f9..edeeb107e 100644 --- a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs @@ -1,3 +1,4 @@ +using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; using ErsatzTV.Core.Scheduling; using NUnit.Framework; @@ -864,4 +865,148 @@ public static class AlternateScheduleSelectorTests result.IsNone.ShouldBeFalse(); } } + + /// + /// ersatztv#823. DaysOfWeek, DaysOfMonth and MonthsOfYear on + /// and are six + /// single-column primitive collections whose columns are nullable: true on both providers. + /// A NULL column materializes as CLR null — EF does not invoke the value converter for a + /// NULL at all — so unguarded, each .Contains in + /// throws + /// . These tests are RED without the read-site guard. + /// + /// A null reads as UNRESTRICTED (the All*() sets), not as empty. The deciding case is + /// SQLite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth, which adds the column + /// nullable: true with NO default: a row inserted before it had no day-of-month + /// restriction, so reading its NULL as empty would INVERT its meaning and silently stop the + /// template applying. That is the one NULL reachable without any code writing one. + /// + /// + /// Reachability itself is pinned by + /// ErsatzTV.Tests.Integration.SchedulingCollectionColumnNullTests against a real + /// TvContext; these tests pin what the selector does once the null is there. + /// + /// + [TestFixture] + public class GetScheduleForDate_NullCollections + { + private static readonly TimeSpan Offset = TimeSpan.FromHours(-5); + + // A Wednesday in March, so no All*() member is coincidentally excluded. + private static readonly DateTimeOffset AnyDate = new(2024, 3, 6, 0, 0, 0, Offset); + + private static PlayoutTemplate Unrestricted() => + new() + { + DaysOfWeek = AlternateScheduleSelector.AllDaysOfWeek(), + DaysOfMonth = AlternateScheduleSelector.AllDaysOfMonth(), + MonthsOfYear = AlternateScheduleSelector.AllMonthsOfYear() + }; + + private static Option Select(params PlayoutTemplate[] templates) => + AlternateScheduleSelector.GetScheduleForDate(templates.ToList(), AnyDate); + + [Test] + public void Null_DaysOfWeek_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + + Select(template).IsSome.ShouldBeTrue( + "a NULL DaysOfWeek means no weekday restriction was recorded, so the template still applies"); + } + + [Test] + public void Null_DaysOfMonth_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfMonth = null!; + + Select(template).IsSome.ShouldBeTrue(); + } + + [Test] + public void Null_MonthsOfYear_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.MonthsOfYear = null!; + + Select(template).IsSome.ShouldBeTrue(); + } + + [Test] + public void All_Three_Null_On_ProgramScheduleAlternate_Reads_As_Unrestricted() + { + var alternate = new ProgramScheduleAlternate + { + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + AlternateScheduleSelector.GetScheduleForDate( + new List { alternate }, + AnyDate) + .IsSome.ShouldBeTrue(); + } + + /// + /// A null must not be confused with an explicitly EMPTY collection. Empty is a legal, reachable + /// state meaning "matches no day", and it keeps that meaning — which is exactly why a NULL + /// cannot be normalized to it. + /// + [Test] + public void An_Explicitly_Empty_Collection_Still_Matches_Nothing() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = []; + + Select(template).IsNone.ShouldBeTrue( + "an empty DaysOfWeek is a recorded restriction of NO days, unlike a NULL"); + } + + /// + /// The guard resolves per item, so a null on the FIRST item must not decide the second. + /// + [Test] + public void A_Null_Item_Does_Not_Disturb_Selection_Of_A_Later_Item() + { + PlayoutTemplate nulled = Unrestricted(); + nulled.DaysOfWeek = null!; + nulled.Index = 0; + + PlayoutTemplate second = Unrestricted(); + second.Index = 1; + + foreach (PlayoutTemplate selected in Select(nulled, second)) + { + // index 0 is unrestricted once its NULL is read, so it legitimately wins on ordering + selected.ShouldBeSameAs(nulled); + return; + } + + Assert.Fail("the null-collection item was skipped instead of read as unrestricted"); + } + + /// + /// The read-site guard must not be written BACK onto the item. These are single-column + /// primitive collections, so assigning the guard would flip a tracked entity to + /// Modified and the next SaveChanges would persist the substituted collection + /// over the NULL — the mechanism recorded as media.nullable-primitive-collection-mutation. + /// + [Test] + public void Guard_Must_Not_Be_Written_Back_Onto_The_Item() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + template.DaysOfMonth = null!; + template.MonthsOfYear = null!; + + Select(template); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + } } diff --git a/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs b/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs index c9ca6b162..672829d0b 100644 --- a/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs +++ b/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs @@ -85,19 +85,46 @@ public static class AlternateScheduleSelector } } - bool daysOfWeek = item.DaysOfWeek.Contains(date.DayOfWeek); + // These three are NULLABLE single-column primitive collections, and a runtime null IS + // reachable (ersatztv#823, measured against a real TvContext on SQLite and MySQL 8.4): EF does + // NOT invoke the value converter for a NULL column, so it materializes as CLR null rather than + // through IntCollectionValueConverter's null-to-empty branch, which never runs on this path. + // Unguarded, each .Contains below throws NullReferenceException. + // + // A NULL reads as UNRESTRICTED -- the All*() sets -- NOT as empty. This is the whole semantic + // question and it is decided by the one NULL that is reachable WITHOUT any code writing one: + // Sqlite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth adds DaysOfMonth with + // `nullable: true` and NO defaultValue, so a PlayoutTemplate row inserted before it holds NULL + // and, by construction, had NO day-of-month restriction. Reading that as empty would INVERT + // the row's meaning and silently stop the template applying at all. All*() preserves it, and + // it is how "no restriction recorded" is already represented elsewhere in this domain + // (GetPlayoutAlternateSchedulesHandler, PreviewBlockPlayoutHandler). Note what does NOT decide + // it: the API request records normalize an omitted field with `?? []`, but that is a client + // omitting a field on a WRITE and says nothing about what a legacy database NULL meant. + // + // Guarded at the READ SITE, into locals, and NEVER assigned back onto `item`: the property IS + // the column value, so writing the guard back would flip a tracked entry to Modified and + // persist the substituted collection over the NULL + // (`media.nullable-primitive-collection-mutation`). The matching substitution happens at the + // entity->DTO boundary in the two Mapper.ProjectToViewModel overloads, so the SPA's + // appliesToDate -- an exact port of this method -- previews what this actually schedules. + ICollection itemDaysOfWeek = item.DaysOfWeek ?? AllDaysOfWeek(); + ICollection itemDaysOfMonth = item.DaysOfMonth ?? AllDaysOfMonth(); + ICollection itemMonthsOfYear = item.MonthsOfYear ?? AllMonthsOfYear(); + + bool daysOfWeek = itemDaysOfWeek.Contains(date.DayOfWeek); if (!daysOfWeek) { continue; } - bool daysOfMonth = item.DaysOfMonth.Contains(date.Day); + bool daysOfMonth = itemDaysOfMonth.Contains(date.Day); if (!daysOfMonth) { continue; } - bool monthOfYear = item.MonthsOfYear.Contains(date.Month); + bool monthOfYear = itemMonthsOfYear.Contains(date.Month); if (monthOfYear) { return item; diff --git a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs new file mode 100644 index 000000000..4a9a85742 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs @@ -0,0 +1,120 @@ +using ErsatzTV.Application.Playouts; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; +using NUnit.Framework; +using Shouldly; +using PlayoutsMapper = ErsatzTV.Application.Playouts.Mapper; +using SchedulingMapper = ErsatzTV.Application.Scheduling.Mapper; + +namespace ErsatzTV.Tests.Application.Scheduling; + +/// +/// ersatztv#823. Guarding alone would have left the OTHER read +/// of the same six columns unguarded — the entity→view-model mappers, which feed +/// PlayoutController's response models and therefore the SPA. +/// +/// Two things break without the guard, and neither is a C# exception, which is why the selector +/// tests cannot see them. web/src/screens/PlayoutScheduleEditors.tsx spreads the collection +/// (daysOfMonth: [...template.daysOfMonth]) and throws TypeError: not iterable on a +/// JSON null; and web/src/screens/playoutTemplateCalendar.ts's appliesToDate — +/// an exact TypeScript port of — +/// calls .includes on it. +/// +/// +/// So the mappers substitute the SAME unrestricted defaults the selector reads. That agreement is +/// the point: a DTO that said "empty" while the selector scheduled "unrestricted" would make the +/// preview calendar disagree with the playout it is previewing. +/// +/// +[TestFixture] +public class RecurrenceLimitsMapperNullTests +{ + private static Template MinimalTemplate() => + new() + { + Id = 7, + Name = "T", + TemplateGroupId = 1, + TemplateGroup = new TemplateGroup { Name = "G" }, + Items = [] + }; + + [Test] + public void ProgramScheduleAlternate_Null_Collections_Map_To_Unrestricted() + { + var alternate = new ProgramScheduleAlternate + { + Id = 1, + Index = 0, + ProgramScheduleId = 2, + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate); + + vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek()); + vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth()); + vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear()); + + // Never assigned back: these are single-column primitive collections, so writing the guard onto a + // tracked entity would persist the substituted set over the NULL + // (media.nullable-primitive-collection-mutation). + alternate.DaysOfWeek.ShouldBeNull(); + alternate.DaysOfMonth.ShouldBeNull(); + alternate.MonthsOfYear.ShouldBeNull(); + } + + [Test] + public void PlayoutTemplate_Null_Collections_Map_To_Unrestricted() + { + var template = new PlayoutTemplate + { + Id = 1, + Index = 0, + Template = MinimalTemplate(), + DecoTemplate = null, + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + PlayoutTemplateViewModel vm = SchedulingMapper.ProjectToViewModel(template); + + vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek()); + vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth()); + vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear()); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + + /// + /// An explicitly EMPTY collection is a recorded restriction of no days and must survive the mapper + /// unchanged. Without this, a guard written as "empty or null becomes All*" would pass the two tests + /// above while silently rewriting real user data on the way out. + /// + [Test] + public void An_Explicitly_Empty_Collection_Is_Not_Rewritten() + { + var alternate = new ProgramScheduleAlternate + { + Id = 1, + Index = 0, + ProgramScheduleId = 2, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [] + }; + + PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate); + + vm.DaysOfWeek.ShouldBeEmpty(); + vm.DaysOfMonth.ShouldBeEmpty(); + vm.MonthsOfYear.ShouldBeEmpty(); + } +} diff --git a/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs b/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs new file mode 100644 index 000000000..1e82cb022 --- /dev/null +++ b/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs @@ -0,0 +1,299 @@ +using System.Reflection; +using System.Text; +using Elastic.Clients.Elasticsearch; +using Elastic.Transport; +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 Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#824 — the gap ersatztv#701 named rather than papered over. +/// +/// ElasticSearchIndex.UpdateSong holds an INDEPENDENT copy of the logic +/// pins on LuceneSearchIndex. #701 removed +/// metadata.AlbumArtists ??= []; metadata.Artists ??= []; from both, but only Lucene gained +/// a regression test — so reintroducing the mutation in the Elastic copy ALONE left the whole +/// suite green. This fixture closes that: the assertions are the same three, driven through the +/// real ElasticSearchIndex against a real . +/// +/// +/// Why a stubbed transport rather than a live server. #824 listed "inject a non-network +/// ElasticsearchClient transport" as option 1 and it is what shipped: +/// Elastic.Transport.InMemoryRequestInvoker is public in the pinned Elastic.Transport, and +/// ElasticsearchClientSettings(NodePool, IRequestInvoker) accepts it. No server, no socket, +/// no new package. +/// +/// +/// The canned response body is load-bearing, not decoration. A bare +/// InMemoryRequestInvoker() answers with an EMPTY body, which the client cannot deserialize +/// into an IndexResponse. That throw lands in UpdateSong's catch, which logs a +/// warning and assigns metadata.Song = null — so the fixture would measure the ERROR path +/// while every "did not mutate" assertion below still passed, vacuously. The +/// is the belt to that brace: it fails the test if the +/// catch ran at all. +/// +/// +/// The client is injected into the private _client field rather than obtained normally, +/// because CreateClient reads the process-wide static ElasticSearchIndex.Uri and +/// would open a real socket. UpdateItems — unlike IndexExists and +/// Initialize — never runs _client ??= CreateClient(), so the injected instance is +/// the one used and an uninjected one would simply be null. +/// +/// +[TestFixture] +[NonParallelizable] +public class ElasticSongIndexerMetadataMutationTests +{ + private const string TestIndexName = "etv-824-test"; + + private string? _originalIndexName; + + /// + /// ElasticSearchIndex.IndexName is a process-wide static. Only Startup reads it today, + /// so leaving it set leaks nothing that currently runs — but a static this fixture writes and never + /// restores is a cross-test hazard waiting for the first test that does read it. + /// + [SetUp] + public void SetUp() => _originalIndexName = ElasticSearchIndex.IndexName; + + [TearDown] + public void TearDown() => ElasticSearchIndex.IndexName = _originalIndexName; + + /// + /// A well-formed IndexResponse. See the fixture docstring: an empty body diverts the run + /// into UpdateSong's catch and makes every assertion below vacuous. + /// + private const string IndexResponseBody = + """ + {"_index":"etv-824-test","_id":"1","_version":1,"result":"created", + "_shards":{"total":1,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1} + """; + + [Test] + public async Task UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity() + { + await using var harness = await InMemoryTvContext.CreateAsync(); + + int metadataId; + int songId; + 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; + songId = song.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 -- the indexer's own contract is what is being pinned, not the + // AsNoTracking() habit of today's two callers. See SongIndexerMetadataMutationTests. + 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"); + + var logger = new ThrowOnWarningLogger(); + var index = new ElasticSearchIndex( + new SearchQueryParser( + Substitute.For(), + Substitute.For>()), + logger); + + var invoker = new CapturingRequestInvoker( + new InMemoryRequestInvoker( + Encoding.UTF8.GetBytes(IndexResponseBody), + 200, + exception: null, + contentType: "application/json", + // The X-Elastic-Product header is REQUIRED, not cosmetic. The client runs a product + // check on its first response and throws UnsupportedProductException ("the server is + // not a supported distribution of Elasticsearch") without it -- which lands in + // UpdateSong's catch and makes the fixture measure the error path. Measured: this is + // exactly how this fixture first failed. + headers: ProductCheckHeaders())); + + var settings = new ElasticsearchClientSettings( + new SingleNodePool(new Uri("http://localhost:9200")), + invoker) + .DefaultIndex(TestIndexName); + + ElasticSearchIndex.IndexName = TestIndexName; + + typeof(ElasticSearchIndex) + .GetField("_client", BindingFlags.NonPublic | BindingFlags.Instance)! + .SetValue(index, new ElasticsearchClient(settings)); + + // A bare substitute returns null from GetAllLanguageCodes, which NPEs inside AddLanguages and + // would divert the run into UpdateSong's catch. + var languageCodeService = Substitute.For(); + languageCodeService.GetAllLanguageCodes(Arg.Any>()).Returns([]); + languageCodeService.GetAllLanguageCodes(Arg.Any()).Returns([]); + + await index.UpdateItems( + Substitute.For(), + Substitute.For(), + languageCodeService, + [tracked]); + + // Surfacing the exception rather than asserting ShouldBeNull: the catch is the fixture's + // most likely failure mode (see the canned-response note above), and "expected null but was + // " without the message sends the next reader hunting for a cause the fixture + // already had in its hand. + if (logger.Failure is not null) + { + Assert.Fail("UpdateSong threw and its catch ran, so this probe measured the error path " + + $"rather than the indexing path: {logger.Failure}"); + } + + // POSITIVE CONTROL, and it is not optional: every assertion below says something did NOT + // happen, so all of them hold vacuously if UpdateSong never ran. The Lucene fixture uses + // `writer.NumDocs == 1` for exactly this; the transport-level equivalent is that the indexer + // actually issued the index request for THIS song. Like NumDocs, it proves the song-indexing + // path ran -- it does NOT prove the artist reads specifically ran. + invoker.Requests.Count.ShouldBe( + 1, + "UpdateSong did not issue exactly one index request, so the assertions below would pass " + + $"without exercising the code under test. Captured: [{string.Join(", ", invoker.Requests)}]"); + + // The document id is compared as the LAST PATH SEGMENT, not with ShouldContain. A substring + // test is a false-pass vector here: the index name itself carries digits ("etv-824-test"), so + // ShouldContain("2") or ShouldContain("4") would be satisfied by the index name alone for a + // song whose id happened to be 2 or 4, and the assertion would stop discriminating without + // ever failing. + string path = invoker.Requests[0].Split(' ')[^1].Split('?')[0]; + path.Split('/')[^1].ShouldBe( + songId.ToString(), + $"the index request was not for the seeded song. Captured: {invoker.Requests[0]}"); + + // 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 static Dictionary> ProductCheckHeaders() => + new(StringComparer.OrdinalIgnoreCase) { ["x-elastic-product"] = ["Elasticsearch"] }; + + /// + /// Records every request the client actually issues, so the fixture can prove the code under test + /// ran. Delegates the answering to a real rather than + /// hand-building a response. + /// + private sealed class CapturingRequestInvoker(InMemoryRequestInvoker inner) : IRequestInvoker + { + public List Requests { get; } = []; + + public ResponseFactory ResponseFactory => inner.ResponseFactory; + + public TResponse Request( + Endpoint endpoint, + BoundConfiguration boundConfiguration, + PostData? postData) + where TResponse : TransportResponse, new() + { + Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}"); + return inner.Request(endpoint, boundConfiguration, postData); + } + + public Task RequestAsync( + Endpoint endpoint, + BoundConfiguration boundConfiguration, + PostData? postData, + CancellationToken cancellationToken) + where TResponse : TransportResponse, new() + { + Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}"); + return inner.RequestAsync(endpoint, boundConfiguration, postData, cancellationToken); + } + + // IRequestInvoker extends IDisposable, but InMemoryRequestInvoker holds no disposable state and + // exposes no Dispose of its own -- there is nothing to forward to. + public void Dispose() + { + } + } + + 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/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs b/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs new file mode 100644 index 000000000..7ed8bb851 --- /dev/null +++ b/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs @@ -0,0 +1,388 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Infrastructure; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.MySql.Data; +using ErsatzTV.Infrastructure.Sqlite.Data; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using MySqlConnector; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#823 — the question ersatztv#701 split out rather than answered: can a runtime null reach +/// one of the SIX collection-valued scalar columns (DaysOfMonth, MonthsOfYear, +/// DaysOfWeek on and )? +/// +/// Measured, not reasoned about, because the reasoning available beforehand pointed the +/// wrong way. The two converters differ on their read side — +/// IntCollectionValueConverter maps null-or-blank to Array.Empty<int>() while +/// EnumCollectionJsonValueConverter would dereference the result of +/// JsonConvert.DeserializeObject — so the expectation was that the two behave differently +/// on a NULL row. They do not: EF does not invoke a value converter for a NULL column at +/// all, so all six materialize as CLR null and the int converter's null-to-empty +/// branch is dead on this path. That is the measurement this fixture pins. +/// +/// +/// It also pins the WRITE half: assigning null to one of the six and calling +/// SaveChanges SUCCEEDS and stores SQL NULL — the converter is skipped on the way out too. +/// State that precisely, because the overclaim is tempting: this is a property of the CODE, not a +/// live caller. NO caller supplies a null today — every production construction of +/// ReplacePlayoutAlternateSchedule / ReplacePlayoutTemplate goes through the HTTP +/// request records, which normalize with ?? []. What makes it a latent gun is that +/// ReplacePlayoutAlternateScheduleItemsHandler / ReplacePlayoutTemplateItemsHandler +/// assign the command value straight onto the entity, so nothing in the write path itself refuses. +/// +/// +/// The LEGACY route is narrower than "the columns are nullable", and conflating the two is the easy +/// error: all six are nullable: true on both providers, but five of the six were present at +/// CreateTable, so a NULL there still needs code to write one. EXACTLY ONE case is +/// code-path-free — SQLite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth is an +/// AddColumn with nullable: true and NO defaultValue, so PlayoutTemplate +/// rows inserted before it hold NULL, and by construction those rows had no day-of-month +/// restriction. On MySQL PlayoutTemplate arrived whole in +/// 20240114034944_Add_BlockScheduling, so there is no code-path-free NULL for any of the six +/// there. This fixture manufactures its NULL with a raw UPDATE, which is a code path — it +/// measures MATERIALIZATION, and the legacy route above is established by reading the migrations. +/// +/// +/// What the selector then does with it is pinned by +/// ErsatzTV.Core.Tests.Scheduling.AlternateScheduleSelectorTests.GetScheduleForDate_NullCollections: +/// unguarded, .Contains throws . +/// +/// +/// Runs against BOTH providers from ONE fixture body, because the question is about provider +/// materialization and a SQLite-only answer would not have settled it. MySQL needs a live server, +/// supplied via ETV_TEST_MYSQL_CONNECTION; without it the MySQL fixture ignores — a +/// visible skip, never a silent pass. ETV_REQUIRE_MYSQL_TESTS turns that skip into a hard +/// failure for a runner that is supposed to have one. This mirrors +/// , which established the pattern. +/// +/// +/// Cost, stated. Each test migrates a FRESH database (per-test isolation by construction — +/// ersatztv#491 measured what a shared database and a wipe-that-must-succeed cost). Replaying every +/// migration is the expensive part and it is deliberate: a reachability fixture should stand on the +/// SHIPPED schema, not on one EnsureCreated builds from the current model. +/// +/// +[TestFixture(TestProvider.Sqlite)] +[TestFixture(TestProvider.MySql)] +[NonParallelizable] +public class SchedulingCollectionColumnNullTests(TestProvider provider) +{ + private const string MySqlConnectionVariable = "ETV_TEST_MYSQL_CONNECTION"; + private const string MySqlRequiredVariable = "ETV_REQUIRE_MYSQL_TESTS"; + + private string _databasePath = null!; + private string? _mySqlConnectionString; + private DbContextOptions _options = null!; + + private string _originalLastInsertedRowId = null!; + private string _originalCollation = null!; + private bool _originalIsSqlite; + private Func _originalUniqueViolation = null!; + + /// + /// TvContext's provider statics are process-wide, and the MySQL arm sets them to MySQL + /// values. Restoring them is NOT belt-and-braces: — the harness + /// most of this suite uses — resets only three of the five (IsSqlite, + /// IsUniqueConstraintViolation, RegisterUnicodeCaseFunctions) and leaves + /// LastInsertedRowId and CaseInsensitiveCollation alone. So without this, a MySQL arm + /// running before a SQLite test leaves last_insert_id() in place for a SQLite connection, + /// which is an order-dependent failure in a fixture that never mentions MySQL. + /// [NonParallelizable] serialises execution; it does not restore state. + /// + [SetUp] + public async Task SetUp() + { + _originalLastInsertedRowId = TvContext.LastInsertedRowId; + _originalCollation = TvContext.CaseInsensitiveCollation; + _originalIsSqlite = TvContext.IsSqlite; + _originalUniqueViolation = TvContext.IsUniqueConstraintViolation; + + if (provider is TestProvider.Sqlite) + { + TvContext.IsSqlite = true; + TvContext.LastInsertedRowId = "last_insert_rowid()"; + TvContext.CaseInsensitiveCollation = "NOCASE"; + TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation; + + _databasePath = Path.Combine(Path.GetTempPath(), $"etv823-{Guid.NewGuid():N}.sqlite3"); + _options = new DbContextOptionsBuilder() + .UseSqlite( + $"Data Source={_databasePath};Foreign Keys=False", + o => o.MigrationsAssembly("ErsatzTV.Infrastructure.Sqlite")) + .Options; + + await using TvContext sqlite = Create(_options); + await sqlite.Database.MigrateAsync(); + return; + } + + string? baseConnectionString = Environment.GetEnvironmentVariable(MySqlConnectionVariable); + if (string.IsNullOrWhiteSpace(baseConnectionString)) + { + string message = + $"{MySqlConnectionVariable} is not set, so the MySql half of the #823 measurement cannot " + + "run. Whether a NULL column materializes as CLR null is a provider question, so a " + + "SQLite-only answer does not settle it."; + + if (IsTrue(Environment.GetEnvironmentVariable(MySqlRequiredVariable))) + { + Assert.Fail($"{message} {MySqlRequiredVariable} is set, so this is a failure, not a skip."); + } + + Assert.Ignore($"{message} Set it to run this locally."); + } + + // A database of our own, and a FRESH one per test: isolation by construction, per #491's finding + // that a shared name trades isolation for a wipe that has to succeed. + _mySqlConnectionString = + new MySqlConnectionStringBuilder(baseConnectionString) { Database = $"etv823_{Guid.NewGuid():N}" } + .ConnectionString; + + TvContext.IsSqlite = false; + TvContext.LastInsertedRowId = "last_insert_id()"; + TvContext.CaseInsensitiveCollation = "utf8mb4_general_ci"; + TvContext.IsUniqueConstraintViolation = MySqlErrorClassifier.IsUniqueConstraintViolation; + + ServerVersion serverVersion = ServerVersion.AutoDetect(_mySqlConnectionString); + _options = new DbContextOptionsBuilder() + .UseMySql( + _mySqlConnectionString, + serverVersion, + o => o.MigrationsAssembly("ErsatzTV.Infrastructure.MySql")) + .Options; + + // NUnit does not run [TearDown] when [SetUp] throws, and by this point the database exists — the + // migration itself created it. Without this, a migration that fails part way strands a database + // and its connection pool on a SHARED server, once per attempt. + try + { + await using TvContext mysql = Create(_options); + await mysql.Database.MigrateAsync(); + } + catch + { + await DropMySqlDatabase(); + _mySqlConnectionString = null; + throw; + } + } + + private async Task DropMySqlDatabase() + { + try + { + await using (TvContext context = Create(_options)) + { + await context.Database.EnsureDeletedAsync(); + } + + await using var probe = new MySqlConnection(_mySqlConnectionString); + await MySqlConnection.ClearPoolAsync(probe); + } + catch (Exception ex) + { + // Never mask the original failure with a cleanup failure, and never fail a PASSING test in + // teardown because the server went away -- but say so, because a silent cleanup failure is how + // a leak becomes invisible. + await TestContext.Out.WriteLineAsync( + $"WARNING: could not drop the MySql test database {_mySqlConnectionString}: {ex.Message}"); + } + } + + [TearDown] + public async Task TearDown() + { + TvContext.LastInsertedRowId = _originalLastInsertedRowId; + TvContext.CaseInsensitiveCollation = _originalCollation; + TvContext.IsSqlite = _originalIsSqlite; + TvContext.IsUniqueConstraintViolation = _originalUniqueViolation; + + if (provider is TestProvider.Sqlite) + { + Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); + foreach (string path in new[] { _databasePath, $"{_databasePath}-wal", $"{_databasePath}-shm" }) + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + + return; + } + + if (_mySqlConnectionString is not null) + { + await DropMySqlDatabase(); + _mySqlConnectionString = null; + } + } + + /// + /// A row whose three columns are NULL materializes as CLR null on every one of them — including + /// the two IntCollectionValueConverter columns, whose converter would have produced + /// Array.Empty<int>() had it been invoked. + /// + [Test] + public async Task A_Null_Column_Materializes_As_Clr_Null_On_All_Six() + { + await using (TvContext context = Create(_options)) + { + await DisableForeignKeys(context); + + context.ProgramScheduleAlternates.Add(NewAlternate(playoutId: 1)); + context.PlayoutTemplates.Add(NewTemplate(playoutId: 1)); + await context.SaveChangesAsync(); + + // Out of band, so the row is byte-identical to a legacy one. This UPDATE is itself a code + // path and does NOT demonstrate the legacy route -- see the class docstring for which single + // (column, provider) case is genuinely code-path-free. What is being measured here is what EF + // MATERIALIZES from such a row, which is the same regardless of how the NULL got there. + await context.Database.ExecuteSqlRawAsync( + "UPDATE ProgramScheduleAlternate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL"); + await context.Database.ExecuteSqlRawAsync( + "UPDATE PlayoutTemplate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL"); + } + + // ANTI-VACUITY: a read that found no row would leave every ShouldBeNull below trivially true, + // so both reads are Single and would throw on an empty table. + await using (TvContext context = Create(_options)) + { + ProgramScheduleAlternate alternate = + await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync(); + + alternate.DaysOfWeek.ShouldBeNull( + "EnumCollectionJsonValueConverter must not be invoked for a NULL column"); + alternate.DaysOfMonth.ShouldBeNull( + "IntCollectionValueConverter's null-to-empty branch must not run — EF skips the " + + "converter for a NULL column, so this is null rather than an empty array"); + alternate.MonthsOfYear.ShouldBeNull(); + + PlayoutTemplate template = await context.PlayoutTemplates.AsNoTracking().SingleAsync(); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + } + + /// + /// The write half. A caller that hands one of the six a null — which the two Replace* handlers do + /// verbatim from their command, and only the HTTP request records guard against — gets a stored + /// SQL NULL and no error at all. + /// + [Test] + public async Task Assigning_Null_Persists_Sql_Null_Rather_Than_Throwing() + { + await using (TvContext context = Create(_options)) + { + await DisableForeignKeys(context); + + ProgramScheduleAlternate alternate = NewAlternate(playoutId: 2); + alternate.DaysOfWeek = null!; + alternate.DaysOfMonth = null!; + alternate.MonthsOfYear = null!; + + context.ProgramScheduleAlternates.Add(alternate); + + // Not Should.NotThrow: the point is that this is the SHIPPED behaviour of the write path, so + // the assertion is that the round-trip below finds NULL, not merely that nothing blew up. + await context.SaveChangesAsync(); + } + + await using (TvContext context = Create(_options)) + { + ProgramScheduleAlternate reloaded = + await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync(); + + reloaded.DaysOfWeek.ShouldBeNull(); + reloaded.DaysOfMonth.ShouldBeNull(); + reloaded.MonthsOfYear.ShouldBeNull(); + } + + // ...and it really is SQL NULL in the column, not an empty string the converter round-trips. + // ExecuteScalar hands back CLR null both for a NULL column and for NO SUCH ROW, so the row is + // counted first — otherwise a fixture that silently deleted its row would report the same thing. + await using (TvContext context = Create(_options)) + { + (await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate")) + .ShouldNotBeNull(); + Convert.ToInt32(await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate")) + .ShouldBe(1, "the row is gone, so a null read below would prove nothing"); + + (await ScalarAsync(context, "SELECT DaysOfWeek FROM ProgramScheduleAlternate")).ShouldBeNull(); + (await ScalarAsync(context, "SELECT DaysOfMonth FROM ProgramScheduleAlternate")).ShouldBeNull(); + (await ScalarAsync(context, "SELECT MonthsOfYear FROM ProgramScheduleAlternate")).ShouldBeNull(); + } + } + + private static ProgramScheduleAlternate NewAlternate(int playoutId) => + new() + { + PlayoutId = playoutId, + ProgramScheduleId = 1, + Index = 0, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [], + StartMonth = 1, + StartDay = 1, + EndMonth = 12, + EndDay = 31 + }; + + private static PlayoutTemplate NewTemplate(int playoutId) => + new() + { + PlayoutId = playoutId, + TemplateId = 1, + Index = 0, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [], + StartMonth = 1, + StartDay = 1, + EndMonth = 12, + EndDay = 31 + }; + + /// + /// The rows here are deliberately partial graphs (a PlayoutId pointing at no Playout), so + /// foreign keys are off. SQLite takes it as a connection-string keyword; MySQL's + /// foreign_key_checks is a SESSION variable, so it is set on the context's own connection + /// and lives as long as that context does. + /// + private async Task DisableForeignKeys(TvContext context) + { + if (provider is TestProvider.MySql) + { + await context.Database.OpenConnectionAsync(); + await context.Database.ExecuteSqlRawAsync("SET SESSION foreign_key_checks = 0"); + } + } + + private static async Task ScalarAsync(TvContext context, string sql) + { + await context.Database.OpenConnectionAsync(); + await using System.Data.Common.DbCommand command = context.Database.GetDbConnection().CreateCommand(); + command.CommandText = sql; + object? value = await command.ExecuteScalarAsync(); + return value is DBNull ? null : value; + } + + private static bool IsTrue(string? value) => + !string.IsNullOrWhiteSpace(value) + && (value == "1" || value.Equals("true", StringComparison.OrdinalIgnoreCase)); + + private static TvContext Create(DbContextOptions options) => + new( + options, + NullLoggerFactory.Instance, + new SlowQueryInterceptor(NullLogger.Instance)); +} diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs new file mode 100644 index 000000000..d4a6afefb --- /dev/null +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -0,0 +1,89 @@ +using ErsatzTV.Core.Interfaces.Search; +using ErsatzTV.Infrastructure.Search; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#824. The defect that produced #824 was not that ElasticSearchIndex was hard to +/// test — it was that NOTHING NOTICED it had no cover. #701 fixed two independent copies of the same +/// UpdateSong logic and pinned one; the suite stayed green, and the gap survived on a +/// hand-written list of what had been covered (namely, one entry). +/// +/// So the covered set is compared against a population DERIVED FROM THE ASSEMBLY rather than +/// restated: a third implementation reddens this test until it is +/// given a mutation fixture of its own. That is +/// testing.guard-derives-population-from-source applied to a test population instead of a +/// file population. +/// +/// +/// Scope, stated rather than implied. The derivation is over the assembly that declares +/// both indexers (ErsatzTV.Infrastructure). An implementation added in a DIFFERENT +/// assembly is outside what this sees — it is not covered and this test cannot say so. Both +/// implementations have lived here since the interface existed, so the narrower scope buys a +/// guard that cannot be defeated by an unrelated assembly load order; widening it to every +/// loaded assembly would be the false-confidence version of the same check. +/// +/// +[TestFixture] +public class SearchIndexMutationCoverageTests +{ + /// + /// The indexers whose UpdateSong is pinned against the ersatztv#701 mutation. Referenced by + /// TYPE, so renaming an indexer or deleting a fixture is a compile error rather than a silent + /// divergence. + /// + private static readonly Dictionary CoveredBy = new() + { + [typeof(LuceneSearchIndex)] = typeof(SongIndexerMetadataMutationTests), + [typeof(ElasticSearchIndex)] = typeof(ElasticSongIndexerMetadataMutationTests) + }; + + [Test] + public void Every_ISearchIndex_Implementation_Has_A_Metadata_Mutation_Fixture() + { + List implementations = typeof(LuceneSearchIndex).Assembly + .GetTypes() + .Where(t => t is { IsAbstract: false, IsInterface: false }) + .Where(t => typeof(ISearchIndex).IsAssignableFrom(t)) + .OrderBy(t => t.FullName, StringComparer.Ordinal) + .ToList(); + + // The set comparison below would ALREADY fail on an empty derivation, because the expected side + // is non-empty -- so this floor is not load-bearing for correctness and saying it is would be a + // false claim about a check. It is a DIAGNOSTIC: it separates "the reflection stopped finding + // types" (a moved type, a renamed interface) from "someone added an indexer", which the set + // comparison alone reports identically. + implementations.Count.ShouldBeGreaterThanOrEqualTo( + 2, + "the ISearchIndex population derivation found almost nothing -- this is the derivation " + + "breaking, not an indexer being added"); + + implementations.ShouldBe( + CoveredBy.Keys.OrderBy(t => t.FullName, StringComparer.Ordinal), + ignoreOrder: false, + "every ISearchIndex implementation needs an UpdateSong metadata-mutation fixture — see " + + "ersatztv#824, where a second copy of the same logic went uncovered and an Elastic-only " + + "reintroduction of `metadata.Artists ??= []` left the whole suite green"); + + // Comparing the KEYS alone would leave the mapping half-checked: a third indexer could be pointed + // at an EXISTING fixture, or at a fixture class holding no runnable test, and the set comparison + // above would still pass. Both are closed here. What NO static check can establish is that the + // named fixture actually DRIVES its indexer -- that is stated as a residual on this guard rather + // than implied away, and it is why the record claims a third implementation cannot be added + // WITHOUT NOTICE, not that it cannot be mis-covered. + CoveredBy.Values.Distinct().Count().ShouldBe( + CoveredBy.Count, + "two indexers are mapped to the SAME fixture, so one of them is not actually covered"); + + foreach ((Type indexer, Type fixture) in CoveredBy) + { + fixture.GetMethods() + .Any(m => m.GetCustomAttributes(typeof(TestAttribute), inherit: true).Length > 0) + .ShouldBeTrue( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but declares no " + + "[Test] method, so it proves nothing"); + } + } +} diff --git a/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs index b22b43662..b409722d4 100644 --- a/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs +++ b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs @@ -186,28 +186,6 @@ public class SongIndexerMetadataMutationTests (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(); diff --git a/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs b/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs new file mode 100644 index 000000000..a58e20867 --- /dev/null +++ b/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs @@ -0,0 +1,37 @@ +using Microsoft.Extensions.Logging; + +namespace ErsatzTV.Tests.Support; + +/// +/// Captures the first warning-or-worse a component logs, so a fixture can fail on it instead of +/// silently measuring an error path. +/// +/// Both search indexers wrap each Update* body in a catch that logs a warning and +/// assigns metadata.Song = null — which severs a required relationship and cascades the +/// metadata row to . A fixture +/// that let that catch run quietly would report the wrong cause for every assertion after it, and +/// on the first run of the ersatztv#701 probe it did exactly that (a bare +/// ILanguageCodeService substitute NPEs inside AddLanguages). +/// +/// +public 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)); + } + } +} diff --git a/docs/README.md b/docs/README.md index 795608ce1..9b57cbcb9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -39,6 +39,7 @@ doc below, or that changes which sections a task signal points to.** | Live local run / Playwright-MCP verification | `docs/e2e-local.md` + `scripts/e2e-local.sh` | | Adding/changing a UI-E2E browser flow | `docs/e2e-local.md` → "UI-E2E harness" + `scripts/e2e-ui.sh` | | What does a test suite cover | `docs/testing.md` | +| Writing a test whose behaviour is PROVIDER-SPECIFIC (collation, a value converter, data-migration DML) | `docs/testing.md` → "Provider-parity fixtures (opt-in MySQL)" — run one fixture body against both providers via `ETV_TEST_MYSQL_CONNECTION`; without it the MySQL arm `Assert.Ignore`s visibly, and CI does not currently run it (ersatztv#627) | | Legacy Blazor route lookup | `docs/blazor-route-parity.md` (historical #91 phase (b) inventory) | | "Why do we do X this way" / challenging a convention | **Catalog-first**: `docs/decisions/README.md` (active rows) → follow the row's link to `docs/decisions/records//.md` for full rationale. `docs/decisions/archive//` only for "what did the rule used to be." | diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 85dd76ade..2868693a3 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,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.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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API's `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as 'applies on every date' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. RESIDUAL, stated rather than argued away: the substitution is still SILENT, so a legacy NULL row changes from 'playout build throws' to 'template applies' with nothing logged; the static, hot-loop selector has nowhere to log from. Revisit if a real NULL row is ever observed. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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 index 5989a0593..1ff28edcc 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,9 +5,9 @@ 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.' +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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API''s `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as ''applies on every date'' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. RESIDUAL, stated rather than argued away: the substitution is still SILENT, so a legacy NULL row changes from ''playout build throws'' to ''template applies'' with nothing logged; the static, hot-loop selector has nowhere to log from. Revisit if a real NULL row is ever observed. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.' +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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that declares no `[Test]`. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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` diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 443b2dc8c..46344fe3f 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -64,7 +64,10 @@ with have sat in these gaps. Treat anything not named here as unassessed, not as the check proves each job was classified, never that the classification is CORRECT; and an `inline` assertion cannot be proven to work without running the job, so those rows carry `Proof: NONE` honestly instead of borrowing a neighbouring file's proof. -2. **C# and TypeScript guards** — five files, none with a row: +2. **C# and TypeScript guards** — six files, none with a row: + `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs` (ersatztv#824 — derives the + `ISearchIndex` population from the declaring assembly and asserts set equality against a + hand-written covered set), `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `web/src/api/pageSizeCallSites.guard.test.ts`, `web/src/api/completeRequest.guard.test.ts`, and the pair `web/vite-plugins/trackedSourceFiles.test.ts` + its `.realgit.test.ts` sibling, which together diff --git a/docs/testing.md b/docs/testing.md index ee1a6bfcb..0d93d1685 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -116,6 +116,33 @@ UI-E2E (needs a built solution + built SPA; boots and tears down its own instanc scripts/e2e-ui.sh # from the repo root, NOT web/ ``` +## Provider-parity fixtures (opt-in MySQL) + +Most of `ErsatzTV.Tests` runs on the in-memory SQLite harness described above. A few fixtures in +`ErsatzTV.Tests/Integration/` instead drive a **real, migrated** database, and run the SAME body against +**both** providers because the behaviour they pin is provider-specific: + +| Fixture | What is provider-specific about it | +| --- | --- | +| `LibraryFolderDedupeMigrationTests` | the #491 dedupe DML — two MySQL-only collation defects (case-insensitive grouping, then `PAD SPACE`) were unreachable from SQLite | +| `SchedulingCollectionColumnNullTests` | what a NULL column materializes as through a value converter (ersatztv#823) | +| `SearchFieldValuesProviderTests` | the search-field-values query shape, which differs per provider | + +The MySQL half needs a live server, supplied as `ETV_TEST_MYSQL_CONNECTION`. **Without it these +fixtures `Assert.Ignore` — a visible skip, never a silent pass**, so an ordinary local run needs no +MySQL. Setting `ETV_REQUIRE_MYSQL_TESTS=1` turns that skip into a hard failure, for a runner that is +supposed to have one. + +```bash +ETV_TEST_MYSQL_CONNECTION='Server=;Port=3306;Uid=root;Pwd=;DefaultCommandTimeout=300;' \ + dotnet test ErsatzTV.Tests --filter FullyQualifiedName~SchedulingCollectionColumnNullTests +``` + +Each test uses a database name it generates per run, so isolation does not depend on a wipe +succeeding, and drops it in teardown. **CI does not currently run any of these MySQL halves** — the +`migrations` job spins a `mysql:8.4` service but only applies migrations to a fresh EMPTY database, +so it executes no data rows; re-arming these fixtures there is tracked by ersatztv#627. + ## Per-PR verification gate Before opening a PR: build the solution, run `ErsatzTV.Tests` + `ErsatzTV.Core.Tests` (plus -- 2.47.3 From 62007139658429a5ae409c3cf0d800b904da6b21 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 20:27:33 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(823,824):=20close=20the=20review=20roun?= =?UTF-8?q?d's=20findings=20=E2=80=94=20a=20discriminating=20control,=20th?= =?UTF-8?q?e=20second=20mapper's=20empty=20case,=20and=20honest=20test=20d?= =?UTF-8?q?etection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up commit (the branch is pushed, so not an amend). Two more cold reviews landed on the previous head; both reported 0 Blocker and 0 High, and these are their Mediums and Lows. Each fix carries its own witnessed mutation. 1. The selector fixture could not tell the fix from a much broader one. Every null test set a NULL and expected the item SELECTED, so all of them pass equally under "NULL means unrestricted" and under "any NULL makes this item match unconditionally" -- a refactor short-circuiting the whole date check on any null kept them green. Added the discriminating control: a NULL DaysOfWeek paired with MonthsOfYear = [1] against a MARCH date must be None. Only the narrow reading passes. 2. A_Null_Item_Does_Not_Disturb_Selection_Of_A_Later_Item never measured its own docstring. The nulled item was unrestricted and at Index 0, so it always won and the second item was never evaluated -- the stated invariant ("a null on the first item must not decide the second") went unmeasured while the test passed. Split into two: one where the nulled item genuinely does not match, which measures that the loop CONTINUES; and one that pins the index-order win separately. 3. The empty-preservation control existed for one of two identical mappers. The anti-mutant test for "empty or null becomes All*" covered only Playouts.Mapper; Scheduling.Mapper is a byte-identical triple in another file and had none, so a defensive edit to it alone would have rewritten a deliberately-empty user selection to 1..31 with the suite green. That is the one-helper-two-callers shape this repo has been bitten by. Added the matching test. 4. The coverage guard's [Test] clause did not check what its message claimed. GetMethods() without BindingFlags returns INHERITED methods, so a fixture that merely subclasses another satisfied it while driving the wrong indexer -- and Values.Distinct() cannot catch that, since the two Types differ. It also matched TestAttribute alone, so a future fixture written as [TestCase] would have falsely reddened, and it accepted an [Explicit]/[Ignore]d fixture that never runs, which is the "wired is not running" failure the guard exists to prevent. Now DeclaredOnly, the full test-method vocabulary, and Explicit/Ignore rejected at both method and fixture level. 5. Three residuals recorded on media.nullable-primitive-collection-mutation that the previous head asserted nothing about: - the LOUDNESS change, worst for an all-three-NULL ProgramScheduleAlternate, which now matches unconditionally and shadows the default schedule where it previously threw. Unreachable today, and a choice over an unreachable state rather than a measured requirement -- said plainly. - the normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row persists All*() over EVERY NULL row in that playout, and afterwards "the operator selected all 31" and "this is a legacy row" are indistinguishable. An ordinary user action closes that door. - the WRITE side disagrees with the READ side about what ABSENCE means: an omitted daysOfWeek normalizes to [] ("never applies") while a NULL column reads as unrestricted, so an API client gets HTTP 200 and a row that silently never fires. Filed as #880 rather than folded in here, because a client omitting a field on a write is a different question from what a legacy NULL meant. Two more mutations executed, both witnessed: - DaysOfWeek guard disarmed in Scheduling.Mapper -> 1 red, 3 green. - A fixture with no DECLARED test named in the covered set -> coverage red. Local gate (MySQL lane armed): ErsatzTV.Tests 2097 passed / 0 skipped, Core.Tests 695/1 -- 0 failures. Format clean, no BOM on the touched set with the population count asserted. decisions_validate OK. Refs #823 Refs #824 Refs #880 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .../AlternateScheduleSelectorTests.cs | 54 +++++++++++++++++-- .../RecurrenceLimitsMapperNullTests.cs | 28 ++++++++++ .../SearchIndexMutationCoverageTests.cs | 29 ++++++++-- docs/decisions/README.md | 2 +- .../nullable-primitive-collection-mutation.md | 2 +- 5 files changed, 105 insertions(+), 10 deletions(-) diff --git a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs index edeeb107e..bce3353e3 100644 --- a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs @@ -950,6 +950,27 @@ public static class AlternateScheduleSelectorTests .IsSome.ShouldBeTrue(); } + /// + /// THE DISCRIMINATING CONTROL. Every test above sets a NULL and expects the item to be selected, + /// so all of them pass equally under "NULL means unrestricted" and under the much broader + /// "any NULL makes this item match unconditionally" — a refactor that short-circuits the whole + /// date check when any dimension is null keeps them green. Here the nulled dimension is paired + /// with a RESTRICTIVE non-null one that the date fails, so only the narrow reading passes. + /// + [Test] + public void A_Null_Dimension_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + + // AnyDate is in MARCH; restrict to January only. + template.MonthsOfYear = [1]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL DaysOfWeek relaxes ONLY the weekday dimension — the January restriction still " + + "excludes a March date"); + } + /// /// A null must not be confused with an explicitly EMPTY collection. Empty is a legal, reachable /// state meaning "matches no day", and it keeps that meaning — which is exactly why a NULL @@ -966,10 +987,38 @@ public static class AlternateScheduleSelectorTests } /// - /// The guard resolves per item, so a null on the FIRST item must not decide the second. + /// The guard resolves PER ITEM: a null on the first item must not decide the second. Making the + /// nulled item genuinely non-matching is what measures that — with an unrestricted nulled item + /// at index 0 it simply wins on ordering and the second item is never evaluated, so the + /// invariant would go unmeasured while the test passed. /// [Test] - public void A_Null_Item_Does_Not_Disturb_Selection_Of_A_Later_Item() + public void A_Null_On_One_Item_Does_Not_Decide_A_Later_Item() + { + PlayoutTemplate nulled = Unrestricted(); + nulled.DaysOfWeek = null!; + nulled.MonthsOfYear = [1]; // AnyDate is in March, so this item must NOT match + nulled.Index = 0; + + PlayoutTemplate second = Unrestricted(); + second.Index = 1; + + foreach (PlayoutTemplate selected in Select(nulled, second)) + { + selected.ShouldBeSameAs(second); + return; + } + + Assert.Fail("the loop stopped at the null-collection item instead of continuing to the next"); + } + + /// + /// ...and when the nulled item IS unrestricted it legitimately wins on ordering. Paired with the + /// test above so "index 0 wins" and "the loop continues past a non-matching null item" are + /// separately pinned. + /// + [Test] + public void An_Unrestricted_Null_Item_Wins_On_Index_Order() { PlayoutTemplate nulled = Unrestricted(); nulled.DaysOfWeek = null!; @@ -980,7 +1029,6 @@ public static class AlternateScheduleSelectorTests foreach (PlayoutTemplate selected in Select(nulled, second)) { - // index 0 is unrestricted once its NULL is read, so it legitimately wins on ordering selected.ShouldBeSameAs(nulled); return; } diff --git a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs index 4a9a85742..4000be479 100644 --- a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs @@ -97,8 +97,36 @@ public class RecurrenceLimitsMapperNullTests /// An explicitly EMPTY collection is a recorded restriction of no days and must survive the mapper /// unchanged. Without this, a guard written as "empty or null becomes All*" would pass the two tests /// above while silently rewriting real user data on the way out. + /// + /// There is one of these per MAPPER, not one in total. The two overloads are byte-identical + /// triples in different files, so a defensive edit to one alone is exactly the "one helper, two + /// callers" shape this repo has been bitten by: covering only the Playouts mapper would leave + /// the PlayoutTemplate one free to acquire an `empty-or-null` guard with the suite still green. + /// /// [Test] + public void An_Explicitly_Empty_Collection_Is_Not_Rewritten_By_The_PlayoutTemplate_Mapper() + { + var template = new PlayoutTemplate + { + Id = 1, + Index = 0, + Template = MinimalTemplate(), + DecoTemplate = null, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [] + }; + + PlayoutTemplateViewModel vm = SchedulingMapper.ProjectToViewModel(template); + + vm.DaysOfWeek.ShouldBeEmpty(); + vm.DaysOfMonth.ShouldBeEmpty(); + vm.MonthsOfYear.ShouldBeEmpty(); + } + + /// + [Test] public void An_Explicitly_Empty_Collection_Is_Not_Rewritten() { var alternate = new ProgramScheduleAlternate diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs index d4a6afefb..aefc3e24c 100644 --- a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -1,3 +1,4 @@ +using System.Reflection; using ErsatzTV.Core.Interfaces.Search; using ErsatzTV.Infrastructure.Search; using NUnit.Framework; @@ -79,11 +80,29 @@ public class SearchIndexMutationCoverageTests foreach ((Type indexer, Type fixture) in CoveredBy) { - fixture.GetMethods() - .Any(m => m.GetCustomAttributes(typeof(TestAttribute), inherit: true).Length > 0) - .ShouldBeTrue( - $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but declares no " - + "[Test] method, so it proves nothing"); + // DeclaredOnly, and the whole NUnit test-method vocabulary rather than [Test] alone. Both + // narrowings are load-bearing: without DeclaredOnly a fixture that merely SUBCLASSES another + // one inherits its [Test] and satisfies this while driving the wrong indexer (and Values + // .Distinct() cannot catch that, since the two Types differ); and matching only TestAttribute + // would falsely redden a future fixture written entirely as [TestCase]/[TestCaseSource]. + // [Explicit] and [Ignore] are rejected because such a fixture never runs in CI, which is the + // same "wired is not running" failure the guard exists to prevent. + bool runnable = fixture + .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) + .Any(m => + m.GetCustomAttributes(inherit: true).Any(a => + a is TestAttribute or TestCaseAttribute or TestCaseSourceAttribute) + && m.GetCustomAttributes(inherit: true).All(a => a is not (ExplicitAttribute or IgnoreAttribute))); + + bool fixtureRuns = fixture.GetCustomAttributes(inherit: true) + .All(a => a is not (ExplicitAttribute or IgnoreAttribute)); + + runnable.ShouldBeTrue( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but DECLARES no " + + "runnable test method of its own, so it proves nothing"); + fixtureRuns.ShouldBeTrue( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is [Explicit] or " + + "[Ignore]d, so it never runs and proves nothing"); } } } diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 2868693a3..81d6637d9 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API's `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as 'applies on every date' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. RESIDUAL, stated rather than argued away: the substitution is still SILENT, so a legacy NULL row changes from 'playout build throws' to 'template applies' with nothing logged; the static, hot-loop selector has nowhere to log from. Revisit if a real NULL row is ever observed. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API's `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as 'applies on every date' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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 index 1ff28edcc..17e0abd71 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,7 +5,7 @@ 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API''s `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as ''applies on every date'' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. RESIDUAL, stated rather than argued away: the substitution is still SILENT, so a legacy NULL row changes from ''playout build throws'' to ''template applies'' with nothing logged; the static, hot-loop selector has nowhere to log from. Revisit if a real NULL row is ever observed. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.' +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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API''s `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as ''applies on every date'' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that declares no `[Test]`. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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.' --- -- 2.47.3 From ea888011aa49acb4714688be64e2a81752e6fb9d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 20:43:07 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(823):=20the=20decision=20record=20argue?= =?UTF-8?q?d=20BOTH=20readings=20=E2=80=94=20and=20the=20per-dimension=20m?= =?UTF-8?q?utant=20that=20survived=20the=20fixture?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-three review findings. One HIGH, and it was in the durable artifact rather than the code. THE HIGH: the record stated the shipped reading and its inverse. The semantic reversal (empty -> unrestricted) rewrote the residual and the write-half of `media.nullable-primitive-collection-mutation` but left the ORIGINAL reasoning standing two sentences earlier: "A null reads as EMPTY, so the item matches nothing"; "the REJECTED alternative was the All*() set"; "SKIPPING the row is the conservative repair". The shipped code is `?? AllDaysOfWeek()` -- precisely the alternative that passage calls rejected. The previous commit then inserted residual (1), which reasons entirely FROM the All*() reading, two sentences after the sentence denying it. That is worse than a stale comment. A session resolving this key -- or reading the MemPalace mirror, which carries `rule:` verbatim -- would have been told to write the guard the other way, i.e. talked into the `[]` reading that the same record elsewhere argues is data corruption one save later. Replaced the whole passage, then swept the record for every other mention of the empty reading rather than trusting the one replacement: the only survivor is the new sentence that records EMPTY as the rejected alternative, which is the direction that stops it being re-adopted. THE MEDIUM: one arrangement did not close the hole it claimed to. The discriminating control added last commit nulls DaysOfWeek against a restrictive MonthsOfYear. It excludes "any NULL matches unconditionally" only for that dimension. The review supplied the surviving mutant -- `if (item.MonthsOfYear is null) { return item; }` ahead of the checks -- and traced it green through all nine tests. Verified by EXECUTION, not by reading: applied to the previous fixture it passes; applied now it FAILS 1 of 11. Each of the three dimensions is now nulled against a restriction on a different dimension. The rest, all from the same round: - The coverage guard's test detection listed attribute TYPES, and each list falsely reddened whatever it omitted: TestAttribute alone missed [TestCase], and the three-type replacement missed [Theory]. Now decided by NUnit's own ITestBuilder/ISimpleTestBuilder interfaces, which cannot fall behind the vocabulary. It also dropped BindingFlags.Static (GetMethods() defaults to including it), which would have falsely reddened a static test method. - The same guard accepted an ABSTRACT fixture -- NUnit never instantiates one. The indexer population already filtered IsAbstract; the fixture side now mirrors it. - The record's `mechanics:` still described the old `[Test]`-only clause, in the same file the change edited. - An made the ProgramScheduleAlternate empty-case test inherit a docstring written from the PlayoutTemplate test's viewpoint. Two more mutations executed: - `if (item.MonthsOfYear is null) return item;` -> 1 red, 10 green. This is the mutant that survived the previous head; it no longer does. - an abstract type named in the covered set -> coverage guard red. Local gate: ErsatzTV.Tests 2091 passed / 6 skipped (the three fixtures' MySQL halves, skipping visibly without ETV_TEST_MYSQL_CONNECTION), Core.Tests 697/1 -- 0 failures. Format clean, no BOM on the touched set. decisions_validate OK. Refs #823 Refs #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .../AlternateScheduleSelectorTests.cs | 36 ++++++++++++++ .../RecurrenceLimitsMapperNullTests.cs | 5 +- .../SearchIndexMutationCoverageTests.cs | 48 ++++++++++++------- docs/decisions/README.md | 2 +- .../nullable-primitive-collection-mutation.md | 4 +- 5 files changed, 74 insertions(+), 21 deletions(-) diff --git a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs index bce3353e3..1592b6885 100644 --- a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs @@ -971,6 +971,42 @@ public static class AlternateScheduleSelectorTests + "excludes a March date"); } + /// + /// One arrangement is not enough: with only the DaysOfWeek case above, a PER-DIMENSION + /// mutant survives the whole fixture — e.g. if (item.MonthsOfYear is null) return item; + /// placed ahead of the checks is never reached by that test, because its MonthsOfYear is + /// non-null. So each of the three dimensions is nulled in turn against a restriction on a + /// DIFFERENT dimension. + /// + [Test] + public void A_Null_MonthsOfYear_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.MonthsOfYear = null!; + + // AnyDate is a WEDNESDAY; restrict to Monday only. + template.DaysOfWeek = [DayOfWeek.Monday]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL MonthsOfYear relaxes ONLY the month dimension — the Monday restriction still " + + "excludes a Wednesday"); + } + + /// + [Test] + public void A_Null_DaysOfMonth_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfMonth = null!; + + // AnyDate is in MARCH; restrict to January only. + template.MonthsOfYear = [1]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL DaysOfMonth relaxes ONLY the day-of-month dimension — the January restriction " + + "still excludes a March date"); + } + /// /// A null must not be confused with an explicitly EMPTY collection. Empty is a legal, reachable /// state meaning "matches no day", and it keeps that meaning — which is exactly why a NULL diff --git a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs index 4000be479..3a4d613b3 100644 --- a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs @@ -125,7 +125,10 @@ public class RecurrenceLimitsMapperNullTests vm.MonthsOfYear.ShouldBeEmpty(); } - /// + /// + /// The ProgramScheduleAlternate half of the same pair — see the PlayoutTemplate one above + /// for why there is one per MAPPER rather than one in total. + /// [Test] public void An_Explicitly_Empty_Collection_Is_Not_Rewritten() { diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs index aefc3e24c..f29cc24da 100644 --- a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -2,6 +2,7 @@ using System.Reflection; using ErsatzTV.Core.Interfaces.Search; using ErsatzTV.Infrastructure.Search; using NUnit.Framework; +using NUnit.Framework.Interfaces; using Shouldly; namespace ErsatzTV.Tests.Integration; @@ -80,27 +81,40 @@ public class SearchIndexMutationCoverageTests foreach ((Type indexer, Type fixture) in CoveredBy) { - // DeclaredOnly, and the whole NUnit test-method vocabulary rather than [Test] alone. Both - // narrowings are load-bearing: without DeclaredOnly a fixture that merely SUBCLASSES another - // one inherits its [Test] and satisfies this while driving the wrong indexer (and Values - // .Distinct() cannot catch that, since the two Types differ); and matching only TestAttribute - // would falsely redden a future fixture written entirely as [TestCase]/[TestCaseSource]. - // [Explicit] and [Ignore] are rejected because such a fixture never runs in CI, which is the - // same "wired is not running" failure the guard exists to prevent. - bool runnable = fixture - .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) - .Any(m => - m.GetCustomAttributes(inherit: true).Any(a => - a is TestAttribute or TestCaseAttribute or TestCaseSourceAttribute) - && m.GetCustomAttributes(inherit: true).All(a => a is not (ExplicitAttribute or IgnoreAttribute))); + // Test-ness is decided by NUnit's INTERFACES, not by a hand-listed set of attribute types. + // A list has to be kept in step with NUnit and silently falsely-reddens whatever it omits -- + // an earlier revision listed TestAttribute alone (so a [TestCase]-only fixture failed), then + // three types (so a [Theory] one did). ITestBuilder/ISimpleTestBuilder is what NUnit itself + // dispatches on, so it cannot fall behind the vocabulary. + // + // DeclaredOnly is load-bearing: without it a fixture that merely SUBCLASSES another inherits + // its [Test] and satisfies this while driving the wrong indexer -- and Values.Distinct() + // cannot catch that, since the two Types differ. Static is included because NUnit runs a + // public static test method in a non-static fixture, and omitting the flag would falsely + // redden one. + const BindingFlags TestMethods = + BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly; - bool fixtureRuns = fixture.GetCustomAttributes(inherit: true) - .All(a => a is not (ExplicitAttribute or IgnoreAttribute)); + static bool NeverRuns(object[] attributes) => + attributes.Any(a => a is ExplicitAttribute or IgnoreAttribute); - runnable.ShouldBeTrue( + bool declaresARunnableTest = fixture + .GetMethods(TestMethods) + .Select(m => m.GetCustomAttributes(inherit: true)) + .Any(attrs => + attrs.Any(a => a is ITestBuilder or ISimpleTestBuilder) && !NeverRuns(attrs)); + + declaresARunnableTest.ShouldBeTrue( $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but DECLARES no " + "runnable test method of its own, so it proves nothing"); - fixtureRuns.ShouldBeTrue( + + // The same "wired is not running" failure at fixture level, in its three forms: an abstract + // class NUnit will not instantiate, and [Explicit]/[Ignore]. The indexer population at the top + // of this method already filters IsAbstract; the fixture side needs the mirror of that. + fixture.IsAbstract.ShouldBeFalse( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is abstract, so " + + "NUnit never runs it"); + NeverRuns(fixture.GetCustomAttributes(inherit: true)).ShouldBeFalse( $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is [Explicit] or " + "[Ignore]d, so it never runs and proves nothing"); } diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 81d6637d9..cd54cea74 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API's `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as 'applies on every date' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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 index 17e0abd71..53e9bf2b2 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,9 +5,9 @@ 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as EMPTY, so the item matches nothing and the loop moves on; the REJECTED alternative was the `All*()` set that represents unrestricted elsewhere in this domain. The API''s `?? []` does NOT decide this and must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant. What decides it: there is no correct behaviour to preserve, because the row THROWS today and fails the playout build outright. Given absent restriction data, SKIPPING the row (the alternate falls through to the default schedule; a template contributes no blocks) is the conservative repair, while reading absence as ''applies on every date'' lets a row carrying no information OVERRIDE the default at its index. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.' +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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that declares no `[Test]`. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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.' +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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that does not DECLARE a runnable test of its own. Test-ness is decided by NUnit''s `ITestBuilder`/`ISimpleTestBuilder` interfaces rather than a list of attribute types, because two successive hand-written lists each falsely reddened whatever they omitted (`[TestCase]`, then `[Theory]`); `DeclaredOnly` is what stops a fixture SUBCLASSING another from inheriting its `[Test]` and passing while driving the wrong indexer, which `Values.Distinct()` cannot see since the two types differ; and `abstract`, `[Explicit]` and `[Ignore]` are each rejected as the fixture-level form of ''wired is not running''. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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` -- 2.47.3 From 3951fcf51669fb80fb7a060e6a06525a9f58880d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 21:00:22 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(823):=20stop=20patching=20the=20record?= =?UTF-8?q?=20by=20grep=20=E2=80=94=20a=20second=20`=3F=3F=20[]`=20survive?= =?UTF-8?q?d,=20and=20my=20"swept=20it"=20claim=20was=20false?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-four review. One HIGH, again in the decision record, and the previous commit message asserted this exact class was cleared. It was not. THE HIGH, and the reason it recurred. A second sentence still described the rejected reading: "The guard form is `?? []` into a local rather than this record's Optional(x).Flatten(), a STATED deviation". The shipped guard is `?? AllDaysOfWeek()`. That sentence is the one that dictates guard FORM to the next implementer, so it would have taught the `[]` reading the same record spends a paragraph calling data corruption -- and it had already propagated into docs/decisions/README.md, the mandated entry point, which carries `rule:` verbatim. The mechanism, not the sentence, is the defect. I swept with a regex keyed on "null" plus a reading word; this sentence talks about guard FORM and contains neither, so it could not match. That is grepping the retracted WORDING instead of sweeping the CONCEPT, which is exactly what this corpus warns about -- and three rounds in a row have now found a defect introduced by the previous round's targeted string edit. So the fix is not another targeted edit: the whole `rule:` field was split into its 39 sentences and read back one by one against the code. Everything below came out of that pass rather than a grep. Its secondary damage is worth recording because it is the shape of a rationale that outlives its claim: the deviation was justified by ".ToList() allocates for nothing", which is now BOTH irrelevant to the choice AND false about the shipped code, since AllDaysOfMonth()/AllMonthsOfYear() are themselves Enumerable.Range(...).ToList() on exactly the null path it describes. - The opening sentence of `rule:` prescribed Optional(x).Flatten() as THE read-site form. It is the sentence most likely to be read in isolation, and it is wrong for six of the eight columns. It now separates the universal half (a LOCAL, never assigned back) from the half that is not (the substituted value), and names where each applies. - `signals:` had never been touched, so roughly 60% of `rule:` was unreachable by the discovery surface built for it -- no AlternateScheduleSelector, no mapper, no "unrestricted", and its paths: list named none of the files this work touched. It also advertised "Optional Flatten hoisted local" as the form, which is precisely what the six do NOT use. - The body prose was still entirely about SongMetadata while `rule:` had grown a whole second subject. Added the two results that contradicted the prior reasoning, in prose, where a reader meets them. A REAL BUG in my own guard, not just prose: fixture.IsAbstract.ShouldBeFalse(...) A C# `static class` compiles to `abstract sealed`, and NUnit runs tests declared in one -- this repo already has such a fixture (AlternateScheduleSelectorTests is `public static class`). So the check I added one commit ago to reject an un-runnable fixture would have falsely reddened a perfectly good static one. Now rejects an abstract BASE (abstract and NOT sealed), which is the case NUnit actually cannot instantiate. A SURVIVING MUTANT the added controls did not kill: AnyDate was 2024-03-06. With a day <= 12 a CROSS-WIRED substitution survives the whole fixture -- `DaysOfMonth ?? AllMonthsOfYear()` hands back 1..12, which still contains day 6, so every assertion passes while the guard substitutes the wrong set. Moved to 2024-03-20, still a Wednesday in March, outside 1..12. Measured both ways rather than reasoned: the cross-wire mutant passes the old fixture and FAILS 2 of 11 on the new one. Also re-witnessed, because I had modified that file and never re-proved it: restoring `??=` in LuceneSearchIndex reddens the LUCENE fixture (1 red, 1 green) -- the exact mirror of the Elastic mutation. Extracting ThrowOnWarningLogger did not cost #701 its proof, and the two fixtures are independently load-bearing in both directions. The record is now 73 prose lines, over the 60-line WARNING ceiling. Stated rather than trimmed: it is 42nd of 42 records over that line, and the added content is distinct findings (a second subject, a migration analysis and three residuals), not redundancy against a sibling. Local gate: ErsatzTV.Tests 2091 passed / 6 skipped (the three fixtures' MySQL halves), Core.Tests 697/1 -- 0 failures. Format clean, no BOM. decisions validate OK. Refs #823 Refs #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .../AlternateScheduleSelectorTests.cs | 15 ++++++++++--- .../SearchIndexMutationCoverageTests.cs | 10 ++++++--- docs/decisions/README.md | 2 +- .../nullable-primitive-collection-mutation.md | 21 +++++++++++++++++-- 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs index 1592b6885..4c584ee8b 100644 --- a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs @@ -892,8 +892,12 @@ public static class AlternateScheduleSelectorTests { private static readonly TimeSpan Offset = TimeSpan.FromHours(-5); - // A Wednesday in March, so no All*() member is coincidentally excluded. - private static readonly DateTimeOffset AnyDate = new(2024, 3, 6, 0, 0, 0, Offset); + // A Wednesday in March, so no All*() member is coincidentally excluded — and deliberately the + // 20th rather than the 6th. With a day <= 12 a CROSS-WIRED substitution survives the whole + // fixture: `DaysOfMonth ?? AllMonthsOfYear()` hands back 1..12, which still contains day 6, so + // every assertion here passes while the guard substitutes the wrong set. Day 20 is outside 1..12 + // and kills it. + private static readonly DateTimeOffset AnyDate = new(2024, 3, 20, 0, 0, 0, Offset); private static PlayoutTemplate Unrestricted() => new() @@ -992,7 +996,12 @@ public static class AlternateScheduleSelectorTests + "excludes a Wednesday"); } - /// + /// + /// The third of the per-dimension controls — see + /// for why one + /// arrangement is not enough. Here the nulled dimension is DaysOfMonth and the + /// restriction that must still bite is on MonthsOfYear. + /// [Test] public void A_Null_DaysOfMonth_Does_Not_Relax_The_Other_Dimensions() { diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs index f29cc24da..a94b2789c 100644 --- a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -111,9 +111,13 @@ public class SearchIndexMutationCoverageTests // The same "wired is not running" failure at fixture level, in its three forms: an abstract // class NUnit will not instantiate, and [Explicit]/[Ignore]. The indexer population at the top // of this method already filters IsAbstract; the fixture side needs the mirror of that. - fixture.IsAbstract.ShouldBeFalse( - $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is abstract, so " - + "NUnit never runs it"); + // `IsAbstract` ALONE is wrong here and would have been a false red: a C# `static class` + // compiles to `abstract sealed`, and NUnit runs tests declared in one -- this repo already has + // such a fixture (`AlternateScheduleSelectorTests`). What must be rejected is an abstract BASE + // (abstract and NOT sealed), which NUnit cannot instantiate. + (fixture.IsAbstract && !fixture.IsSealed).ShouldBeFalse( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is an abstract " + + "base class, so NUnit never runs it"); NeverRuns(fixture.GetCustomAttributes(inherit: true)).ShouldBeFalse( $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is [Explicit] or " + "[Ignore]d, so it never runs and proves nothing"); diff --git a/docs/decisions/README.md b/docs/decisions/README.md index cd54cea74..00be9fc08 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record's `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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 index 53e9bf2b2..802efc2cd 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,8 +5,8 @@ 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. The guard form is `?? []` into a local rather than this record''s `Optional(x).Flatten()`, a STATED deviation: the selector runs per item per date during playout build, where `.ToList()` allocates for nothing, and the property that matters — a local, never assigned back — is identical. 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' +rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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 for the SongMetadata pair · but the six SCHEDULING columns substitute the All*() sets, not empty · a legacy NULL DaysOfMonth means NO day-of-month restriction, not ''matches no day'' · reading it as empty INVERTS a pre-2024 PlayoutTemplate row · EF skips the value converter entirely for a NULL column, on both providers · AlternateScheduleSelector .Contains throws NRE on a null · guard the two Mapper.ProjectToViewModel overloads too or the SPA spreads a null and throws TypeError · appliesToDate is an exact port and must preview what is scheduled · the full-replace PUT persists the substituted set over the NULL on the next save · write side normalizes an omitted array to [] while the read side reads NULL as unrestricted (#880) · paths: `ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs`, `ErsatzTV.Application/Playouts/Mapper.cs`, `ErsatzTV.Application/Scheduling/Mapper.cs`, `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, `ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs`, `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs`, 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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that does not DECLARE a runnable test of its own. Test-ness is decided by NUnit''s `ITestBuilder`/`ISimpleTestBuilder` interfaces rather than a list of attribute types, because two successive hand-written lists each falsely reddened whatever they omitted (`[TestCase]`, then `[Theory]`); `DeclaredOnly` is what stops a fixture SUBCLASSING another from inheriting its `[Test]` and passing while driving the wrong indexer, which `Values.Distinct()` cannot see since the two types differ; and `abstract`, `[Explicit]` and `[Ignore]` are each rejected as the fixture-level form of ''wired is not running''. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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.' --- @@ -64,5 +64,22 @@ is unreproducible by construction.) The exception type follows the read FORM, no 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. +**The six scheduling columns were a SEPARATE question, and it was answered later (#823).** The body above +is about `SongMetadata` and the `??=` write; everything in `rule:` about `ProgramScheduleAlternate` / +`PlayoutTemplate` was added when #823 measured the reachability this record originally declined to assert. +Two results are worth having in prose rather than only in the rule, because both contradicted the +reasoning available beforehand: + +- **The converters do not run.** The read-side difference between `IntCollectionValueConverter` + (null-or-blank → `Array.Empty()`) and `EnumCollectionJsonValueConverter` (would dereference) is + what the question was framed around, and it is IRRELEVANT: EF does not invoke a value converter for a + NULL column at all, on either provider. Measured, not reasoned about — the same step that changed the + answer in #701. +- **A NULL means UNRESTRICTED, not empty**, and the reading is not a matter of taste. Exactly one + (column, provider) case is reachable without code writing a NULL, and it decides the rest: a + `PlayoutTemplate` row predating Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` had no + day-of-month restriction at all, so reading its NULL as empty would stop the template applying — an + inversion, and silent. The `All*()` sets preserve it. + 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. -- 2.47.3 From 9685132ee059a8b2a668c6cfdd1f5817187e6906 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 29 Aug 2026 21:14:27 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(823):=20three=20cleanups=20from=20the?= =?UTF-8?q?=20coherence=20pass=20=E2=80=94=20a=20wrong=20witness,=20a=20wr?= =?UTF-8?q?ong=20`because`,=20a=20duplicated=20`paths:`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-five review confirmed the decision record is coherent with no third survivor of the empty reading, and returned three LOW findings. All are in prose I wrote in the last two commits. - The comment defending `(IsAbstract && !IsSealed)` cited AlternateScheduleSelectorTests as an in-repo static-fixture witness. That class IS static, but it merely NESTS its [TestFixture]es and declares no test of its own, so it would fail the sibling "declares no runnable test" assertion rather than demonstrating the point. The rule is right and the witness was wrong, which is the worse of the two failures because a wrong example is what a reader checks the rule against. No witness is cited now, and why is stated. - A mis-bound `because` in `rule:`: "assigning a null and calling SaveChanges SUCCEEDS ... because only the HTTP request records normalize with `?? []`". The `?? []` clause explains how a null could REACH the entity; what makes the save succeed is the column being nullable. A right observation with a wrong cause attached. Split into the two claims. - `signals:` carried the literal token `paths:` twice, an artifact of appending the #823 path list to the existing one. It degrades the field the discovery surface parses. Also recorded from that review, and NOT changed: `MonthsOfYear ?? AllDaysOfMonth()` survives the selector fixture and no date can kill it -- 1..31 contains every valid month, so it is an EQUIVALENT mutant there rather than a coverage gap. Its non-equivalent twin at the DTO boundary is pinned per-dimension by RecurrenceLimitsMapperNullTests. Left alone deliberately: chasing an equivalent mutant with a contrived date would buy nothing and cost the fixture's readability. Local gate: ErsatzTV.Tests 2091 passed / 6 skipped, Core.Tests 697/1 -- 0 failures. Format clean, no BOM. decisions_validate OK. Refs #823 Refs #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW --- .../Integration/SearchIndexMutationCoverageTests.cs | 9 ++++++--- docs/decisions/README.md | 2 +- .../media/nullable-primitive-collection-mutation.md | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs index a94b2789c..115b8ad4f 100644 --- a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -112,9 +112,12 @@ public class SearchIndexMutationCoverageTests // class NUnit will not instantiate, and [Explicit]/[Ignore]. The indexer population at the top // of this method already filters IsAbstract; the fixture side needs the mirror of that. // `IsAbstract` ALONE is wrong here and would have been a false red: a C# `static class` - // compiles to `abstract sealed`, and NUnit runs tests declared in one -- this repo already has - // such a fixture (`AlternateScheduleSelectorTests`). What must be rejected is an abstract BASE - // (abstract and NOT sealed), which NUnit cannot instantiate. + // compiles to `abstract sealed`, and NUnit runs a test declared in one. What must be rejected + // is an abstract BASE (abstract and NOT sealed), which NUnit cannot instantiate. Deliberately + // no in-repo witness is cited: `AlternateScheduleSelectorTests` is a static class, but it + // merely NESTS its `[TestFixture]`es and declares no test of its own, so it would fail the + // sibling assertion above instead -- naming it here would have been a wrong example attached + // to a right rule. (fixture.IsAbstract && !fixture.IsSealed).ShouldBeFalse( $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is an abstract " + "base class, so NUnit never runs it"); diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 00be9fc08..2272d95bc 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,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 — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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 index 802efc2cd..07b3fad23 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,8 +5,8 @@ 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 — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL (the converter is skipped outbound too), because only the HTTP request records normalize with `?? []` while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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 for the SongMetadata pair · but the six SCHEDULING columns substitute the All*() sets, not empty · a legacy NULL DaysOfMonth means NO day-of-month restriction, not ''matches no day'' · reading it as empty INVERTS a pre-2024 PlayoutTemplate row · EF skips the value converter entirely for a NULL column, on both providers · AlternateScheduleSelector .Contains throws NRE on a null · guard the two Mapper.ProjectToViewModel overloads too or the SPA spreads a null and throws TypeError · appliesToDate is an exact port and must preview what is scheduled · the full-replace PUT persists the substituted set over the NULL on the next save · write side normalizes an omitted array to [] while the read side reads NULL as unrestricted (#880) · paths: `ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs`, `ErsatzTV.Application/Playouts/Mapper.cs`, `ErsatzTV.Application/Scheduling/Mapper.cs`, `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, `ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs`, `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs`, 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' +rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. 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. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. 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 for the SongMetadata pair · but the six SCHEDULING columns substitute the All*() sets, not empty · a legacy NULL DaysOfMonth means NO day-of-month restriction, not ''matches no day'' · reading it as empty INVERTS a pre-2024 PlayoutTemplate row · EF skips the value converter entirely for a NULL column, on both providers · AlternateScheduleSelector .Contains throws NRE on a null · guard the two Mapper.ProjectToViewModel overloads too or the SPA spreads a null and throws TypeError · appliesToDate is an exact port and must preview what is scheduled · the full-replace PUT persists the substituted set over the NULL on the next save · write side normalizes an omitted array to [] while the read side reads NULL as unrestricted (#880) · paths: `ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs`, `ErsatzTV.Application/Playouts/Mapper.cs`, `ErsatzTV.Application/Scheduling/Mapper.cs`, `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, `ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs`, `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs`, `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 and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that does not DECLARE a runnable test of its own. Test-ness is decided by NUnit''s `ITestBuilder`/`ISimpleTestBuilder` interfaces rather than a list of attribute types, because two successive hand-written lists each falsely reddened whatever they omitted (`[TestCase]`, then `[Theory]`); `DeclaredOnly` is what stops a fixture SUBCLASSING another from inheriting its `[Test]` and passing while driving the wrong indexer, which `Values.Distinct()` cannot see since the two types differ; and `abstract`, `[Explicit]` and `[Ignore]` are each rejected as the fixture-level form of ''wired is not running''. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. 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.' --- -- 2.47.3