feat(api): optimistic-concurrency contract for replace-all PUTs — PR1 infra + Block reference (#253)
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m24s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m25s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m24s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m25s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Adds the shared optimistic-concurrency contract so a stale second tab can no longer silently overwrite a fresher edit. PR1 lands the infra + the Block reference aggregate; PRs 2–4 fan the same recipe across the other 8 roots (design: #253#issuecomment-8472). Contract - `IVersionedAggregate` (`int Version`) on all 9 replace-all roots (ProgramSchedule, Block, Template, DecoTemplate, Playlist, Collection, Playout, MultiCollection, RerunCollection), EF-mapped `.IsConcurrencyToken()`; one dual-provider migration `AddAggregateVersions` (nullable:false, default 0). - Strong `ETag` of `Version` on the aggregate GET; `If-Match` on the PUT; mismatch → 412 (distinct from the §3a 409 build-lock guard). Successful PUT returns the new ETag. - `PreconditionFailedError : BaseError` → 412 in `ApiResults.ToErrorResult`; `ConcurrencyHeaders.ParseIfMatch/SetETag`; malformed If-Match → 400; `*`/absent = Phase-1 force-write. Block reference wiring - Handler: standalone `Either` via `CheckVersion` AFTER validation (never through `Apply`, which Join()-flattens the subtype to 422), unconditional `Version++`, `SaveChangesWithConcurrencyGuard` backstop (DbUpdateConcurrencyException → 412). - `BlockViewModel.Version` (header-only, not echoed in the body); controller sets the ETag on GET items and on the successful PUT. - SPA: `client.requestWithMeta` seam; `blocks.getBlockItemsWithMeta` + `replaceBlock` If-Match/ETag round-trip; `BlockEditor` holds the ETag, sends If-Match, and on 412 opens a blocking "changed elsewhere — reload" dialog. Tests - Handler contract tests: stale-If-Match → 412 (no mutation), matching/absent → success + bump, no-op save still bumps, and a two-context racing save → 412; proven non-vacuous (drop `.IsConcurrencyToken()` → the race test fails). - Controller tests: malformed If-Match → 400, If-Match threaded to the command, ETag on GET/PUT, 412 passthrough. SPA: requestWithMeta ETag, replaceBlock If-Match, 412 dialog. Docs: api-conventions §7a, spa-conventions §4a, domain-model glossary, decisions log. Refs #253 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+54
-1
@@ -84,7 +84,7 @@ hand-rolling `IActionResult` status codes:
|
||||
|
||||
| Method | Input | Output |
|
||||
|---|---|---|
|
||||
| `ToErrorResult()` | `BaseError` | 404 if `NotFoundError`, else 422 (`ProblemDetails`) |
|
||||
| `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 |
|
||||
@@ -258,6 +258,59 @@ via `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` (the one i
|
||||
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; PRs 2–4 fan the same
|
||||
recipe across the other aggregates.
|
||||
|
||||
**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 malformed header → 400. 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.
|
||||
|
||||
## 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
|
||||
|
||||
Reference in New Issue
Block a user