fix(brief): make GET /brief a pure query, 404 when absent (RB-23)
GET /brief allocated a row on first call (BriefStore.GetOrCreate) — the one endpoint in the backend where a read performed a persisted write. The FE retries GETs automatically, so a transient failure could enter the create path more than once; a lock prevented a duplicate row, but the safety depended on the lock, not on the endpoint being a query. Split GetOrCreate into Get (a pure query) and the already-existing ResetAndCreate (POST /brief/reset owns creation). GET /brief now 404s when the owner has no brief yet. GET /brief/preview used GetOrCreate too, so it gets the same Get + 404 treatment, forced by the split. RB-22 already made BriefStore.load() on the FE tolerate a 404 by calling reset() once; this ticket is what makes that branch live. Updated the brief/preview/org-template backend tests that assumed GET seeded a brief on first call to create one explicitly first, and added a test that GET 404s and writes no row without the fix (verified red beforehand). Regenerated the API client (npm run gen:api). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,183 @@
|
||||
# RB-23 — `GET /brief` 404s when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 ·
|
||||
`99-backlog.md` RB-23, "Tickets that were rejected and split" · `implementation/rb-22.md`
|
||||
(the FE **expand** half this ticket **contracts** against)
|
||||
|
||||
This is the **contract** half of the RB-22/RB-23 expand/contract pair. RB-22 shipped first
|
||||
and made `BriefStore.load()` tolerate a 404 by calling `reset()` once, as a no-op against
|
||||
the (then) still-seeding backend. This ticket is what makes that branch live: `GET /brief`
|
||||
now 404s when the owner has no brief yet, and the endpoint no longer performs a persisted
|
||||
write on a read.
|
||||
|
||||
## What was wrong
|
||||
|
||||
CQ-007 flagged `GET /brief` (`Program.cs:676` → `BriefStore.GetOrCreate`,
|
||||
`Data/BriefStore.cs:50`) as the one endpoint in the backend where a GET performs a
|
||||
persisted write, breaking the read/write split every other endpoint respects. The FE
|
||||
retries GETs automatically (`api-client.provider.ts`, `retry({ count: 2, delay: 500 })`,
|
||||
GET-only, precisely because GETs are assumed safe), so a transient failure could enter the
|
||||
create path more than once; `GetOrCreate`'s `lock` prevented a duplicate row today, but the
|
||||
safety depended on the lock rather than on the endpoint being a query.
|
||||
|
||||
The ticket read as filed against the current code: `GetOrCreate` was exactly at
|
||||
`BriefStore.cs:50`, `GET /brief` called it exactly as described, and `ResetAndCreate`
|
||||
already existed and was already the sole body of `POST /brief/reset`. One thing the
|
||||
ticket's own text did not mention: `BriefStore.GetOrCreate` had a **second** call site,
|
||||
`GET /brief/preview` (`Program.cs:769`, excluded from the OpenAPI doc — a hand-written FE
|
||||
`fetch`, same seam as uploads). Splitting `GetOrCreate` away necessarily touches that
|
||||
call site too, or the file does not compile. See "What changed" below — this was a forced
|
||||
consequence of the split, not a new business decision, and it is reported here rather than
|
||||
silently worked around.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| `backend/src/BigRegister.Api/Data/BriefStore.cs` | `GetOrCreate` removed. New `Get(string owner): BriefEntity?` — pure query, `lock`-guarded like every other method in this file for consistency, no write. `ResetAndCreate` is untouched. |
|
||||
| `backend/src/BigRegister.Api/Program.cs` | `GET /brief`: calls `BriefStore.Get`; returns `Results.NotFound()` when null, `Results.Ok(ToView(ctx, e))` otherwise; declares `.Produces(StatusCodes.Status404NotFound)` (the same bare-404 pattern already used at 17 other call sites in this file). `GET /brief/preview`: same `Get` + 404 treatment — forced by the split (see above), not a scope decision made independently. |
|
||||
| `backend/src/BigRegister.Api/Data/AppDbContext.cs` | One comment updated (`GetOrCreate's invariant` → `ResetAndCreate's invariant`) — the unique index on `Owner` it annotates is unchanged. |
|
||||
| `backend/tests/BigRegister.Tests/BriefEndpointTests.cs` | New `Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner` (the DoD-required test). The `Get()` seeding helper, used by nearly every other test in the file, renamed to `SeedBrief()` and changed to create the brief explicitly via `POST /brief/reset` instead of relying on `GET /brief`'s old side effect. One test renamed (`Get_creates_a_draft_with_expected_sections_locked_and_empty` → `SeedBrief_creates_a_draft_with_expected_sections_locked_and_empty`) — it asserts on the shape of a freshly created brief, which is now `SeedBrief()`'s job, not `GET`'s. |
|
||||
| `backend/tests/BigRegister.Tests/PreviewEndpointTests.cs` | Two tests explicitly create the brief (`POST /brief/reset`) before hitting `/brief/preview`, instead of relying on the old `GET /brief` implicit create. |
|
||||
| `backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs` | Five call sites (three bare seeding `GetAsync` calls, two `GetFromJsonAsync<BriefViewDto>` calls used as seeding) changed to an explicit `POST /brief/reset` first. One call site (`Sent_brief_keeps_its_pinned_template_after_a_republish`, reading a brief already created and sent by the shared `WalkBriefToSentThenRepublish` helper) needed no change — a brief already exists by the time it runs. |
|
||||
| `backend/tests/BigRegister.Tests/RouteInventoryTests.cs` | Two `AllowList` reason strings updated (`GetOrCreate` → `Get`, 404 noted) — documentation text only, not itself a check the test enforces beyond "some reason is on record". |
|
||||
| `e2e/brief-v2.spec.ts` | One header comment updated to name the current methods and to state explicitly that this spec's own first click ("Opnieuw beginnen (demo)") is fixture setup, not a workaround for the new 404 — see "e2e and seeding paths" below. |
|
||||
| `libs/shared/src/infrastructure/api-client.ts` | Regenerated (`npm run gen:api`). `briefGET()` gains a `status === 404` branch. See "The generated client" below for the shape it actually took. |
|
||||
| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-23's status cell: `open` → `implemented`. |
|
||||
|
||||
No `apps/ssp/src/app/brief/**` file was touched — RB-22's `BriefStore.load()` recovery and
|
||||
`BriefAdapter.load()`'s `BriefLoadFailure`/`isHttpNotFound` are unchanged, per this
|
||||
ticket's explicit scope.
|
||||
|
||||
## The generated client
|
||||
|
||||
RB-22's handoff note predicted `briefGET()` would regenerate "throwing the parsed
|
||||
`ProblemDetails` (matching the shape most other endpoints already use)". That did not
|
||||
happen, and the actual result is still correct. `Results.NotFound()` (this ticket's
|
||||
implementation, and the pattern used at every one of the 17 other bare-404 call sites in
|
||||
`Program.cs` — none of them use `ProducesProblem`/a typed body) declares a 404 with **no**
|
||||
response body schema. With nothing to parse into, NSwag emits a generic branch that throws
|
||||
a plain `SwaggerException` carrying `status: 404` — the same shape `briefGET()` already
|
||||
threw before this ticket, for the same reason (no declared 404 body). `BriefAdapter.load()`'s
|
||||
`isHttpNotFound` predicate (`(e as {status?:unknown}).status === 404`) already tolerates
|
||||
both a `SwaggerException` and a parsed `ProblemDetails`, by design, precisely so this
|
||||
detail would not matter — RB-22's own comment says as much. No FE follow-up was needed, and
|
||||
none was made.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **`GET /brief/preview` also moved off `GetOrCreate`, to `Get` + 404.** Not mentioned in
|
||||
the ticket text, but unavoidable: `GetOrCreate` no longer exists once split, and this
|
||||
was its only other caller. The alternative — leaving a private, undocumented
|
||||
`GetOrCreate`-shaped helper only for this one endpoint — would have reintroduced
|
||||
exactly the GET-writes-on-read pattern CQ-007 is about, in the one place nobody would
|
||||
think to look for it. Returning 404 there too keeps both `/brief` GETs behaving the
|
||||
same way. In the running app this is unreachable in practice: the preview button
|
||||
only renders inside the brief page's `@if (loaded(); as s)` block
|
||||
(`apps/ssp/src/app/brief/ui/brief.page.ts`), which by construction only shows once
|
||||
`BriefStore.load()` has already succeeded — including via RB-22's 404-recovery branch.
|
||||
So a brief always exists by the time a real user can trigger `/brief/preview`; the 404
|
||||
path there is a defensive consequence of the type split, not a new user-facing
|
||||
behaviour anyone will hit.
|
||||
- **`BriefStore.Get` keeps the `lock (_gate)` wrap**, even though a plain SQLite read
|
||||
does not strictly need the same mutual exclusion a write does. Every other method in
|
||||
this file, including the pre-existing `ApplicationStore.Get`-style query in the
|
||||
sibling store, locks unconditionally — matching that convention was judged more
|
||||
valuable than a lock-free read this ticket did not need to justify removing.
|
||||
- **Existing test changes create the brief via `POST /brief/reset`, not a new
|
||||
`BriefStore.Get`/`ResetAndCreate` direct call from the test.** Going through the HTTP
|
||||
endpoint (as the old `Get()` helper always did) keeps the tests exercising the real
|
||||
request pipeline (identity resolution, `ToView` mapping) rather than reaching around
|
||||
it — the same reasoning that already justified an `IClassFixture<TestWebApplicationFactory>`
|
||||
HTTP-level test suite in the first place.
|
||||
|
||||
## e2e and seeding paths
|
||||
|
||||
- **`e2e/brief-v2.spec.ts`** is the only e2e spec that reaches `/brief`. It already opens
|
||||
`/brief?role=drafter` and immediately clicks "Opnieuw beginnen (demo)" (`POST
|
||||
/brief/reset`) before asserting anything — a deliberate fixture reset, not a
|
||||
workaround. With this ticket live, the page's first `GET /brief` on the fresh
|
||||
per-run database (WP-74) now 404s; RB-22's `BriefStore.load()` recovers from that by
|
||||
calling `reset()` once, so the page still renders correctly, and the spec's own
|
||||
explicit reset click still runs on top of that (harmless — resetting an
|
||||
already-fresh brief). No behavioural change to the spec was needed; one comment was
|
||||
updated to say this explicitly rather than leave it to be re-derived.
|
||||
- **Storybook**: no `brief.page.stories.ts` exists, and none of the eleven `brief/ui/**`
|
||||
component stories call `HttpClient`/`fetch`/`ApiClient` — every story supplies data
|
||||
through component `input()`s, per the house convention (design-system/component
|
||||
stories are not live-network integration tests). Nothing in Storybook depended on
|
||||
`GET /brief`'s old seeding behaviour.
|
||||
|
||||
## The double round-trip — verdict
|
||||
|
||||
CQ-007 named this its least certain point: a first-ever visit to `/brief` now costs a 404
|
||||
followed by a `reset()` call, instead of one request that both creates and returns the
|
||||
brief. **Shipped as-is; the cost is acceptable.** Three reasons:
|
||||
|
||||
1. **It happens once per browser tab, ever, for one demo entity.** `BriefStore`'s
|
||||
`hasRecoveredFromMissingBrief` flag (RB-22) makes the 404 unreachable again for the
|
||||
life of the store instance; a real deployment has one brief per zorgverlener, created
|
||||
the first time that person ever opens the page. This is not a cost paid on every
|
||||
page load, or even every session — a page reload still 404s once if the flag reset
|
||||
with the page, but the underlying row is already there by then, so the _second_ call
|
||||
in the pair — `reset()` — is now hitting an existing row rather than truly
|
||||
first-creating one, and returns just as fast as `Get` would have.
|
||||
2. **An extra round-trip is not an extra spinner.** `BriefStore.load()`'s failure
|
||||
handling for `notFound` calls `reset()` and applies the result through the same
|
||||
`applyLoadedView` the success path uses — there is no intermediate "not found" UI
|
||||
state rendered to the user between the two calls; the page shows its loading state
|
||||
once, for the combined duration of both requests.
|
||||
3. **The alternative was rejected, not merely deprioritized.** CQ-007's own
|
||||
documentation-only alternative — leave `GetOrCreate` in place, just write down that
|
||||
the GET seeds on first call — was rejected outright by agent 07 in `99-backlog.md`:
|
||||
"a non-idempotent GET must be visible in the code, not only in a ticket." Given that,
|
||||
the only way to remove the mixing is some version of this two-call shape; a
|
||||
single-call alternative would mean either GET creates (the defect) or `POST
|
||||
/brief/reset` runs unconditionally on load (destructive — it deletes an existing
|
||||
brief, unacceptable for anyone with real content already saved).
|
||||
|
||||
## The once-only guard's lifetime — re-verified
|
||||
|
||||
RB-22 flagged this as worth re-checking once a real 404 could occur in production
|
||||
traffic, not only in a test's fake adapter. Having now made the 404 real: `hasRecoveredFromMissingBrief`
|
||||
is a private field on `BriefStore`, which is `providedIn: 'root'` — one instance per
|
||||
browser tab (per CLAUDE.md's "shared cross-page state = one root singleton" convention),
|
||||
reset only by a full page reload. That lifetime is still correct for what the flag
|
||||
guards: it exists to stop a _second, separate_ `load()` call in the same tab session from
|
||||
re-triggering `reset()` (e.g. a caller retrying navigation after the first recovery
|
||||
already ran) — not to remember "this owner has a brief" across reloads or across owners,
|
||||
which is the server's job (`BriefStore.Get` returning non-null). A page reload correctly
|
||||
starts the guard over: the first `load()` after a reload will find the now-existing row
|
||||
via a plain `GET` (no 404, no `reset()` call at all), so the flag never actually gets
|
||||
exercised a second time in the reload case either. No FE change was needed or made.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Verified red without the fix.** Temporarily (via `Edit`, never `git checkout`)
|
||||
restored `BriefStore.GetOrCreate` alongside the new `Get`, and pointed `GET /brief` in
|
||||
`Program.cs` back at `GetOrCreate`. Ran the new test alone:
|
||||
```
|
||||
BigRegister.Tests.BriefEndpointTests.Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner [FAIL]
|
||||
Assert.Equal() Failure: Values differ
|
||||
Expected: NotFound
|
||||
Actual: OK
|
||||
```
|
||||
Restored the real fix with a second `Edit` (removed the temporary `GetOrCreate`,
|
||||
pointed `GET /brief` back at `Get` + 404) and reran: green.
|
||||
- Full backend suite after the fix: **262/262 passing**, plus the one known,
|
||||
pre-existing, container-dependent failure
|
||||
(`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
"Connection refused (localhost:8000)") — not this ticket's bug, does not run under
|
||||
`npm run ci`, reproduces on a clean tree with no OpenZaak container running.
|
||||
- `npm run gen:api`: the client changed (`libs/shared/src/infrastructure/api-client.ts`,
|
||||
`briefGET()` gains a `status === 404` branch — 4 lines). Regenerated and committed;
|
||||
see "The generated client" above for why the shape differs from RB-22's prediction and
|
||||
why that difference is harmless.
|
||||
- `npm run ci` (foreground, no background/Monitor): see result below.
|
||||
|
||||
## What this ticket did not touch
|
||||
|
||||
`apps/ssp/src/app/brief/application/brief.store.ts`, `brief.store.spec.ts`, and
|
||||
`apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` are unchanged — RB-22's FE logic
|
||||
was already correct and already tested against exactly this contract, per this ticket's
|
||||
explicit scope.
|
||||
Reference in New Issue
Block a user