timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Scheduling collection-valued columns: is a runtime null reachable on DaysOfMonth / MonthsOfYear / DaysOfWeek? #823

New Issue
Closed
opened 2026-08-23 00:07:12 +02:00 by timothy · 3 comments
timothy commented 2026-08-23 00:07:12 +02:00
Owner
Copy Link
Copy Source

Split out of #701, which derived the population of collection-valued scalar columns from the model and then deliberately declined to make a claim about six of them.

What #701 established

Eight properties in this model are a whole collection stored in ONE column:

Property Entity Mapping Storage
Artists, AlbumArtists SongMetadata EF-native primitive collection JSON array
DaysOfMonth, MonthsOfYear ProgramScheduleAlternate, PlayoutTemplate IntCollectionValueConverter comma-separated text
DaysOfWeek ProgramScheduleAlternate, PlayoutTemplate EnumCollectionJsonValueConverter JSON array

#701 fixed the SongMetadata pair. It verified that no site applies ??= to any of the six scheduling ones, so #701's own defect has no instance there, and stopped — because whether a null can reach one of the six at runtime is a different question it did not measure.

The open question

  • The API request records normalize (DaysOfMonth ?? [] in PlayoutAlternateScheduleItemRequest / PlayoutTemplateItemRequest), so the HTTP path looks covered.
  • But ReplacePlayoutAlternateScheduleItemsHandler (:103/:123) and ReplacePlayoutTemplateItemsHandler (:67/:89) assign the command value straight onto the entity. A non-API caller constructing the command directly is not covered by the request record's guard.
  • The columns are nullable and historical migrations created them nullable, so legacy rows may hold NULL independently of any code path.
  • If a null does reach one, AlternateScheduleSelector (~:94/:100) calls .Contains(...) on it.

Note the two converters behave differently on the read side and that difference should be measured, not assumed: IntCollectionValueConverter maps null-or-blank to Array.Empty<int>(), while EnumCollectionJsonValueConverter would dereference the result of JsonConvert.DeserializeObject. EF also does not normally invoke a converter for a NULL column at all. Measure what actually materializes from a NULL row on both providers rather than reasoning about it — that is precisely the step #701's first done-when box forced, and it changed the answer there.

Done-when

  • Determined empirically (a real TvContext, a row with a NULL column, both providers) what each of the two converters materializes
  • Determined whether any non-API caller can put a null into one of the six
  • If a null is reachable: guarded at the read site per media.nullable-primitive-collection-mutation (never by assigning back onto the entity), with a test that goes red without the guard
  • If it is NOT reachable: recorded as measured, dated, with the reproduction — not left as an assumption
  • media.nullable-primitive-collection-mutation updated to point at the answer instead of at this issue
  • Adversarial review passed
Split out of #701, which derived the population of collection-valued scalar columns from the model and then deliberately declined to make a claim about six of them. ## What #701 established Eight properties in this model are a whole collection stored in ONE column: | Property | Entity | Mapping | Storage | |---|---|---|---| | `Artists`, `AlbumArtists` | `SongMetadata` | EF-native primitive collection | JSON array | | `DaysOfMonth`, `MonthsOfYear` | `ProgramScheduleAlternate`, `PlayoutTemplate` | `IntCollectionValueConverter` | comma-separated text | | `DaysOfWeek` | `ProgramScheduleAlternate`, `PlayoutTemplate` | `EnumCollectionJsonValueConverter` | JSON array | #701 fixed the `SongMetadata` pair. It verified that **no site applies `??=` to any of the six** scheduling ones, so #701's own defect has no instance there, and stopped — because whether a **null can reach one of the six at runtime** is a different question it did not measure. ## The open question - The API request records normalize (`DaysOfMonth ?? []` in `PlayoutAlternateScheduleItemRequest` / `PlayoutTemplateItemRequest`), so the HTTP path looks covered. - But `ReplacePlayoutAlternateScheduleItemsHandler` (~:103/:123) and `ReplacePlayoutTemplateItemsHandler` (~:67/:89) assign the **command** value straight onto the entity. A non-API caller constructing the command directly is not covered by the request record's guard. - The columns are nullable and historical migrations created them nullable, so legacy rows may hold NULL independently of any code path. - If a null does reach one, `AlternateScheduleSelector` (~:94/:100) calls `.Contains(...)` on it. Note the two converters behave differently on the read side and that difference should be measured, not assumed: `IntCollectionValueConverter` maps null-or-blank to `Array.Empty<int>()`, while `EnumCollectionJsonValueConverter` would dereference the result of `JsonConvert.DeserializeObject`. EF also does not normally invoke a converter for a NULL column at all. **Measure what actually materializes from a NULL row on both providers rather than reasoning about it** — that is precisely the step #701's first done-when box forced, and it changed the answer there. ## Done-when - [x] Determined empirically (a real `TvContext`, a row with a NULL column, both providers) what each of the two converters materializes - [x] Determined whether any non-API caller can put a null into one of the six - [x] If a null is reachable: guarded at the read site per `media.nullable-primitive-collection-mutation` (never by assigning back onto the entity), with a test that goes red without the guard - [x] If it is NOT reachable: recorded as measured, dated, with the reproduction — not left as an assumption - [x] `media.nullable-primitive-collection-mutation` updated to point at the answer instead of at this issue - [x] Adversarial review passed
timothy added the priority: medium label 2026-08-23 01:07:17 +02:00
timothy commented 2026-08-29 11:22:39 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — bundled session closing out both #701 deferrals (#823 + #824) together, because both must edit the same decision record (media.nullable-primitive-collection-mutation) and splitting them across two sessions would produce conflicting edits to its rule/mechanics text.

Claude Code / Opus 5, orchestrator tier. Work happens in a worktree off origin/main.

Claiming — bundled session closing out both #701 deferrals (#823 + #824) together, because both must edit the same decision record (`media.nullable-primitive-collection-mutation`) and splitting them across two sessions would produce conflicting edits to its `rule`/`mechanics` text. Claude Code / Opus 5, orchestrator tier. Work happens in a worktree off `origin/main`.
timothy added the in-progress label 2026-08-29 11:22:46 +02:00
timothy commented 2026-08-29 20:08:47 +02:00
Author
Owner
Copy Link
Copy Source

Note on the two mutually-exclusive Done-when boxes, so the tick is not read as more than it is.

Boxes 3 and 4 are an either/or pair — reachable vs not reachable — and box 3 is the branch that applied: a null IS reachable, so the guard shipped. Box 4 is ticked because its substance was delivered regardless of which branch held: the measurement is recorded, dated 2026-08-29, with the reproduction, in media.nullable-primitive-collection-mutation rather than left as an assumption. It is not a claim that the null turned out to be unreachable.

Note on the two mutually-exclusive Done-when boxes, so the tick is not read as more than it is. Boxes 3 and 4 are an either/or pair — reachable vs not reachable — and **box 3 is the branch that applied**: a null IS reachable, so the guard shipped. Box 4 is ticked because its *substance* was delivered regardless of which branch held: the measurement is recorded, dated 2026-08-29, with the reproduction, in `media.nullable-primitive-collection-mutation` rather than left as an assumption. It is not a claim that the null turned out to be unreachable.
timothy commented 2026-08-29 21:56:51 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #879 (merged as 1afad0851). A runtime null IS reachable on the six collection-valued scalar columns, and is now guarded at both read sites — AlternateScheduleSelector.GetScheduleForDate and the two Mapper.ProjectToViewModel overloads that feed the SPA. A NULL reads as UNRESTRICTED (the All*() sets), never assigned back onto the entity.

Root cause: The columns are nullable: true in every migration on both providers, and EF materializes a NULL column as CLR null — so .Contains threw NullReferenceException. The narrower, load-bearing fact: exactly ONE (column, provider) case is reachable without code writing a NULL — SQLite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth is an AddColumn with no defaultValue, so rows predating it hold NULL and by construction had no day-of-month restriction.

Decisions/conventions changed: media.nullable-primitive-collection-mutation rewritten — it now answers the reachability question it previously declined to assert, records the UNRESTRICTED reading with EMPTY as the explicitly rejected alternative, and carries three residuals. docs/testing.md gained a "Provider-parity fixtures (opt-in MySQL)" section; docs/README.md a matching task signal; docs/guard-inventory.md's hand-written C# guard list went from five files to six.

Reusable knowledge:

  1. EF does not invoke a value converter for a NULL column at all — on either provider. The read-side difference between IntCollectionValueConverter (null→Array.Empty<int>()) and EnumCollectionJsonValueConverter (would dereference) is what the question was framed around and is irrelevant: neither runs. Measured, and it inverted the expected answer.
  2. A nullable column does not imply a NULL row. Five of the six were present at CreateTable, so a NULL there still needs code to write one; on MySQL there is no code-path-free NULL for any of the six. "The columns are nullable, so legacy rows can hold NULL" is the plausible-and-wrong version.
  3. A read guard becomes a WRITE one save later. The DTO round-trips (mapper → SPA draft → full-replace PUT → entity), so whatever the mapper substitutes gets persisted over the NULL. With All*() that is benign; the rejected [] would have rewritten the row to "matches no day" — silent data corruption, not merely a debatable reading.
  4. Sweeping a long decision record by grep does not work. Two consecutive review rounds found the record asserting both the shipped reading and its inverse, because targeted string replacements left earlier sentences standing and my regex keyed on the retracted wording. The mechanism that worked was splitting rule: into its sentences and reading every one back against the code.

Verification: ErsatzTV.Tests 2091–2097 passed, Core.Tests 697, Infrastructure.Tests 114, Architecture.Tests 7, Scanner.Tests 1504 — 0 failures. scripts/tests 1228 passed. The dual-provider fixture ran against a real MySQL 8.4 (ephemeral, on jazz) as well as SQLite. Ten clause mutations executed and witnessed red, each scoped so its siblings stayed green — including two that a reviewer proved had survived an earlier head (a per-dimension early return, and a cross-wired All*() substitution that day-of-month 6 could not distinguish). CI green on 9685132.

Deferred: #880 — an omitted recurrence array means "never applies" on the write path while a NULL means "unrestricted" on the read path. Deliberately not resolved here: a client omitting a field on a write is a different question from what a legacy NULL meant. Also recorded as residual (3) on the decision record. Re-arming the MySQL fixtures in CI remains #627.

Docs updated: docs/decisions/records/media/nullable-primitive-collection-mutation.md, docs/decisions/README.md (regenerated), docs/testing.md, docs/README.md, docs/guard-inventory.md.

## Closing record **Outcome:** Shipped in PR #879 (merged as `1afad0851`). A runtime null IS reachable on the six collection-valued scalar columns, and is now guarded at **both** read sites — `AlternateScheduleSelector.GetScheduleForDate` and the two `Mapper.ProjectToViewModel` overloads that feed the SPA. A NULL reads as **UNRESTRICTED** (the `All*()` sets), never assigned back onto the entity. **Root cause:** The columns are `nullable: true` in every migration on both providers, and EF materializes a NULL column as CLR `null` — so `.Contains` threw `NullReferenceException`. The narrower, load-bearing fact: exactly ONE (column, provider) case is reachable without code writing a NULL — SQLite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with no `defaultValue`, so rows predating it hold NULL and by construction had no day-of-month restriction. **Decisions/conventions changed:** `media.nullable-primitive-collection-mutation` rewritten — it now answers the reachability question it previously declined to assert, records the UNRESTRICTED reading with EMPTY as the explicitly rejected alternative, and carries three residuals. `docs/testing.md` gained a "Provider-parity fixtures (opt-in MySQL)" section; `docs/README.md` a matching task signal; `docs/guard-inventory.md`'s hand-written C# guard list went from five files to six. **Reusable knowledge:** 1. **EF does not invoke a value converter for a NULL column at all** — on either provider. The read-side difference between `IntCollectionValueConverter` (null→`Array.Empty<int>()`) and `EnumCollectionJsonValueConverter` (would dereference) is what the question was framed around and is *irrelevant*: neither runs. Measured, and it inverted the expected answer. 2. **A nullable column does not imply a NULL row.** Five of the six were present at `CreateTable`, so a NULL there still needs code to write one; on MySQL there is no code-path-free NULL for any of the six. "The columns are nullable, so legacy rows can hold NULL" is the plausible-and-wrong version. 3. **A read guard becomes a WRITE one save later.** The DTO round-trips (mapper → SPA draft → full-replace PUT → entity), so whatever the mapper substitutes gets persisted over the NULL. With `All*()` that is benign; the rejected `[]` would have rewritten the row to "matches no day" — silent data corruption, not merely a debatable reading. 4. **Sweeping a long decision record by grep does not work.** Two consecutive review rounds found the record asserting both the shipped reading and its inverse, because targeted string replacements left earlier sentences standing and my regex keyed on the retracted *wording*. The mechanism that worked was splitting `rule:` into its sentences and reading every one back against the code. **Verification:** ErsatzTV.Tests 2091–2097 passed, Core.Tests 697, Infrastructure.Tests 114, Architecture.Tests 7, Scanner.Tests 1504 — 0 failures. `scripts/tests` 1228 passed. The dual-provider fixture ran against a real MySQL 8.4 (ephemeral, on jazz) as well as SQLite. Ten clause mutations executed and witnessed red, each scoped so its siblings stayed green — including two that a reviewer proved had *survived* an earlier head (a per-dimension early return, and a cross-wired `All*()` substitution that day-of-month 6 could not distinguish). CI green on `9685132`. **Deferred:** #880 — an omitted recurrence array means "never applies" on the write path while a NULL means "unrestricted" on the read path. Deliberately not resolved here: a client omitting a field on a write is a different question from what a legacy NULL meant. Also recorded as residual (3) on the decision record. Re-arming the MySQL fixtures in CI remains #627. **Docs updated:** `docs/decisions/records/media/nullable-primitive-collection-mutation.md`, `docs/decisions/README.md` (regenerated), `docs/testing.md`, `docs/README.md`, `docs/guard-inventory.md`.
timothy removed the in-progress label 2026-08-29 21:56:58 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#823