fix(880): an absent recurrence array means unrestricted, an explicit [] is rejected #892

Merged

timothy merged 2 commits from fix/880-omitted-recurrence-means-unrestricted into main 2026-08-30 11:29:03 +02:00

Conversation 1 Commits 2 Files Changed 11

+540 -24

2 Commits

Author	SHA1	Message	Date
TimothyandClaude Opus 5	3500bf3727	fix(880): move the empty-set rejection into the handlers, where the stored row is ... Independent cold review (Codex, cross-family) returned BLOCKED on the first implementation with three findings. All three were verified against the code before acting; all three were real. 1. HIGH -- an unchanged legacy empty set was rejected, making the row uneditable. api.ffmpeg-profile-numeric-bounds requires rejecting a NEWLY submitted bad value and accepting an unchanged one, and that binds harder here than in the ffmpeg case: both PUT paths are WHOLE-LIST replaces, so one pre-existing empty row made every OTHER item in the playout unsaveable. The docs I wrote even claimed legacy rows stayed editable, which the code did not do. The comparison needs the STORED row, which the controller does not have. 2. MEDIUM -- the alternate-schedule catch-all was falsely rejected. The handler discards the highest-Index item's recurrence along with its date range, so an empty set there cannot make anything "never apply"; the 422 stated a reason that is false for exactly that item. The ValidateDateRanges precedent I cited is precedent for a STYLE, not evidence the behaviour is correct -- and my own rejection tests used a one-item (i.e. catch-all) request, so they pinned the defect instead of the rule. 3. MEDIUM -- the message said "omit the field", but making a C# property nullable does not make it optional: all three are still in `required` in v1.json with type ["null","array"], so a client generated from the published contract cannot omit them. It now says "send null", which is contract-legal. Findings 1 and 2 share one root -- validation was in the wrong layer -- so both are fixed by the same move. The rejection is now RecurrenceSetBounds (ErsatzTV.Application/Scheduling), one validator called from both replace handlers, which is the FFmpegProfileBounds shape that api.ffmpeg-profile- numeric-bounds already prescribes. The handler has `existing` loaded, so the exemption is free; and it derives the validated set from `incoming` -- the exact list whose recurrence the writes below iterate -- so the catch-all is excluded by construction rather than by restating "which item is the catch-all" in a second place (api.put-replace-index-order). The exemption is per FIELD, not per row, and a stored NULL is not exempt, because null means unrestricted. Normalization is unchanged and stays in the request records. 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 (the second because the rejection then names DaysOfWeek instead of MonthsOfYear). Rejection tests assert the version did NOT bump, so a 422 that had already mutated cannot pass. One fixture trap worth the comment it got: the template handler's `.ThenInclude(t => t.Template)` navigation is REQUIRED, so a PlayoutTemplate seeded without its Template row is joined out of `existing` entirely -- the stored row goes invisible and the exemption test fails claiming the rule is broken when the fixture is. refs #880 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 10:46:43 +02:00
TimothyandClaude Opus 5	4466ea894c	fix(880): an absent recurrence array means unrestricted, an explicit [] is rejected ... The three recurrence arrays on the two playout write paths are consumed CONJUNCTIVELY by AlternateScheduleSelector.GetScheduleForDate -- a miss on any one `continue`s -- so the empty set is the maximally RESTRICTIVE value, not a neutral one. `?? []` on an omitted array therefore read as "default to no filter" and meant "default to matching nothing": the API returned HTTP 200 and stored an alternate-schedule or template item that could never apply on any date, with nothing logged. The read side had already been decided the other way in #823 (a NULL column reads as the All*() sets), so the two halves of "this field is absent" pointed in opposite directions -- residual (3) of media.nullable-primitive-collection-mutation. Absent and explicitly-empty are two different requests and get two answers: - ABSENT (missing, or an explicit `null` -- Newtonsoft maps both to null and they are indistinguishable) normalizes to AlternateScheduleSelector.All*(), the SAME symbols the selector's null guard and both Mapper.ProjectToViewModel overloads already substitute. Requires List<T>? on the request records, hence the per-file `#nullable enable` the sibling request records already use. - EXPLICIT [] is rejected with a 422 naming the consequence, per api.ffmpeg-profile-numeric-bounds -- not rewritten into the permissive value, which would re-collapse the distinction in the other direction and make "no days" inexpressible. Checked before committing to the normalization, because it is a write-path semantics change: the SPA's generated type is `null | Array<T>` (nullable, not optional) and PlayoutScheduleEditors.tsx always sends the key, and ErsatzTV.Mcp/ToolCatalog.cs explicitly defers these replace-list writes. So no existing client takes the absent path -- the trap was reachable only by a direct API client omitting a field. Rejection is uniform across items, including the highest-Index catch-all whose recurrence the handler discards. Exempting it would restate "which item is the catch-all" in the controller, a second copy of a rule that lives in the handler (api.put-replace-index-order); ValidateDateRanges already makes exactly this choice for the date fields it equally discards. Prod was measured before choosing to reject rather than grandfather an existing empty row: ProgramScheduleAlternate and PlayoutTemplate both hold ZERO rows (jazz /config/ersatztv.sqlite3, 2026-08-30), so no stored [] can be bricked and the ffmpeg-profile compare-against-stored exemption was not needed. Both halves mutation-proved by execution, separately, against the real predecessor clause: reverting the `?? All*()` normalization alone reddens exactly the 3 normalization tests; removing the two ValidateRecurrenceSets call sites alone reddens exactly the 6 rejection cases. Disjoint sets, so the guards are not masking each other. OpenAPI and the generated SPA client are byte-identical before and after -- a non-nullable-annotated List<T> was already emitted as nullable -- so the contract shape did not move, only the behaviour behind it. fixes #880 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 10:25:45 +02:00