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.