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

Schedule-item recurrence fields: an omitted array means "never applies" on write but "unrestricted" on read #880

New Issue
Closed
opened 2026-08-29 20:21:30 +02:00 by timothy · 2 comments
timothy commented 2026-08-29 20:21:30 +02:00
Owner
Copy Link
Copy Source

Surfaced by the independent reviews of #823 (PR #879). Pre-existing, and deliberately NOT fixed there — #823 decided what a legacy database NULL means on the READ side; this is the WRITE side of the same field, which is a different question.

The asymmetry

After #823, the two halves of "this field is absent" point in opposite directions:

Where Absence means
A NULL column, read by AlternateScheduleSelector / the two Mapper.ProjectToViewModel overloads unrestricted (the All*() sets)
An omitted daysOfWeek / daysOfMonth / monthsOfYear in a PUT, normalized by PlayoutAlternateScheduleItemRequest.ToReplaceItem / PlayoutTemplateItemRequest.ToReplaceItem [], i.e. matches no day

Neither ReplacePlayoutAlternateScheduleItemsHandler nor ReplacePlayoutTemplateItemsHandler validates emptiness — only Items.Count == 0 is rejected.

The concrete outcome

An MCP or curl client PUTs an alternate-schedule item omitting daysOfWeek. It gets HTTP 200. The row is stored with an empty set, and therefore silently never applies — the playout falls through to the default schedule and nothing anywhere says why. The SPA does not hit this (it always sends the full arrays it received), so this is an API-client-only trap.

Why it was left out of #823

#823's scope is the read path and the meaning of a legacy NULL. The reviewers were split on whether this belongs there, and the deciding argument is that a client omitting a field on a write says nothing about what a legacy NULL meant — so folding it in would have widened that PR past its issue and mixed two different judgements. It is recorded as residual (3) on media.nullable-primitive-collection-mutation.

What to decide

  1. Reject an empty set with 422 naming the consequence, per api.ffmpeg-profile-numeric-bounds' shape (reject out of range rather than accept-then-rewrite). Most honest; a breaking change for any client currently sending [] deliberately.
  2. Normalize an OMITTED field to All*() so absence means the same thing on both sides, while an explicitly-sent [] keeps meaning "no days". Requires distinguishing absent from empty in the request record — List<T>? plus ?? All*() rather than ?? [].
  3. Leave it and document the trap in docs/api-conventions.md.

Option 2 looks right and is the one that actually removes the asymmetry, but it needs a check of how the generated SPA client and the MCP server serialize an absent vs empty array before committing to it.

Done-when

  • Absence on the write path and absence on the read path agree, or the disagreement is documented as deliberate with its reason
  • An API client cannot silently create a schedule item that never applies — it either succeeds meaningfully or fails with a 422 naming the consequence
  • A test pins the chosen behaviour and reddens when the normalization alone is removed
  • media.nullable-primitive-collection-mutation residual (3) updated to point at the outcome
  • Adversarial review passed
Surfaced by the independent reviews of #823 (PR #879). **Pre-existing, and deliberately NOT fixed there** — #823 decided what a legacy database NULL means on the READ side; this is the WRITE side of the same field, which is a different question. ## The asymmetry After #823, the two halves of "this field is absent" point in opposite directions: | Where | Absence means | |---|---| | A NULL column, read by `AlternateScheduleSelector` / the two `Mapper.ProjectToViewModel` overloads | **unrestricted** (the `All*()` sets) | | An omitted `daysOfWeek` / `daysOfMonth` / `monthsOfYear` in a PUT, normalized by `PlayoutAlternateScheduleItemRequest.ToReplaceItem` / `PlayoutTemplateItemRequest.ToReplaceItem` | **`[]`, i.e. matches no day** | Neither `ReplacePlayoutAlternateScheduleItemsHandler` nor `ReplacePlayoutTemplateItemsHandler` validates emptiness — only `Items.Count == 0` is rejected. ## The concrete outcome An MCP or curl client PUTs an alternate-schedule item omitting `daysOfWeek`. It gets **HTTP 200**. The row is stored with an empty set, and therefore **silently never applies** — the playout falls through to the default schedule and nothing anywhere says why. The SPA does not hit this (it always sends the full arrays it received), so this is an API-client-only trap. ## Why it was left out of #823 #823's scope is the read path and the meaning of a legacy NULL. The reviewers were split on whether this belongs there, and the deciding argument is that a client omitting a field on a write says nothing about what a legacy NULL meant — so folding it in would have widened that PR past its issue and mixed two different judgements. It is recorded as residual (3) on `media.nullable-primitive-collection-mutation`. ## What to decide 1. **Reject an empty set with 422** naming the consequence, per `api.ffmpeg-profile-numeric-bounds`' shape (reject out of range rather than accept-then-rewrite). Most honest; a breaking change for any client currently sending `[]` deliberately. 2. **Normalize an OMITTED field to `All*()`** so absence means the same thing on both sides, while an explicitly-sent `[]` keeps meaning "no days". Requires distinguishing absent from empty in the request record — `List<T>?` plus `?? All*()` rather than `?? []`. 3. Leave it and document the trap in `docs/api-conventions.md`. Option 2 looks right and is the one that actually removes the asymmetry, but it needs a check of how the generated SPA client and the MCP server serialize an absent vs empty array before committing to it. ## Done-when - [x] Absence on the write path and absence on the read path agree, or the disagreement is documented as deliberate with its reason - [x] An API client cannot silently create a schedule item that never applies — it either succeeds meaningfully or fails with a 422 naming the consequence - [x] A test pins the chosen behaviour and reddens when the normalization alone is removed - [x] `media.nullable-primitive-collection-mutation` residual (3) updated to point at the outcome - [x] Adversarial review passed
timothy added the priority: medium label 2026-08-29 20:21:30 +02:00
timothy added the in-progress label 2026-08-30 09:51:51 +02:00
timothy commented 2026-08-30 09:51:52 +02:00
Author
Owner
Copy Link
Copy Source

Claiming #880 (Claude Code / Opus 5 session, 2026-08-30).

Context for the other sessions live right now: I first claimed #887, found it was a three-way collision, and yielded it there. Picking this one specifically because it is disjoint from the CI/Dockerfile/workflow surface those sessions are editing — this is ErsatzTV.Application write-path + a test, no .gitea/, no docker/, no scripts/.

Starting from the issue's option 2 (normalize an OMITTED field to All*(), keep an explicit [] meaning "no days"), but the issue is explicit that it needs a check of how the generated SPA client and the MCP server serialize absent-vs-empty before committing — doing that check first and will report what it says.

Claiming #880 (Claude Code / Opus 5 session, 2026-08-30). Context for the other sessions live right now: I first claimed #887, found it was a three-way collision, and yielded it there. Picking this one specifically because it is disjoint from the CI/Dockerfile/workflow surface those sessions are editing — this is `ErsatzTV.Application` write-path + a test, no `.gitea/`, no `docker/`, no `scripts/`. Starting from the issue's option 2 (normalize an OMITTED field to `All*()`, keep an explicit `[]` meaning "no days"), but the issue is explicit that it needs a check of how the generated SPA client and the MCP server serialize absent-vs-empty before committing — doing that check first and will report what it says.
timothy commented 2026-08-30 11:29:37 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #892 (squash-merged to main). An absent recurrence array (daysOfWeek / daysOfMonth / monthsOfYear) on both playout replace-list write paths now normalizes to AlternateScheduleSelector.All*() — unrestricted — instead of []; an explicitly empty array is rejected with a 422 naming the consequence, via a new shared RecurrenceSetBounds (ErsatzTV.Application/Scheduling) called from both replace handlers. Follow-up #894 filed for the SPA half.

Root cause: The three recurrence sets are read conjunctively by AlternateScheduleSelector.GetScheduleForDate — a miss on any one continues — so the empty set is the maximally restrictive value, not a neutral one. ?? [] in the request records read as "default to no filter" and meant "default to matching nothing": the API returned HTTP 200 and stored an item that could never apply on any date, with nothing logged. #823 had already decided the READ side the other way (a NULL column reads as All*()), so the two halves of "this field is absent" pointed in opposite directions.

Decisions/conventions changed:

  • Added api.absent-collection-means-unrestricted (docs/decisions/records/api/).
  • Updated media.nullable-primitive-collection-mutation — residual (3) now points at the outcome instead of forward-referencing this issue.
  • Added docs/api-conventions.md §3e.

Reusable knowledge:

  1. Absent and explicitly-empty are two requests, not one, and merging them is lossy in either direction. Normalizing [] too makes "no days" inexpressible; rejecting absence breaks every client that omits an optional field.
  2. Nullable ≠ optional in the generated schema. Making the C# property List<T>? left all three in required in v1.json (type ["null","array"]). An error message saying "omit the property" would instruct a schema violation — it says "send null". A byte-identical update-openapi.sh diff is therefore not evidence a change shipped.
  3. Validation that must exempt an unchanged legacy value has to live where the stored row is — the handler, not the controller. This bites hardest on whole-list replace PUTs, where one pre-existing bad row otherwise blocks saving every other item.
  4. Precedent for a STYLE is not evidence the BEHAVIOUR is correct. "The sibling ValidateDateRanges validates every item uniformly" was used to justify validating the catch-all, whose recurrence the handler discards — producing a 422 whose stated reason is false for that item.
  5. Fixture trap (measured): the template handler's .ThenInclude(t => t.Template) navigation is required, so a PlayoutTemplate seeded without its Template row is joined out of existing — the stored row goes invisible and an exemption test fails claiming the rule is broken when the fixture is.
  6. Harness trap: scripts/tests/test_mutation_harness.py builds its sandbox from git, so a new decision record written but not git added makes the regenerated catalog look stale — the red names decisions_validate.py and the real cause is the untracked file.

Verification:

  • ErsatzTV.Tests full suite: 2099 passed / 0 failed (6 skipped).
  • Three mutation proofs, executed separately, each reddening an explainable set: reverting ?? All*() reddens exactly the 3 normalization tests; neutering IsNewlyEmpty reddens exactly the 3 rejection tests; dropping only the stored comparison reddens the 2 exemption tests. Rejection tests also assert the version did not bump (anti-vacuity).
  • Live-E2E (required for a write-path PR) against a real local instance: an omitted property → 200 and round-trips as 7/31/12 unrestricted — the Newtonsoft missing-property chain unit tests cannot reach, since they construct the record with null directly; an explicit [] on a stored item → 422 verbatim; [] on the catch-all → 200 with the sibling's explicit ['Friday'] preserved.
  • Prod measured 2026-08-30 (jazz /config/ersatztv.sqlite3): ProgramScheduleAlternate and PlayoutTemplate both hold 0 rows. The exemption is justified by the shape of a whole-list replace, not by that count.
  • Independent cold cross-family review (Codex): first pass BLOCKED with 3 findings (all verified against the code, all real); re-review of the fix commit returned MERGEABLE with a boundary matrix over every (submitted x stored) combination.
  • CI on head 3500bf3: all 14 checks success (image build skipped — PR path).

Deferred: #894 — the SPA (PlayoutScheduleEditors.tsx) still lets a user deselect every day and build the empty state the server now rejects, so the feedback arrives at save time rather than at the field. Labelled priority: low / frontend.

Docs updated: docs/api-conventions.md (§3e, new), docs/decisions/records/api/absent-collection-means-unrestricted.md (new), docs/decisions/records/media/nullable-primitive-collection-mutation.md (residual 3), docs/decisions/README.md (regenerated).

## Closing record **Outcome:** Shipped in PR #892 (squash-merged to `main`). An **absent** recurrence array (`daysOfWeek` / `daysOfMonth` / `monthsOfYear`) on both playout replace-list write paths now normalizes to `AlternateScheduleSelector.All*()` — unrestricted — instead of `[]`; an **explicitly empty** array is rejected with a 422 naming the consequence, via a new shared `RecurrenceSetBounds` (`ErsatzTV.Application/Scheduling`) called from both replace handlers. Follow-up #894 filed for the SPA half. **Root cause:** The three recurrence sets are read **conjunctively** by `AlternateScheduleSelector.GetScheduleForDate` — a miss on any one `continue`s — so the empty set is the maximally *restrictive* value, not a neutral one. `?? []` in the request records read as "default to no filter" and meant "default to matching nothing": the API returned HTTP 200 and stored an item that could never apply on any date, with nothing logged. #823 had already decided the READ side the other way (a NULL column reads as `All*()`), so the two halves of "this field is absent" pointed in opposite directions. **Decisions/conventions changed:** - **Added** `api.absent-collection-means-unrestricted` (`docs/decisions/records/api/`). - **Updated** `media.nullable-primitive-collection-mutation` — residual (3) now points at the outcome instead of forward-referencing this issue. - **Added** `docs/api-conventions.md` §3e. **Reusable knowledge:** 1. **Absent and explicitly-empty are two requests, not one**, and merging them is lossy in *either* direction. Normalizing `[]` too makes "no days" inexpressible; rejecting absence breaks every client that omits an optional field. 2. **Nullable ≠ optional in the generated schema.** Making the C# property `List<T>?` left all three in `required` in `v1.json` (type `["null","array"]`). An error message saying "omit the property" would instruct a schema violation — it says "send null". A byte-identical `update-openapi.sh` diff is therefore *not* evidence a change shipped. 3. **Validation that must exempt an unchanged legacy value has to live where the stored row is** — the handler, not the controller. This bites hardest on whole-list replace PUTs, where one pre-existing bad row otherwise blocks saving every *other* item. 4. **Precedent for a STYLE is not evidence the BEHAVIOUR is correct.** "The sibling `ValidateDateRanges` validates every item uniformly" was used to justify validating the catch-all, whose recurrence the handler discards — producing a 422 whose stated reason is false for that item. 5. **Fixture trap (measured):** the template handler's `.ThenInclude(t => t.Template)` navigation is *required*, so a `PlayoutTemplate` seeded without its `Template` row is joined **out** of `existing` — the stored row goes invisible and an exemption test fails claiming the rule is broken when the fixture is. 6. **Harness trap:** `scripts/tests/test_mutation_harness.py` builds its sandbox from **git**, so a new decision record written but not `git add`ed makes the regenerated catalog look stale — the red names `decisions_validate.py` and the real cause is the untracked file. **Verification:** - `ErsatzTV.Tests` full suite: **2099 passed / 0 failed** (6 skipped). - **Three mutation proofs, executed separately**, each reddening an explainable set: reverting `?? All*()` reddens exactly the 3 normalization tests; neutering `IsNewlyEmpty` reddens exactly the 3 rejection tests; dropping only the stored comparison reddens the 2 exemption tests. Rejection tests also assert the version did **not** bump (anti-vacuity). - **Live-E2E** (required for a write-path PR) against a real local instance: an **omitted** property → 200 and round-trips as 7/31/12 unrestricted — the Newtonsoft missing-property chain unit tests cannot reach, since they construct the record with `null` directly; an explicit `[]` on a stored item → 422 verbatim; `[]` on the catch-all → 200 with the sibling's explicit `['Friday']` preserved. - **Prod measured 2026-08-30** (jazz `/config/ersatztv.sqlite3`): `ProgramScheduleAlternate` and `PlayoutTemplate` both hold **0 rows**. The exemption is justified by the shape of a whole-list replace, not by that count. - **Independent cold cross-family review (Codex):** first pass **BLOCKED** with 3 findings (all verified against the code, all real); re-review of the fix commit returned **MERGEABLE** with a boundary matrix over every (submitted x stored) combination. - CI on head `3500bf3`: all 14 checks success (image build skipped — PR path). **Deferred:** #894 — the SPA (`PlayoutScheduleEditors.tsx`) still lets a user deselect every day and build the empty state the server now rejects, so the feedback arrives at save time rather than at the field. Labelled `priority: low` / `frontend`. **Docs updated:** `docs/api-conventions.md` (§3e, new), `docs/decisions/records/api/absent-collection-means-unrestricted.md` (new), `docs/decisions/records/media/nullable-primitive-collection-mutation.md` (residual 3), `docs/decisions/README.md` (regenerated).
timothy removed the in-progress label 2026-08-30 11:29:47 +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#880