Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m31s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m40s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Docs update reminder (push) Has been skipped
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 4m34s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 5m34s
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 3m54s
# Conflicts: # docs/decisions.md
514 lines
39 KiB
Markdown
514 lines
39 KiB
Markdown
# API conventions — "Add a REST endpoint" checklist
|
||
|
||
Purpose: a precise, file-path-and-exemplar checklist for adding or changing a `/api/*` endpoint on
|
||
the ErsatzTV fork, so an agent with no prior context in this repo can do it correctly on the first
|
||
pass. **Update this doc in the same PR that changes any convention below.**
|
||
|
||
This is a companion to `docs/contributing.md` (general CQRS/LanguageExt/testing conventions) and
|
||
`docs/ci-cd.md` (build/release pipeline) — read those first for anything not API-specific.
|
||
|
||
## 1. Controller shape
|
||
|
||
Controllers live in `ErsatzTV/Controllers/Api/*.cs`, one per domain (e.g. `TemplateController.cs`,
|
||
`BlockController.cs`, `LogsController.cs`). Every action needs this attribute set:
|
||
|
||
- `[ApiController]` on the class.
|
||
- `[HttpGet/Post/Put/Delete("/api/...")]` on the action, with `Name = "..."` on at least the
|
||
primary GET (used by the SPA's OpenAPI-generated client and by route-assertion tests).
|
||
- `[Tags("Domain")]` — groups the endpoint in Swagger UI / the SPA's generated client namespace.
|
||
- `[EndpointSummary("...")]` — one-line description; optionally `[EndpointDescription("...")]` for
|
||
more detail.
|
||
- `[EndpointGroupName("general")]` — **REQUIRED**. Endpoints without it are excluded from the
|
||
generated OpenAPI document entirely (verified: every action in `TemplateController.cs` and
|
||
`LogsController.cs` has it; there is no other endpoint group in use).
|
||
- `[ProducesResponseType(typeof(X), StatusCodes.StatusYyy)]` for every status code the action can
|
||
actually return (200/201/204 success, plus 404/422 as applicable) — this drives the generated
|
||
TypeScript response types on the SPA side.
|
||
|
||
Exemplars:
|
||
- **Full CRUD controller**: `ErsatzTV/Controllers/Api/TemplateController.cs` — group CRUD +
|
||
item CRUD + copy, all four verbs, `ToCreatedResult`/`ToErrorResult` usage.
|
||
- **Paged GET with clamped params**: `ErsatzTV/Controllers/Api/LogsController.cs` —
|
||
`pageNum` clamped via `Math.Max(0, pageNum)`, `pageSize` via `Math.Clamp(pageSize, 1, MaxPageSize)`
|
||
(`MaxPageSize = 100`). Any new paged endpoint should clamp the same way — don't trust client
|
||
input for page math.
|
||
- **Sortable GET with allow-listed sort params**: same file — `sortField`/`sortDirection` are
|
||
normalized against a fixed allow-list (`AllowedSortFields`) rather than trusted or rejected with
|
||
a 422: an unrecognized `sortField` silently falls back to the default field, an unrecognized
|
||
`sortDirection` falls back to the default direction. Copy this pattern (normalize, don't 422) for
|
||
any new sortable endpoint — it matches the pageNum/pageSize clamp precedent above and keeps a bad
|
||
query string from ever producing an error response for a read-only listing.
|
||
|
||
## 2. DTOs: where they live and their nullable context
|
||
|
||
- **Response DTOs**: `ErsatzTV.Core/Api/<Domain>/*ResponseModel.cs`. They are `record`s that mirror
|
||
the shape of the Application-layer ViewModel for that domain — **never expose a VM type directly**
|
||
from a controller. Convert framework types the SPA can't consume as-is (e.g. `CultureInfo` →
|
||
its name string).
|
||
- Most response-model files start with `#nullable enable` (verified: 60 of 72 files under
|
||
`ErsatzTV.Core/Api/`, e.g. `ErsatzTV.Core/Api/Scheduling/BlockResponseModel.cs`,
|
||
`ErsatzTV.Core/Api/Settings/*ResponseModel.cs`). A minority of the simplest ones (e.g.
|
||
`FFmpegProfileResponseModel.cs`) have no pragma and rely on the project default. Check
|
||
`ErsatzTV.Core/ErsatzTV.Core.csproj` — the project sets `<Nullable>disable</Nullable>` — so
|
||
**add `#nullable enable` at the top of any new response-model file** that has an optional
|
||
(nullable) member; don't rely on the project default.
|
||
- **Request DTOs**: `ErsatzTV/Controllers/Api/Requests/*Request.cs`. No `#nullable enable` pragma —
|
||
match the existing files (e.g. `CreateFFmpegProfileRequest.cs`, `ReplaceScheduleItemsRequest.cs`).
|
||
A request record typically carries a `ToCommand()` (or `ToCommand(int parentId)`,
|
||
`ToReplaceCommand(int index)`) method that maps it to the Application-layer command type.
|
||
- **`ErsatzTV.Application` has no nullable context** (no `<Nullable>` = C# default `disable` for
|
||
that TFM in this repo — confirms CS8632 would otherwise fire) — do **not** add `?` nullable
|
||
annotations to types living there; that's a Core/Api-layer-only convention. A static mapper that
|
||
lives in `ErsatzTV.Application` but returns a Core/Api response DTO with nullable members is fine
|
||
(e.g. `ScheduleItemResponseMapper`) — the nullability lives on the DTO record, not the mapper.
|
||
- **Shared `{id, name}` embeds**: use `ErsatzTV.Core/Api/NamedIdResponseModel.cs`
|
||
(`record NamedIdResponseModel(int Id, string Name)`) when a response DTO needs to embed a list of
|
||
named references (e.g. a schedule item's `watermarks` / `graphicsElements`) rather than minting a
|
||
one-off `(int, string)` record per domain.
|
||
- **Flatten polymorphic VMs for the wire**: when an Application ViewModel is an abstract/polymorphic
|
||
record (subtypes carrying extra fields), the OpenAPI schema only captures the base shape — promote
|
||
every subtype field to a nullable top-level member on a flat response DTO and pattern-match the
|
||
concrete VM in the mapper. Exemplar: `ScheduleItemResponseModel` (issue #126, see
|
||
`docs/decisions.md` 2026-07-10). Keep the flat DTO's **mutation** fields named 1:1 with the
|
||
matching request DTO so GET→PUT is lossless (guard with a round-trip handler test).
|
||
- **Optional enum filter via query param**: to filter a list endpoint by an enum, add a nullable
|
||
enum parameter to the query record (default `null`) and bind it with `[FromQuery] TEnum? name` on
|
||
the action; filter server-side only when it has a value. Exemplar: `?fillerKind=` on
|
||
`GET /api/filler-presets` (`GetAllFillerPresetsForApi(FillerKind? FillerKind = null)`). An invalid
|
||
enum value is rejected by model binding (400) — no handler-side guard needed.
|
||
|
||
## 3. Error mapping
|
||
|
||
Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of
|
||
hand-rolling `IActionResult` status codes:
|
||
|
||
| Method | Input | Output |
|
||
|---|---|---|
|
||
| `ToErrorResult()` | `BaseError` | 404 if `NotFoundError`, **412 if `PreconditionFailedError`** (optimistic-concurrency mismatch, §7a), else 422 (`ProblemDetails`) |
|
||
| `ToCreatedResult(location, body)` | `Either<BaseError, T>` | `Left` → `ToErrorResult()`; `Right` → 201 + `Location` header |
|
||
| `ToUpdatedResult()` | `Either<BaseError, T>` | `Left` → `ToErrorResult()`; `Right` → 200 + body |
|
||
| `ToDeletedResult()` | `Either<BaseError, Unit>` | `Left` → `ToErrorResult()`; `Right` → 204 |
|
||
| `ToGetResult()` | `Option<T>` | `Some` → 200 + body; `None` → 404 |
|
||
| `ApiResults.NotFoundProblem(detail?)` | — | 404 `ProblemDetails` directly (e.g. when a controller has to pre-check existence itself, see `TemplateController.DeleteGroup`) |
|
||
| `ApiResults.ConflictProblem(title, detail)` | — | 409 `ProblemDetails` directly — for a mutation that races a background operation holding a lock (see §3a) |
|
||
|
||
### 3a. 409 when a mutation races a background lock
|
||
|
||
When an endpoint mutates an entity that a background operation may be actively rebuilding under an
|
||
`IEntityLocker` lock, guard the mutation and return **409 Conflict** (`ApiResults.ConflictProblem`)
|
||
while the lock is held. This mirrors the Blazor UI, which disables the same actions while the lock
|
||
event is live.
|
||
|
||
**This guard is advisory check-then-act, not mutual exclusion.** It narrows the race but does not
|
||
eliminate it: a build already queued can acquire the lock a moment *after* the check passes, and the
|
||
mutation then interleaves with the build anyway. That residual window is accepted where the
|
||
consequences are self-healing (a playout half-mutated during a build is corrected by the next
|
||
rebuild). If an entity's consequences were NOT self-healing, this pattern would be insufficient —
|
||
the mutation would need to actually acquire the lock for its duration instead.
|
||
|
||
Established by issue #215 (`PlayoutController` + `ChannelController.ResetPlayout`): inject
|
||
`IEntityLocker`, and at the top of every id-keyed mutation (`PUT`/`POST`/`DELETE`) check
|
||
`IsPlayoutLocked(id)` → `ConflictProblem("Playout build in progress", ...)`; add
|
||
`[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)]` to each guarded
|
||
action. Precedent for the 409 shape: `TraktController` (its private `ConflictProblem()`). Two nuances:
|
||
- **Fire-and-forget bulk operations don't 409** — `POST /api/playouts/reset-all` stays 202; its
|
||
handler (`ResetAllPlayoutsHandler`) *skips* locked playouts, matching Blazor + the handler
|
||
semantics. Only per-id mutations 409. As of #235 the handler returns a `ResetAllPlayoutsResult`
|
||
(`QueuedPlayoutIds` / `SkippedLocked` / `SkippedUnsupported`) and the controller returns the 202
|
||
**with a `ResetAllPlayoutsResponseModel` body** reporting what was queued vs. skipped (locked, or
|
||
an unsupported `ExternalJson`/`None` kind) — a fire-and-forget bulk op still reports its outcome
|
||
rather than silently swallowing skips.
|
||
- **Surface the lock state to clients** so they can pre-disable the buttons: stamp an `IsLocked`
|
||
boolean onto the list DTO (`PlayoutListItemResponseModel`, set from `IsPlayoutLocked` in the
|
||
controller's list projection) rather than adding a push channel — and (as of #235) onto the
|
||
single-playout GET DTO (`PlayoutResponseModel.IsLocked`, set the same way in every action that
|
||
maps it) so a client polling one playout has the same flag. The SPA reads it and, on a 409,
|
||
refreshes to pick up the flag.
|
||
- **Async-op success is 202, not 200** — an endpoint whose success path only *queues* a background
|
||
rebuild returns **202 Accepted**, not 200 (#235: `POST /api/channels/{channelNumber}/playout/reset`
|
||
queues a `BuildPlayout` → `AcceptedResult`). Reserve 200 for a synchronous durable result.
|
||
|
||
### 3b. Map a "queue a background job" outcome to status codes with an enum, not a `bool`
|
||
|
||
When an endpoint *starts* a background operation guarded by an `IEntityLocker` lock, return an
|
||
**outcome enum from the handler** and map it in the controller — don't collapse distinct outcomes
|
||
into a lying `bool`/200. Exemplar (issue #232): `QueueLibraryScanByLibraryId` →
|
||
`QueueLibraryScanResult { Queued | NotFound | SyncDisabled | AlreadyScanning }`, mapped by
|
||
`LibrariesController.ScanLibrary` to **202** (`AcceptedResult`, queued), **404**
|
||
(`ApiResults.NotFoundProblem`), **422** (`UnprocessableEntityObjectResult` + `ProblemDetails`, a
|
||
domain precondition such as sync-disabled), and **409** (`ApiResults.ConflictProblem`, the lock is
|
||
already held = already scanning). Here the acquired lock **is** the running job, so
|
||
`LockLibrary(id) == false` means "already scanning" → 409 (a variant of §3a where the lock is the
|
||
operation itself, not a mutation racing it). Add `[ProducesResponseType]` for 202/404/409/422 and
|
||
`typeof(ProblemDetails)` on the error ones. **Guard the lock→enqueue** with the
|
||
`EnqueueWithTraktLock` compensating-unlock pattern (`TraktController`): if a `WriteAsync` throws
|
||
after a successful `Lock*`, `Unlock*` in a `catch` and rethrow — one lock ⇄ exactly one release.
|
||
|
||
A second exemplar (issue #235 slice B), where the lock lives on the **controller** rather than in a
|
||
handler: `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=` acquires the
|
||
per-source collections lock (`entityLocker.LockPlexCollections()` etc.) — the lock IS the running
|
||
collections scan, so a `false` = 409 — then `WriteAsync`es `Synchronize{X}Collections(id, ForceScan:
|
||
true, deep)` to the scanner channel and returns **202**. `ScannerService` releases that lock in a
|
||
`finally` when it processes the message; the controller compensating-unlocks in a `catch` if the
|
||
enqueue throws. `POST /api/libraries/{id}/scan?deep=` similarly threads an optional `[FromQuery] bool
|
||
deep` into `QueueLibraryScanByLibraryId(id, DeepScan)`.
|
||
|
||
`NotFoundError : BaseError` lives in `ErsatzTV.Core/Errors/NotFoundError.cs` — return it from a
|
||
handler's validation when a lookup fails, so the controller-side mapping falls out for free.
|
||
|
||
**Handler-hardening checklist** when you touch (or add) a handler behind a new/changed endpoint:
|
||
- Missing FK existence checks that would otherwise 500 → convert to a `NotFoundError` (404) or a
|
||
validation `BaseError` (422).
|
||
- Dictionary-indexer lookups (`dict[key]`) that can throw `KeyNotFoundException` → guard or use
|
||
`TryGetValue`.
|
||
- Silent item filtering (e.g. `.Where(x => x.IsValid)` dropping bad rows without telling the
|
||
caller) → surface as 422 instead of silently returning a shorter list.
|
||
- Unbounded `int`/`TimeSpan` inputs from the request → clamp or validate, per the Logs pagination
|
||
pattern above.
|
||
- **Known, deliberate exception**: deep FK ids nested inside item-list request bodies (e.g. a
|
||
schedule item's `CollectionId`) are **not** existence-checked at that depth — this is established
|
||
precedent from the schedules endpoints (see issue #172) and intentional to avoid N+1 validation
|
||
queries; don't "fix" this without discussing it first.
|
||
|
||
### 3c. A durable-save PUT that also triggers a background sync as a side effect
|
||
|
||
Some PUT-replace endpoints (issue #202: `PUT /api/media-sources/{plex|jellyfin|emby}/{id}/libraries`)
|
||
have a synchronous durable write as their primary purpose — the response must reflect that write —
|
||
but Blazor's editor also fired off a background sync per newly-enabled library after the save. This
|
||
is a **different shape from §3b**: §3b is for an endpoint whose entire job *is* starting a
|
||
background operation (so the outcome enum drives the status code); here the enqueue is a fire-and-
|
||
forget side effect of an otherwise-ordinary write, and the response must still be **200 with the
|
||
durably-saved data**, not 202.
|
||
|
||
Pattern (see `PlexMediaSourcesController.ReplaceLibraryPreferences` / `EnqueuePostSaveSync`):
|
||
1. Dispatch the write command; on `Left`, return the error — nothing is enqueued.
|
||
2. **Reload** the saved rows through the same query the GET uses (per §7) — needed both for the
|
||
response and because ids can change as a result of the save (e.g. a disable-then-re-add).
|
||
3. Iterate the reloaded rows and, per row that needs a sync, `LockLibrary(id)` — **skip rows whose
|
||
lock is already held** (mirrors Blazor's `if (Locker.LockLibrary(id))` loop; no 409 for the save
|
||
itself, only a silent skip for that one row's sync).
|
||
4. Enqueue the background message(s) for that row; if a multi-message enqueue can throw partway
|
||
through, wrap it in `try/catch` and release the lock in the `catch` (compensating unlock) before
|
||
rethrowing — the standard `EnqueueWithTraktLock` one-lock-⇄-one-release discipline (§3b), just
|
||
invoked from inside a write endpoint instead of a dedicated "start background job" endpoint.
|
||
5. Return the reloaded data with `200 OK` — never 202 — since the durable save already happened;
|
||
the enqueue is best-effort and its failure (after a caught/compensated exception) surfaces as a
|
||
500 on this same request rather than silently dropping the sync.
|
||
|
||
This is server-side (not SPA-orchestrated) because the SPA has no access to the scanner channel,
|
||
and having the SPA fire a second request after the save would open a crash window between "saved"
|
||
and "synced". See `docs/decisions.md` 2026-07-11 (#202) for the fuller rationale and the specific
|
||
bug this pattern corrected (Blazor's Plex `Unlock` ordering released the library lock before a
|
||
dependent second message ran).
|
||
|
||
## 4. Artwork contract
|
||
|
||
API response DTOs return **rooted, directly-usable artwork URLs** — e.g. `/artwork/posters/...`,
|
||
`/artwork/thumbnails/...`, plus passthrough for `http://`/`https://` absolute URLs and for
|
||
Jellyfin/Emby proxy variants. This was established by PR #181 in
|
||
`ErsatzTV.Application/LibraryBrowse/Queries/GetLibraryBrowseItemsHandler.cs` (private `Artwork(...)`
|
||
helper, ~line 1330) — copy that helper's logic (or call a shared version of it) for any new
|
||
API surface that returns artwork paths. Comment in that file: *"Returns a rooted, directly-usable
|
||
artwork URL for the SPA's `<img src>`. Blazor pages rely on GetPosterUrl to prefix
|
||
`artwork/posters/`... but the SPA [needs it pre-rooted]."*
|
||
|
||
Do **not** reuse the Application-layer Mappers used by Blazor (e.g. `MediaCards`/`Television`
|
||
mappers) for new API DTOs — those still return the old Blazor-convention relative paths. Map from
|
||
the domain/VM directly and root the path yourself, following the PR #181 pattern.
|
||
|
||
### 4a. Artwork content type is sniffed, never client-supplied (issue #283)
|
||
|
||
The uploaded-artwork surfaces (channel logo, watermark) must never trust a client-declared content
|
||
type — doing so was a stored-XSS chain (upload `<script>` as `image/png`, serve it back as
|
||
`text/html`). The contract:
|
||
- **Upload**: `POST /api/artwork/uploads` derives the content type from the actual bytes via
|
||
`ErsatzTV.Core/Images/ImageContentTypes.DetectContentType` (SkiaSharp header sniff — no full
|
||
decode), rejecting non-images 422. That helper (`Accepted` set + `IsAccepted`) is the **single
|
||
source of truth** for which image types are allowed — reuse it, don't re-list content types.
|
||
- **Serve**: the image routes derive the served `Content-Type` from the stored file; there is **no**
|
||
`?contentType=` query parameter. Never add one back — a client must not be able to choose the
|
||
`Content-Type` of an unauthenticated response.
|
||
- **Persisted `{path, contentType}` DTOs** (logo/watermark) run their content type through
|
||
`ArtworkContentTypeModel.Sanitized()` before storage, blanking anything outside the allow-list.
|
||
See `docs/decisions.md` 2026-07-12 (#283) for the fuller rationale.
|
||
|
||
## 5. OpenAPI regeneration — commit both generated artifacts
|
||
|
||
After any controller/DTO change:
|
||
1. `dotnet build ErsatzTV.sln` (normal build first).
|
||
2. `./scripts/update-openapi.sh` — runs `dotnet build -t:GenerateOpenApiDocuments` from `ErsatzTV/`,
|
||
regenerating `ErsatzTV/wwwroot/openapi/v1.json`.
|
||
3. `cd web && npm run generate:api` — runs `scripts/generate-openapi-types.mjs`, regenerating
|
||
`web/src/api/generated/v1.d.ts`.
|
||
|
||
**Commit both** `ErsatzTV/wwwroot/openapi/v1.json` and `web/src/api/generated/v1.d.ts` — the `.d.ts`
|
||
file is what the SPA actually imports (`web/src/api/*.ts` files do
|
||
`import type { components } from './generated/v1'`), and CI enforces it stays in sync
|
||
(`npm run check:api` = regenerate + `git diff --exit-code`).
|
||
|
||
**Known type-generation wart**: `DayOfWeek` serializes as an integer in the OpenAPI schema but the
|
||
runtime JSON payload is actually the enum's **name string** ("Sunday".."Saturday") — the generator
|
||
gets this wrong. The SPA works around it with a manual override type; see `web/src/api/playouts.ts`
|
||
(`type WithDayNames<T> = Omit<T, 'daysOfWeek'> & { daysOfWeek: DayOfWeek[] }`, applied to
|
||
`PlayoutAlternateSchedule`, `PlayoutTemplate`, and their request types). Copy this pattern for any
|
||
new DTO with a `DayOfWeek` (or `DayOfWeek[]`) member — don't trust the generated numeric type.
|
||
|
||
### 5a. Runtime JSON casing vs the generated spec (the `ffmpegProfileId` wart)
|
||
|
||
Runtime `/api/*` JSON is serialized by **Newtonsoft** (`AddNewtonsoftJson` in `Startup.cs`), using
|
||
`ErsatzTV/Serialization/CustomContractResolver.cs` → `CustomNamingStrategy` (camelCase **plus** a
|
||
special case mapping any `FFmpegProfileId` member to `"ffmpegProfileId"`, and honoring any
|
||
`[JsonProperty("...")]` attribute, e.g. `ChannelResponseModel.FFmpegProfile` →
|
||
`[JsonProperty("ffmpegProfile")]`). The OpenAPI document, however, is generated from
|
||
**System.Text.Json** metadata, whose camelCase can differ (it emitted `fFmpegProfileId` /
|
||
`fFmpegProfile`). That drift silently gave the SPA the wrong key to read (issue #198).
|
||
|
||
Fix (do not remove): `ErsatzTV/Serialization/NewtonsoftSchemaNamingTransformer.cs` is an OpenAPI
|
||
**schema transformer** registered on all three documents (`options.AddSchemaTransformer(...)` in
|
||
`Startup.cs`). For each object schema it resolves the CLR type's Newtonsoft `JsonObjectContract`
|
||
through the *same* `CustomContractResolver` the runtime uses and renames `schema.Properties` (and
|
||
`schema.Required`) keys to the exact names Newtonsoft would emit. This mirrors the wire format **by
|
||
construction**, so future naming-strategy special cases or `[JsonProperty]` renames can't drift.
|
||
|
||
Guard: `ErsatzTV.Tests/Controllers/OpenApiSerializerContractTests.cs` serializes fully-populated
|
||
DTOs (ChannelViewModel, FFmpegSettingsResponseModel, WatermarkViewModel,
|
||
MediaItemInfoResponseModel) through the runtime Newtonsoft settings and asserts the emitted top-level
|
||
keys equal the corresponding `v1.json` schema's property set. It fails if spec generation ever drifts
|
||
from the MVC serializer again. Note: only exact-match `FFmpegProfileId` gets the special case —
|
||
`DefaultFFmpegProfileId` stays `defaultFFmpegProfileId` under both serializers, and non-acronym or
|
||
single-leading-cap names (`fFmpegPath`, `fFprobePath`, `zIndex`, `rFrameRate`) already agree.
|
||
|
||
## 6. Tests
|
||
|
||
- **Controller tests**: `ErsatzTV.Tests/Controllers/<Domain>ControllerTests.cs`. NUnit + Shouldly +
|
||
NSubstitute (mock `IMediator`). Exemplar: `ErsatzTV.Tests/Controllers/TemplateControllerTests.cs`
|
||
— asserts every route via a `ShouldHaveActionRoute(actionName, verb, path)` helper (route-table
|
||
regression net), then per-action tests asserting the DTO shape returned and the exact
|
||
`mediator.Received(1).Send(Arg.Is<Command>(...))` call. `PlayoutControllerTests.cs` is the same
|
||
pattern for a larger, mixed-verb controller — use it as the template for a new controller with
|
||
many actions.
|
||
- **`ApiControllerSecurityTests.cs`** (`ErsatzTV.Tests/Controllers/`): reflects over the
|
||
`ErsatzTV.Controllers.Api` namespace to find every concrete, `[ApiController]`-marked
|
||
controller class — no manual registry to maintain; a new controller is covered
|
||
automatically. (It intentionally does **not** filter on `ControllerBase`: several API
|
||
controllers — including the lone exempt `ScannerController` — do not derive from it, and a
|
||
`ControllerBase` filter would silently drop them.) The test walks every mutating (POST/PUT/PATCH/DELETE) action on each scanned
|
||
controller and asserts it's covered by the global `ApiKeyAuthorizationFilter` (or has an
|
||
explicit `[SkipApiKeyAuthorizationAttribute]` exemption; only `ScannerController` is exempt
|
||
today). A minimum-count guard asserts the scan found a sane number of controllers, so a
|
||
namespace rename can't silently make the scan match nothing and give this test a false pass.
|
||
It also asserts the **sensitive-read tier** (`Troubleshoot`/`Logs`/`Settings`/`Maintenance`)
|
||
carries `[RequiresApiKey]` and that `ScannerController` carries `[LocalhostOnly]` — reflectively,
|
||
so the tier can't silently drop a gate. `ApiKeyAuthorizationFilterTests.cs` unit-tests the filter
|
||
itself: writes are always fail-closed, reads are gated when `Api:RequireKeyForReads` (default true)
|
||
or `[RequiresApiKey]`, `OPTIONS` preflight and non-`/api` paths are exempt.
|
||
- **Handler tests** (business logic behind the controller) use the shared in-memory SQLite fixture:
|
||
`ErsatzTV.Tests/Support/InMemoryTvContext.cs`. Pattern: `SqliteConnection("Data Source=:memory:;Foreign
|
||
Keys=False")` kept open for the fixture's lifetime, `EnsureCreatedAsync()` (not full migration
|
||
replay), then `PRAGMA foreign_keys=OFF` so partial object graphs can be seeded without satisfying
|
||
every FK. Exemplar: `ErsatzTV.Tests/Application/LibraryBrowse/GetLibraryBrowseItemsHandlerTests.cs`
|
||
(`_db = await InMemoryTvContext.CreateAsync();` in `[SetUp]`, `_db.CreateContext()` per test body,
|
||
`_db.Factory` where an `IDbContextFactory<TvContext>` is needed by the handler under test).
|
||
|
||
## 7. PUT-replace list endpoints
|
||
|
||
For "replace the whole list" endpoints (PUT semantics over a collection, e.g. schedule/template
|
||
items), index items from **array order** in the request body rather than trusting a client-supplied
|
||
index/order field. Exemplar: `ReplaceScheduleItemsRequest.ToCommand(scheduleId)` —
|
||
`Items.Select((item, index) => item.ToReplaceCommand(index))`.
|
||
|
||
**Project the write-path response through the same include chain the GET uses — never off the
|
||
freshly-built graph.** After `SaveChanges`, a command's entities carry only the foreign-key ids you set
|
||
(e.g. `ProgramScheduleItemWatermark.WatermarkId`); their reference navs are null, and any mapper that
|
||
dereferences one unguarded throws an NRE that surfaces as a 500. Reload with the read-side includes
|
||
before mapping. Exemplars: `ReplaceProgramScheduleItemsHandler` / `AddProgramScheduleItemHandler` reload
|
||
via `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` (the one include chain shared with
|
||
`GetProgramScheduleItemsHandler`). Also beware LanguageExt `Map` is **lazy** — returning
|
||
`items.Map(ProjectToViewModel)` defers the projection, so a test that only checks `.IsRight` won't catch
|
||
the NRE; the controller's `.ToList()`/serialization does (regression: `ScheduleItemWriteProjectionTests`).
|
||
GET handlers that feed an ordered list must also `.OrderBy(i => i.Index)` — id order is not index order.
|
||
|
||
## 7a. PUT-replace concurrency (ETag / If-Match / 412)
|
||
|
||
The replace-all aggregate PUTs (blocks, templates, schedules items, playlists, collections, playouts,
|
||
etc.) carry an **optimistic-concurrency contract** so a stale second tab can't silently overwrite a
|
||
fresher edit (issue #253). The Block endpoints are the reference implementation; **PR2 fanned the same
|
||
recipe onto Template, DecoTemplate, Playlist, and schedule-items** (`PUT /api/templates/{id}`,
|
||
`/api/deco-templates/{id}`, `/api/playlists/{id}`, `/api/schedules/{id}/items`); PR3 covers the
|
||
Diff/Scalar aggregates (Collection, Playout ×2, MultiCollection, RerunCollection) and PR4 is the Phase-2
|
||
428 flip.
|
||
|
||
Each replace PUT keeps its **own** existing 200 body shape (Template/DecoTemplate return a
|
||
`…WithItemsResponseModel`, Block likewise; Playlist and schedule-items return the item array) and adds the
|
||
ETag as a **header only** — except schedules: `ScheduleController` returns the Application-layer
|
||
`ProgramScheduleViewModel` *directly* (no `ProgramScheduleResponseModel` exists), so the added
|
||
`int Version` also surfaces as a redundant `version` field in the `GET /api/schedules[/{id}]` bodies. That
|
||
is intentional and harmless (the ETag remains the authority); introducing a ResponseModel purely to hide
|
||
one field was judged disproportionate. Every replace PUT's *sibling config writers* bump `Version` too
|
||
(Playlist: the five `Add*ToPlaylist` handlers; schedule: `AddProgramScheduleItem` / `DeleteProgramScheduleItem`
|
||
/ `UpdateProgramSchedule`; Template/DecoTemplate have none).
|
||
|
||
**Token.** Each versioned root implements `IVersionedAggregate` (`int Version`, EF-mapped with
|
||
`.IsConcurrencyToken()` in its `IEntityTypeConfiguration`). A single dual-provider migration
|
||
(`AddAggregateVersions`) adds the column (`nullable: false, defaultValue: 0`). Do **not** overload the
|
||
existing `DateUpdated` — a plain `int` is portable across SQLite/MySQL and decoupled from UI cosmetics.
|
||
|
||
**Transport.** The aggregate's GET (the one the editor loads from — e.g. `GET /api/blocks/{id}/items`)
|
||
emits a strong `ETag: "3"` of `Version`; the PUT sends it back as `If-Match: "3"`. Mismatch → **412
|
||
Precondition Failed** (distinct from the §3a **409** "build in progress" lock guard). A successful PUT
|
||
returns the **new** ETag (post-increment) so a same-tab second save doesn't 412 against its own write.
|
||
`If-Match: *` and (Phase 1) a missing header force-write; a non-canonical/weak/list/malformed header
|
||
→ 400 (fail-safe; the stricter RFC 7232 "valid-but-non-matching tag → 412" refinement is deferred to
|
||
#197 — see #265). Parse/emit with `ErsatzTV.Extensions.ConcurrencyHeaders` (`ParseIfMatch` → `IfMatchCondition.ExpectedVersion : Option<int>`,
|
||
`SetETag`). The items GET returns *children*, so the controller reads `root.Version` separately for the
|
||
header (here `BlockViewModel` carries `Version`, projected but **not** echoed in the response body —
|
||
header-only).
|
||
|
||
**Handler recipe (the error-prone part).** Introduce the concurrency check as a **standalone `Either`
|
||
AFTER** the validation pipeline, never via `Apply` — `LanguageExtensions.Apply`/`ToEither` `Join()` a
|
||
`Seq<BaseError>` down to a base `BaseError`, which would flatten `PreconditionFailedError` to a 422. The
|
||
reference shape (`ReplaceBlockItemsHandler`):
|
||
|
||
```csharp
|
||
Either<BaseError, Block> validated = LanguageExtensions.ToEither(validation) // explicit: the native
|
||
.Bind(block => block.CheckVersion(request.ExpectedVersion)); // Validation.ToEither() shadows ours
|
||
return await validated.Match(
|
||
Right: block => Persist(dbContext, request, block, cancellationToken),
|
||
Left: error => Task.FromResult<Either<BaseError, Unit>>(error));
|
||
```
|
||
|
||
In `Persist`, bump **unconditionally** before saving — `root.Version++` — because EF emits the root
|
||
UPDATE only when a scalar actually differs, so a same-value/no-op PUT-back would otherwise neither fire
|
||
the token nor rotate other clients' ETags. Then save through
|
||
`dbContext.SaveChangesWithConcurrencyGuard(ct)` (maps `DbUpdateConcurrencyException` → 412), which is the
|
||
backstop that closes the load→save TOCTOU the pre-check can't. `CheckVersion` (pure, on
|
||
`IVersionedAggregate`) and `SaveChangesWithConcurrencyGuard` live in `ErsatzTV.Core` /
|
||
`ErsatzTV.Application` respectively.
|
||
|
||
Add `[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)]` and `…412…` to the
|
||
PUT action. **Config-only boundary**: every mutating handler of an aggregate's *editor-visible config
|
||
state* bumps `Version` (incl. bulk `ExecuteUpdate/Delete` writers, which add
|
||
`.SetProperty(x => x.Version, x => x.Version + 1)`); regenerated build output (playout items/history) is
|
||
outside the token — its handlers neither bump nor are guarded. Test the reference with: stale-If-Match →
|
||
412 (no mutation), matching/absent → success + bump, no-op save still bumps, and a two-context racing
|
||
save → 412 (prove it non-vacuous by dropping `.IsConcurrencyToken()` and watching the race test fail).
|
||
Phase 2 (a later PR) flips a missing `If-Match` from force-write to **428 Precondition Required** once
|
||
every editor echoes and one release soaks.
|
||
|
||
**Fan-out status.** Block (#2) is the reference. **PR2** wired the RR + Reconcile aggregates
|
||
(Template #3, DecoTemplate #4, Playlist #5, schedule-items #1). **PR3** wired the Diff + Scalar aggregates:
|
||
Collection custom-order #6, Playout alternate-schedules #7 and templates #8 (both share `Playout.Version`;
|
||
their `catch(Exception)`→422 handlers were restructured so the guard's `PreconditionFailedError` Left
|
||
returns before the catch — §9/H1), MultiCollection #9, and RerunCollection #10. Two M2 gate notes for the
|
||
`SaveChangesAsync() > 0` handlers: the RerunCollection/Collection-custom-order refresh now runs on any
|
||
successful save (the bump makes the gate always-true); MultiCollection keeps its "name-only change → no
|
||
playout rebuild" optimization by bumping on the **first** (name) save so the **second** (items) save's
|
||
`> 0` still means "items changed". The one bulk writer in scope, `UpdateDefaultDecoHandler`, bumps via
|
||
`.SetProperty(x => x.Version, x => x.Version + 1)` (bulk `ExecuteUpdate` can't throw the concurrency
|
||
exception, so it needs no guard). **Deferred (tracked follow-up):** the *other* same-root config
|
||
writers — non-bulk siblings like `UpdateCollectionHandler`/`RemoveItemsFromCollectionHandler`,
|
||
`UpdatePlayoutHandler` and the `ScheduleFile` handlers, and the repository-mediated `Add*ToCollection`
|
||
family — do **not** yet rotate their aggregate's ETag. The primary endpoints' own bump+guard fully cover
|
||
the two-tab lost-update this contract targets; the deferred writers only affect cross-editor ETag rotation,
|
||
and adding an unconditional bump to a handler that uses plain `SaveChangesAsync` (not the guard) would open
|
||
a new `DbUpdateConcurrencyException`→500 path — so they need a uniform guard+bump pass of their own.
|
||
|
||
## 7b. Post-commit side effects run on `CancellationToken.None`
|
||
|
||
Once a command handler's `await dbContext.SaveChangesAsync(cancellationToken)` (or repository upsert)
|
||
has **committed**, everything that runs afterwards to complete that mutation's side effect —
|
||
`channel.WriteAsync(new BuildPlayout(...))` / other worker-channel enqueues, `mediator.Publish(...)`,
|
||
`ISearchIndex`/reindex enqueues, a cache `Refresh(...)`, and any post-commit **lookup that gates one
|
||
of those enqueues** — must be passed **`CancellationToken.None`**, not the request `cancellationToken`.
|
||
|
||
Rationale (audit #22, issues #251 → #254): the request token is cancelled when the HTTP client
|
||
disconnects. If it's threaded into a post-commit enqueue, a late disconnect turns an *already-durable*
|
||
commit into a thrown request **and drops the side effect** (e.g. the rebuild is never queued → the
|
||
persisted change silently never takes visible effect). The commit is the point of no return: past it,
|
||
the compensating side effect must not be half-abortable. Exemplar idiom:
|
||
`ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs` (the affected-playout queries **and** the
|
||
`WriteAsync(BuildPlayout..., CancellationToken.None)` enqueue all use `None`).
|
||
|
||
Two boundaries:
|
||
- **Response projection is NOT a side effect.** The post-commit reload that builds the *returned* view
|
||
model (§7 above) legitimately keeps the request `cancellationToken` — if the client disconnected, we
|
||
don't need to compute a response nobody will read, and the durable work (commit + `None`-enqueue) has
|
||
already happened. Only the *side effect* chain gets `None`.
|
||
- **Background-job handlers keep their token.** `BuildPlayoutHandler` and other handlers invoked by the
|
||
worker (not by an HTTP request) receive the *worker's* shutdown token, not a client-disconnect token —
|
||
their downstream enqueues correctly stay on that token so a shutdown stops enqueuing more work.
|
||
|
||
Pre-commit reads/validation and the `SaveChangesAsync` call itself keep the request token (cancelling
|
||
*before* the commit safely aborts with nothing persisted). A handler that passes **no** token to a
|
||
post-commit `WriteAsync()` is already behaviorally correct (`default` == `CancellationToken.None`);
|
||
making it explicit is optional cleanup, not required. Config handlers that commit via several sequential
|
||
`IConfigElementRepository.Upsert` calls are a distinct partial-commit-under-cancellation case not covered
|
||
by this rule (tracked separately).
|
||
|
||
## 7c. Stable child identity in replace lists (schedule items)
|
||
|
||
§7 indexes replace-list children by **array order**, which is the reconcile *key* for most replace PUTs.
|
||
That is correct only when a child row is pure config: reordering merely re-numbers otherwise-interchangeable
|
||
rows. **Schedule items are the exception** (issue #259): a schedule item anchors persisted runtime state —
|
||
`PlayoutScheduleItemFillGroupIndex` (fill-group / shuffle enumerator progression) FKs the item row with
|
||
`OnDelete(Cascade)`. Reconciling those by position makes a **moved** item inherit the state of whatever item
|
||
previously occupied its new slot. So `PUT /api/schedules/{id}/items` carries a stable child identity:
|
||
|
||
- `ScheduleItemRequest.Id` (`int?`) round-trips each existing item's server id (as returned by the items GET).
|
||
**null / absent / 0 ⇒ a new item** (the controller normalizes `0`→null so the handler contract is
|
||
two-state). Never fabricate an id.
|
||
- When **any** request item carries an id, `ReplaceProgramScheduleItemsHandler` reconciles **by id**: matched
|
||
same-subtype rows are updated in place (keeping the id, so the fill-group index never cascades and follows
|
||
the logical item across reorders/inserts); a matched row whose TPT subtype changed is delete+insert (state
|
||
resets, a **new id is returned** — clients must re-sync from the PUT response); unreferenced existing rows are
|
||
deleted; id-less request items are inserted. `Index` is still array-position (ordering is a separate axis
|
||
from identity).
|
||
- **A fully id-less payload falls back to the verbatim positional reconcile** (legacy clients). This preserves
|
||
today's misattribution-on-reorder for such payloads — it is temporary and retires together with the §7a
|
||
Phase-2 `If-Match`→428 flip.
|
||
- **Guards run inside the handler, after the §7a `CheckVersion`** (so a client that is both version-stale and
|
||
id-stale gets **412**, the reload signal, not 422): a duplicate id in one payload → **422**; an id not
|
||
belonging to this schedule → **422** (under Phase-1 force-write a stale id is a live lost-update signal, not
|
||
a new item — reject rather than silently duplicate). Both persist nothing.
|
||
|
||
**Deliberate asymmetry**: the other positional replace handlers (blocks #2, templates #3, deco-templates #4,
|
||
playlists #5) do **not** carry a child id — their children are stateless config rows where positional churn is
|
||
unobservable (#3/#4 don't even emit a child id on GET). Child ids are added only where a child row anchors
|
||
server-side state; positional replace stays the default. A per-endpoint child-id contract can be retrofitted
|
||
later without breaking anything (the field stays optional).
|
||
|
||
## 8. Known API warts (don't "fix" without discussion — they're deliberate synthesized rows)
|
||
|
||
`GET /api/blocks` and `GET /api/templates` (via `GetAllBlocksHandler` / `GetAllTemplatesHandler` in
|
||
`ErsatzTV.Application/Scheduling/Queries/`) synthesize a fake **negative-id "(none)" group row**
|
||
for items that have no group, so the SPA can render an "ungrouped" bucket
|
||
(`Id = unusedGroup.Id * -1`, `Name = "(none)"`). See issue #172. If you add a similar "ungrouped"
|
||
concept elsewhere, this is the established pattern to follow — but be aware it means `Id` is not a
|
||
reliable real-entity id for those synthetic rows.
|
||
|
||
## 9. Authentication — API key posture (fail-closed)
|
||
|
||
The whole `/api` surface is gated by the `X-Api-Key` header via the global `ApiKeyAuthorizationFilter`
|
||
(issue #197 Bundle A). When you add an endpoint:
|
||
|
||
- **Do nothing** for the common case. Writes (POST/PUT/PATCH/DELETE) always require the key
|
||
(fail-closed — there is no "open" mode). Reads (GET/HEAD) require the key when
|
||
`Api:RequireKeyForReads` is enabled, which is the **default** (`true`). `OPTIONS` preflight is exempt.
|
||
- The effective key comes from `IApiKeyProvider` (`ErsatzTV/Services/ApiKeyProvider.cs`): `Api:WriteKey`
|
||
if configured, else a key persisted at `FileSystemLayout.ApiKeyPath` (`/config/api.key`, `0600`), else
|
||
a freshly generated 256-bit key. It is never empty.
|
||
- **Sensitive-read GETs** that disclose secrets/paths or trigger work must carry `[RequiresApiKey]` so
|
||
they stay gated even if an operator sets `Api:RequireKeyForReads=false`. Current tier:
|
||
`Troubleshoot`/`Logs`/`Settings`/`Maintenance`. `ApiControllerSecurityTests` asserts this reflectively.
|
||
- **Internal loopback callbacks** (the scanner's `/api/scan/*`) use `[SkipApiKeyAuthorization]` +
|
||
`[LocalhostOnly]` — the API key is a poor fit for a co-located child process, so the gate is the
|
||
loopback check (sound only because `ForwardedHeaders` trust is restricted via
|
||
`ForwardedHeaders:KnownProxies`/`KnownNetworks`).
|
||
- **CORS** is opt-in: no cross-origin access by default (the SPA is same-origin from `/app`); set
|
||
`Api:CorsAllowedOrigins` (semicolon-separated exact origins) to allow specific browser origins — the
|
||
policy already permits `X-Api-Key`/`If-Match` and exposes `ETag`.
|
||
- The SPA sends the stored key (`ctv-api-key`) on **every** request; users enter it on the keyless
|
||
**API Key** screen (`web/src/screens/ApiKeyScreen.tsx`, route `/app/api-key`). See spa-conventions §5e.
|
||
|
||
The declarative OpenAPI security scheme + global 401 documentation are Phase-2 (#286/#287), not this tier.
|