Reject unknown channel graphicsElementIds with 422; discriminate builtIn by full seeded path #922
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#922
Reference in New Issue
Block a user
fixes #568
refs #74, #917, #921
Root cause
UpdateChannelHandler.Validatechecked name/number/EPG/logo/group but never validated theincoming
graphicsElementIds. The reconcile blindlyAdded aChannelGraphicsElementfor anysubmitted id, so
PUT /api/v1/channels/{id}with a non-existent id hitFK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId->DbUpdateException-> anunhandled 500 (data-safe, the transaction rolled back, but inconsistent with every other FK field
on the same full-replace DTO).
Separately,
GetAllGraphicsElementsForApiHandlerandGraphicsElementSeederkeyed thebuiltIndiscriminator off
Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileNamealone —filename-only, so a user element named exactly
on-now-next.ymlin any of the other graphicsfolders (image/motion/subtitle/script) would also report
builtIn:true.Fix, as the code now stands
ErsatzTV.Application/Validators/IdListValidation.csadds a sharedIdsMustExistused byUpdateChannelHandler(graphicsElementIds, watermarkId,fallbackFillerId, mirrorSourceChannelId) and
UpdateDecoHandler's graphics-element twin —rejects unknown ids with a 422 whose
detailnames the request field(
[GraphicsElementIds] Graphics element(s) do not exist: …).IdsMustExistrejects a submitted list overValidators.MaximumIdListCount(512) beforeDistinctor any DB work — the count is taken fromthe raw list because the request pays for parsing/materializing it regardless of how many ids
are actually distinct or consumed.
MaximumReportedMissingIds(10) bounds how many rejected idsthe 422 body echoes back, so an oversized request can't produce an oversized response.
UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRacecatchesDbUpdateExceptionfrom the save (a concurrent delete of a graphics element/watermark/etc.between validation and write can still hit the FK) and re-runs the whole validator set — not
just the graphics-element half — against a fresh context, translating a real FK loss back into
the same 422 the validator would have returned. Re-running the full set (rather than naming
fields one by one) is what keeps this path from silently missing the next FK the DTO gains.
UpdateDecoHandlercarries the same twin recovery path for its own FK fields.builtIndiscriminator:GraphicsElementDefaults.IsOnNowNext(path, kind)is now the oneidentity predicate — full seeded path (
OnNowNextSeededPath, folder-qualified) comparedordinally, AND
Kind == Text— used in memory by bothGetAllGraphicsElementsForApiHandlerandGraphicsElementSeeder.SeedOnNowNext's own "does the built-in row already exist" check, so thetwo call sites can no longer disagree about the same row. Comparison stays in memory rather than
a SQL
WherebecauseGraphicsElement.Pathtakes no explicit collation, so SQLite comparescase-sensitively while MySQL would use the server default (normally case-insensitive) — pushing
the comparison into SQL would let the two providers disagree.
UpdateDecoHandlermirrors the channel handler's validation, count cap andlost-race recovery for its own graphics-element ids, with one difference: a deco's
DecoModecan make the apply path discard the graphics-element list entirely, so the existence check is
gated off in that case (
idsAreConsumed: false) while the count cap still applies unconditionally— the list is still parsed and materialized out of the request body whatever the apply path does
with it afterwards.
Gate (measured this session)
dotnet build ErsatzTV.sln -c Debug— 0 errors, 0 warnings.dotnet test ErsatzTV.Tests --no-build— Passed: 2137, Failed: 0, Skipped: 6, Total: 2143.dotnet test ErsatzTV.Core.Tests --no-build— Passed: 716, Failed: 0, Skipped: 1, Total: 717.dotnet test ErsatzTV.Infrastructure.Tests --no-build— Passed: 114, Failed: 0, Skipped: 0, Total: 114.(No
ErsatzTV.Application.Testsproject exists in this repo.).csfiles — none carry a UTF-8 BOM.dotnet format whitespace . --folder --verify-no-changes --include <touched files>— clean.scripts/check-doc-narrative.py --diff origin/main— 0 advisory warnings (non-blocking check).scripts/decisions_validate.py— OK.build_decisions_catalog.py --check— catalog up to date.Live-E2E (port 8460, measured this session)
PUT /api/v1/channels/1withgraphicsElementIds:[99999](unknown id) →HTTP 422,
detail: "[GraphicsElementIds] Graphics element(s) do not exist: 99999".PUT /api/v1/channels/1withgraphicsElementIds:[1](the seeded built-in element) →HTTP 200, echoing
graphicsElementIds:[1].Review
Three implementation runs across nine review rounds total; the final run's Codex cross-family
review ran in all three of its rounds. The last remaining Codex should-fix from that final run —
a pre-existing already-attached-principal race unrelated to this issue's two Done-when items — is
pre-existing on
mainand deferred to #921 rather than fixed here. The raw id-list count cap(now shipped, see "Fix" above) was itself deferred once earlier in the process to #917 before
being pulled back in and implemented in this branch's
9fc54fed8commit.Cross-family review status: codex — ran in rounds one to three of the final run.
Deferred
MaximumIdListCountcap inIdListValidation.csbounds thevalidators it added (graphics elements, watermark, fallback filler, mirror source, and the
deco twins) — one piece of #917's ask — but #917's own scope is repo-wide: it also names the
pre-existing, still-unbounded
ApplyUpdateRequestreconcile loop(
O(existing x desired)RemoveAll/foreach) and the other pre-existing FK validators(
FFmpegProfileMustExist,WatermarkMustExist,FillerPresetMustExist, schedule-itemequivalents) that this branch did not touch. Left open for that broader sweep and its own
decision record.
pre-existing on
main(not introduced by this branch) and is out of scope for #568's twoDone-when items; tracked there for a separate fix.
match to remove. The shipped
IsOnNowNextdiscriminator deliberately keeps the comparisoncase-sensitive (ordinal) — see
GraphicsElementDefaults.csremarks and the mutation table(row 33) that pins it — because the seeded path's casing is fixed by the seeder itself, and an
ordinal comparison is required to keep the SQLite and MySQL providers from disagreeing (SQLite
compares
GraphicsElement.Pathcase-sensitively with no explicit collation, MySQL would use itsserver default). What the fix removes is the filename-only and folder-agnostic parts of the
match; case-sensitivity is retained by design, not left unaddressed.
🤖 Generated with Claude Code
https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
UpdateChannelHandler.Validate never checked incoming graphicsElementIds against GraphicsElements, so PUT /api/v1/channels/{id} with a non-existent id hit FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surfaced as an unhandled 500. Add GraphicsElementIdsMustExist, following the existing FFmpegProfileMustExist/WatermarkMustExist/FillerPresetMustExist shape, so an unknown id now returns 422 for parity with every other FK field on this full-replace DTO. GetAllGraphicsElementsForApiHandler and GraphicsElementSeeder.GetBuiltInElementId keyed builtIn off Path.GetFileName(e.Path) == OnNowNextFileName -- folder-agnostic, so a user element named exactly on-now-next.yml in any other template folder would also report builtIn:true. Both now compare against GraphicsElementDefaults.OnNowNextSeededPath, the full path the seeder actually writes to. Follow-up from the #74 whole-branch review (2026-07-22), deferred as data-safe/not SPA-reachable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVTwo review findings, both about docs the branch already rewrote. The mutation-coverage table in docs/graphics-elements.md ended up with two rows numbered 24 -- the new IsOnNowNext row was inserted after 23 without checking what followed -- while 18 was vacated when the Kind-filter row moved to the "no red" list. The section's own prose cites rows by number ("the IsOnNowNext clause (row 10)"), so a duplicate id makes a citation ambiguous. The new row becomes 33, the next unused number, and the rule that made it 24 in the first place is now written down: a row number is an identity, not a position, so a new row takes the next unused number, nothing is renumbered, and a retired clause leaves its number vacant rather than having it reused under a new meaning. Both row claims were re-measured and are unchanged; only the id moves. #568's second half is titled "builtIn discriminator is filename-only, case-sensitive, folder-agnostic", and the branch removes the first and third while deliberately keeping case sensitivity -- which reads like two thirds of a done-when box. It is not: the remedy the same box prescribes, "full seeded relative path", is exactly as case-sensitive as the filename match it replaces, so the three adjectives describe one predicate rather than name three separable demands. Read the other way the box would be unsatisfiable by its own remedy. The reason case sensitivity is kept -- a case-INsensitive test hands the built-in identity to a user element differing from the seeded path only in case -- lived only in GraphicsElementDefaults.cs, where a reader arriving from the issue title would not find it. It is now in the active record that owns the discriminator. Refs #568 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVThe comment asserted an outcome ("also reddens if the Kind filter is dropped") with nothing tying it to a measurement -- the shape testing.mutation-claims-are- executed exists to refuse. Both clauses it covers are rows of the mutation table in docs/graphics-elements.md, measured against this tree; cite them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVReview-verdict: MERGEABLE @
fcdc381Orchestrated-session workflow: nine cold review rounds across three runs (correctness Opus high in its own worktree, conformance sonnet high); Codex cross-family review ran in every round of the final run — its last finding, a pre-existing already-attached-principal race, is deferred to #921 by the orchestrator, and the count cap to #917, both stated in the PR body and the closing record. Gate on the pushed head: ErsatzTV.Tests 2137, Core.Tests 716, Infrastructure.Tests 114 passed, 0 failed; live PUT with an unknown graphicsElementId 422 naming the field, valid PUT 200. The builtIn discriminator stays case-sensitive by design (ordinal, mutation row 33). Verdict on the pushed head.