Files
atomic-design-poc/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-23.md
T
ehoandClaude Opus 5 d0fda08bcc 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>
2026-08-27 19:01:06 +02:00

18 KiB

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.