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

Scripted-playout _off: the documented "null -> all off" behaviour depends on an unrecorded formatter setting, and no test would catch it changing #837

New Issue
Open
opened 2026-08-26 19:06:53 +02:00 by timothy · 0 comments
timothy commented 2026-08-26 19:06:53 +02:00
Owner
Copy Link
Copy Source

Found while folding the Scripted Schedule API table into the canonical ersatztv skill (#755).
This issue has been rewritten twice as measurement refuted successive premises — the history is
kept below so nobody re-derives the dead ends.

  • Originally filed as: _off handlers NRE on an explicit JSON null. Refuted.
  • Then rewritten as: they are null-safe only because of NullValueHandling.Ignore. Also refuted —
    there is a second, stronger mechanism.

What is actually true (measured 2026-08-26)

ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs dereferences the request collection with no null
guard — if (watermarks.Count == 0) _state.ClearChannelWatermarkIds(); — and GraphicsOff is the
identical shape. But .Count is never reached with a null over HTTP, under any configuration.

Two independent mechanisms stop it, and the stronger one is not a serializer setting:

  1. ErsatzTV/Startup.cs:506-512 — AddNewtonsoftJson with NullValueHandling.Ignore, which applies
    on deserialization, so an explicit null is skipped and the DTO's = [] initialiser stands.
  2. MVC implicit [Required]. The DTOs live in ErsatzTV.Core.Nullable, whose csproj sets
    <Nullable>enable</Nullable>, so public List<string> Watermark is a non-nullable reference type
    and MVC synthesises a required-ness check. Neither
    SuppressImplicitRequiredAttributeForNonNullableReferenceTypes nor SuppressModelStateInvalidFilter
    is set anywhere in the solution, so model validation rejects the payload before the action body runs.

Measured in a minimal ASP.NET Core reproduction (net10.0, same DTO shape, [ApiController],
Microsoft.AspNetCore.Mvc.NewtonsoftJson), POSTing each payload under all three formatter configs:

[newtonsoft        ] explicit null  -> HTTP 200 | BOUND-LIST count=0
[newtonsoft-default] explicit null  -> HTTP 400 | {"errors":{"Watermark":["The Watermark field is required."]}}
[stj               ] explicit null  -> HTTP 400 | "One or more validation errors occurred."

  omitted key and empty list -> HTTP 200, count=0, in ALL THREE configurations

So: no NRE, no 500. The alternatives give a 400 at model validation.

The residual worth tracking

Small, but real:

  • A documented behaviour is config-dependent. ControlWatermarkOff's [Description] promises
    "All (scripted) watermarks will be turned off if this list is null or empty". That holds today only
    because of mechanism 1. Swap the input formatter — to System.Text.Json, the .NET default and the
    direction most projects move — and a documented success becomes a 400 rejection. That is a
    behaviour-contract regression, not a crash, and nothing at either site records the dependency.
  • The unguarded .Count is still a defensive-coding smell for any non-HTTP caller of
    WatermarkOff/GraphicsOff, which do not get MVC's validation for free.
  • The test trap, which is the most reusable part of this: a test that OMITS the key passes under
    all three configurations and proves nothing. Only an explicit "watermark": null distinguishes them,
    and no test sends one.

Done-when

  • A test sends an explicit JSON null (not an omitted key) and pins the documented
    "null or empty -> all off" behaviour, so a formatter change fails loudly instead of silently
    converting a 200 into a 400
  • The dependency is recorded at the site that relies on it — either a comment on
    Startup.cs's NullValueHandling.Ignore naming what breaks without it, or the handlers made
    null-safe on their own terms
  • The population of scripted-playout handlers dereferencing a request collection without a null
    guard is derived from source (not from this issue's file list) and recorded, including any
    deliberately left alone with the reason
  • Adversarial review passed
Found while folding the Scripted Schedule API table into the canonical `ersatztv` skill (#755). **This issue has been rewritten twice as measurement refuted successive premises** — the history is kept below so nobody re-derives the dead ends. - Originally filed as: `_off` handlers NRE on an explicit JSON `null`. **Refuted.** - Then rewritten as: they are null-safe only because of `NullValueHandling.Ignore`. **Also refuted** — there is a second, stronger mechanism. ## What is actually true (measured 2026-08-26) `ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs` dereferences the request collection with no null guard — `if (watermarks.Count == 0) _state.ClearChannelWatermarkIds();` — and `GraphicsOff` is the identical shape. But **`.Count` is never reached with a null over HTTP**, under any configuration. Two independent mechanisms stop it, and the stronger one is not a serializer setting: 1. `ErsatzTV/Startup.cs:506-512` — `AddNewtonsoftJson` with `NullValueHandling.Ignore`, which applies on deserialization, so an explicit null is skipped and the DTO's `= []` initialiser stands. 2. **MVC implicit `[Required]`.** The DTOs live in `ErsatzTV.Core.Nullable`, whose csproj sets `<Nullable>enable</Nullable>`, so `public List<string> Watermark` is a non-nullable reference type and MVC synthesises a required-ness check. Neither `SuppressImplicitRequiredAttributeForNonNullableReferenceTypes` nor `SuppressModelStateInvalidFilter` is set anywhere in the solution, so model validation rejects the payload before the action body runs. Measured in a minimal ASP.NET Core reproduction (`net10.0`, same DTO shape, `[ApiController]`, `Microsoft.AspNetCore.Mvc.NewtonsoftJson`), POSTing each payload under all three formatter configs: ``` [newtonsoft ] explicit null -> HTTP 200 | BOUND-LIST count=0 [newtonsoft-default] explicit null -> HTTP 400 | {"errors":{"Watermark":["The Watermark field is required."]}} [stj ] explicit null -> HTTP 400 | "One or more validation errors occurred." omitted key and empty list -> HTTP 200, count=0, in ALL THREE configurations ``` So: **no NRE, no 500.** The alternatives give a 400 at model validation. ## The residual worth tracking Small, but real: - **A documented behaviour is config-dependent.** `ControlWatermarkOff`'s `[Description]` promises *"All (scripted) watermarks will be turned off if this list is null or empty"*. That holds today only because of mechanism 1. Swap the input formatter — to `System.Text.Json`, the .NET default and the direction most projects move — and a documented success becomes a **400 rejection**. That is a behaviour-contract regression, not a crash, and nothing at either site records the dependency. - **The unguarded `.Count` is still a defensive-coding smell** for any **non-HTTP** caller of `WatermarkOff`/`GraphicsOff`, which do not get MVC's validation for free. - **The test trap, which is the most reusable part of this:** a test that OMITS the key passes under all three configurations and proves nothing. Only an explicit `"watermark": null` distinguishes them, and no test sends one. ## Done-when - [ ] A test sends an explicit JSON `null` (not an omitted key) and pins the documented "null or empty -> all off" behaviour, so a formatter change fails loudly instead of silently converting a 200 into a 400 - [ ] The dependency is recorded at the site that relies on it — either a comment on `Startup.cs`'s `NullValueHandling.Ignore` naming what breaks without it, or the handlers made null-safe on their own terms - [ ] The population of scripted-playout handlers dereferencing a request collection without a null guard is derived from source (not from this issue's file list) and recorded, including any deliberately left alone with the reason - [ ] Adversarial review passed
timothy added the apipriority: medium labels 2026-08-26 19:07:03 +02:00
timothy changed title from Scripted-playout `_off` handlers NRE on an explicit JSON `null`, and their OpenAPI description promises the opposite to Scripted-playout `_off` handlers are null-safe only because of a global Newtonsoft setting — undocumented and untested coupling 2026-08-26 19:14:09 +02:00
timothy changed title from Scripted-playout `_off` handlers are null-safe only because of a global Newtonsoft setting — undocumented and untested coupling to Scripted-playout `_off`: the documented "null -> all off" behaviour depends on an unrecorded formatter setting, and no test would catch it changing 2026-08-26 19:21:13 +02:00
timothy added priority: low and removed priority: medium labels 2026-08-26 19:21:28 +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 api priority: low
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#837