diff --git a/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts b/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts index 9bef9c2..ad3aec3 100644 --- a/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts @@ -1,6 +1,6 @@ import { Injectable, inject } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; -import { runSubmit } from '@shared/application/submit'; +import { runResult, runSubmit } from '@shared/application/submit'; import { ApiClient, BriefDecisionsDto, @@ -33,8 +33,9 @@ import { Mark, Paragraph, RichTextBlock, RichTextNode } from '@shared/kernel/ric * The only place brief HTTP lives (ADR-0001 anti-corruption boundary). The wire * uses FLAT unions (a `type`/`tag` string + nullable fields, the repo convention); * the `parse*` boundary narrows them into the domain's proper discriminated unions - * and rejects malformed shapes. Mutations go through `runSubmit` (ProblemDetails → - * error string), then parse the returned brief. + * and rejects malformed shapes. `load` (the only read) folds through `runResult`; + * every mutation folds through `runSubmit` (ProblemDetails → error string, plus the + * Idempotency-Key mint), then parses the returned brief. */ export interface BriefView { @@ -53,7 +54,7 @@ export class BriefAdapter { private client = inject(ApiClient); async load(): Promise> { - const r = await runSubmit(() => this.client.briefGET(), BRIEF_LOAD_FAILED); + const r = await runResult(() => this.client.briefGET(), BRIEF_LOAD_FAILED); return r.ok ? parseBriefView(r.value) : r; } diff --git a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts index fef9891..339c0c2 100644 --- a/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts @@ -1,6 +1,6 @@ import { Injectable, inject, isDevMode } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; -import { runSubmit } from '@shared/application/submit'; +import { runResult, runSubmit } from '@shared/application/submit'; import { currentRole } from '@shared/infrastructure/role'; import { problemDetail } from '@shared/infrastructure/api-error'; import { environment } from '@shared/environments/environment'; @@ -39,7 +39,7 @@ export class OrgTemplateAdapter { private client = inject(ApiClient); async list(): Promise> { - const r = await runSubmit(() => this.client.orgTemplates(), FAILED); + const r = await runResult(() => this.client.orgTemplates(), FAILED); if (!r.ok) return r; const out: SubOrgSummary[] = []; for (const s of r.value ?? []) { @@ -51,7 +51,7 @@ export class OrgTemplateAdapter { } async load(subOrgId: string): Promise> { - const r = await runSubmit(() => this.client.orgTemplateGET(subOrgId), FAILED); + const r = await runResult(() => this.client.orgTemplateGET(subOrgId), FAILED); return r.ok ? parseAdminView(r.value) : r; } diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-17.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-17.md new file mode 100644 index 0000000..e38c6e5 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-17.md @@ -0,0 +1,130 @@ +# RB-17 — split `runResult` (fold) from `runSubmit` (fold + idempotency mint) + +Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-003 + CQ-005 · +`00-baseline.md` BL-007 (+ its §10 amendment) · `99-backlog.md` RB-17 + +## What was wrong + +`runSubmit` (`libs/shared/src/application/submit.ts`) did two things in one function: fold a +call into a `Result`, and mint an Idempotency-Key for it +(`withIdempotencyKey(crypto.randomUUID(), fn)`). Its own docstring called that mint "the one +place a logical submit's Idempotency-Key is minted". Five call sites are reads and had no +business minting one: + +| Adapter | Method | Wire call | +| ---------------------------- | -------------------- | --------------------- | +| `brief.adapter.ts:56` | `load()` | `briefGET()` | +| `org-template.adapter.ts:42` | `list()` | `orgTemplates()` | +| `org-template.adapter.ts:54` | `load(subOrgId)` | `orgTemplateGET(...)` | +| `stamdata.adapter.ts:27` | `list()` | `stamdataTables()` | +| `stamdata.adapter.ts:42` | `load(tableId, ...)` | `stamdataTable(...)` | + +That is **exactly five** — verified by grepping every `runSubmit` call site in +`libs/shared/src/application` plus the `brief` and `beheer` scopes (13 call sites total) and +reading each one's wire call for a request body / non-GET verb. The other 8 are genuine +writes (`brief.adapter.ts` save/submit/approve/reject/send/reset, +`org-template.adapter.ts` save/publish/rollback) and stay on `runSubmit` unchanged. + +`stamdata.adapter.ts`'s own module docstring already said "Both endpoints are reads … There +is no write method" while both called `runSubmit` — the sharpest instance of the mismatch, +and the one BL-007's original "~13 mutations" count mis-classified because the count was +derived from the helper's name, not from what the call actually does. + +**Not this ticket, seen while auditing:** `ApplicationsStore.cancel`, `AdminCasesStore.delete` +(RB-20) and `FeatureFlagStore.set` reach `ApiClient` more directly; the baseline's §10 +amendment flags these as writes the original "~13" count missed. Grepping confirms +`FeatureFlagStore.set` (`libs/shared/src/application/feature-flags.store.ts:63`) already +calls `runSubmit` correctly and returns a `Result` — it is not broken, just outside this +ticket's five. `ApplicationsStore.cancel`/`AdminCasesStore.delete` were not touched; they are +RB-20's. + +## What changed + +| File | Change | +| --------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | +| `libs/shared/src/application/submit.ts` | Split: `runResult` = the try/catch + `problemDetail` fold, no mint. `runSubmit` = `runResult` wrapping `withIdempotencyKey`. | +| `libs/shared/src/application/submit.spec.ts` | Specs for both, including one that would catch a read minting a key again (see below). | +| `apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` | `load()` → `runResult`; docstring updated to name both halves. | +| `apps/ssp/src/app/brief/infrastructure/org-template.adapter.ts` | `list()`, `load()` → `runResult`. | +| `libs/beheer/src/infrastructure/stamdata.adapter.ts` | `list()`, `load()` → `runResult`; import trimmed to `runResult` only (no writes in this file). | +| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the new `runResult` describe block and the extra `runSubmit` case. | + +`runSubmit`'s new body is exactly the minimal composition the ticket asked for: + +```ts +export function runSubmit(fn: () => Promise, fallback: string): Promise> { + return runResult(() => withIdempotencyKey(crypto.randomUUID(), fn), fallback); +} +``` + +Zero behaviour change for the 8 write call sites — same fold, same mint, same timing (the key +is still minted before `fn` runs and cleared in `withIdempotencyKey`'s `.finally`). The five +reads now run the fold with no `pendingIdempotencyKey` touched at all. + +## The spec that would catch a regression + +`currentIdempotencyKey()` (`api-client.provider.ts`) returns the pending key while one is +"in flight" for the duration of a `withIdempotencyKey` call, and a fresh `crypto.randomUUID()` +on every call otherwise. That gives a real, mock-free way to assert "no key was minted": call +`currentIdempotencyKey()` twice inside the function passed to `runResult`/`runSubmit` — two +different reads means no pending key existed (each fell back to its own random UUID); two +equal reads means one pending key was minted and reused. + +```ts +it('mints no Idempotency-Key — the read fold', async () => { + let first = '', + second = ''; + await runResult(async () => { + first = currentIdempotencyKey(); + second = currentIdempotencyKey(); + return 'x'; + }, 'fallback'); + expect(first).not.toBe(second); +}); +``` + +This mirrors the house convention of not mocking relative imports under this repo's +Angular/vitest setup (see `role.interceptor.spec.ts`'s comment) — it asserts on real, +exported behaviour instead of a spy. + +**Verified red without the fix**: temporarily changed `runResult` to also call +`withIdempotencyKey` (i.e. reintroduced the bug it exists to prevent) and reran `ng test +shared`. Result: `runResult > mints no Idempotency-Key — the read fold` failed +(`expected 'd185d827-...' not to be 'd185d827-...'`), all 137 other tests stayed green. Then +reverted the temporary edit back to the real fix (an `Edit` undo, not `git checkout`, so the +rest of the change stayed in place) and reran — 138/138 green. + +## Judgement calls + +- **Docstring on `brief.adapter.ts`** was rewritten (it previously said only "Mutations go + through `runSubmit`") to name `load`'s `runResult` path explicitly, since the file mixes + both now and a future reader needs the split spelled out at the top, not just per-method. + `org-template.adapter.ts` and `stamdata.adapter.ts`'s docstrings needed no change — neither + named `runSubmit` specifically (`stamdata.adapter.ts`'s already correctly said "no write + method"). +- **No new concept, per the ticket's "minimal" framing** — `runSubmit` stays exported with + the same signature and the same call sites for the 8 real mutations; only its body changed + to delegate. +- **Left `runResult`'s JSDoc pointing at `runSubmit`** ("never route a read through that + one") rather than duplicating the Idempotency-Key explanation, so the two docs stay + synchronized by cross-reference instead of by copy. + +## Residuals (not this ticket) + +- RB-18 (key the `IdempotencyStore` on `{SubjectId}:{idemKey}`) is sequenced behind this one + per `99-backlog.md` and is unaffected by this split beyond it now landing on a correctly + write-only call set. +- RB-20 (`ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`) is + untouched, as scoped. + +## Verification + +`npm run ci` (foreground): **green** — lint, typecheck, `dep:check` (341 + 226 modules, 0 +violations), `format:check`, `check:tokens`, `check:seam`, tests (ssp 258/258, behandelportal +31/31, shared 138/138, beheer 23/23 — 450 total), `ng build --localize` (both apps), `npm +audit` (0 vulnerabilities), backend `dotnet test` (255/255 — the known +`OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure did not reproduce on +this run), `gen:snippets` drift clean, `gen:behaviour-spec` drift clean once the regenerated +file is committed alongside the code (the local gate compares the working tree to `HEAD`, so +it necessarily shows a diff pre-commit — this is the documented "will conflict at merge time" +behaviour, not a defect). diff --git a/libs/beheer/src/infrastructure/stamdata.adapter.ts b/libs/beheer/src/infrastructure/stamdata.adapter.ts index a4070ab..7728ae6 100644 --- a/libs/beheer/src/infrastructure/stamdata.adapter.ts +++ b/libs/beheer/src/infrastructure/stamdata.adapter.ts @@ -1,6 +1,6 @@ import { Injectable, inject } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; -import { runSubmit } from '@shared/application/submit'; +import { runResult } from '@shared/application/submit'; import { ApiClient, StamdataColumnDto } from '@shared/infrastructure/api-client'; import { ColumnType, StamColumn, StamRow, StamTable } from '@beheer/domain/stamdata'; @@ -24,7 +24,7 @@ export class StamdataAdapter { /** The tables in the catalog (schema only, no rows) — for the table switcher. */ async list(): Promise> { - const r = await runSubmit(() => this.client.stamdataTables(), FAILED); + const r = await runResult(() => this.client.stamdataTables(), FAILED); if (!r.ok) return r; const out: StamTable[] = []; for (const t of r.value ?? []) { @@ -39,7 +39,7 @@ export class StamdataAdapter { valid on that date; the editor uses it for a server-side cross-check, previewing locally for instant feedback (see `activeOn`). */ async load(tableId: string, peildatum?: string): Promise> { - const r = await runSubmit(() => this.client.stamdataTable(tableId, peildatum), FAILED); + const r = await runResult(() => this.client.stamdataTable(tableId, peildatum), FAILED); return r.ok ? parseStamdataTable(r.value) : r; } } diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 37de06e..14d0283 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 440 frontend behaviours across +**is** the suite, reshaped for a business reader. 445 frontend behaviours across 9 contexts; 231 backend behaviours across 39 test classes. @@ -800,11 +800,19 @@ classes. - leaves an unrelated endpoint untouched +#### runResult + +- folds a resolved call into ok(value) +- maps a ProblemDetails rejection to err(detail) +- falls back when the rejection has no detail +- mints no Idempotency-Key — the read fold + #### runSubmit - folds a resolved call into ok(value) - maps a ProblemDetails rejection to err(detail) - falls back when the rejection has no detail +- mints exactly one Idempotency-Key for the whole call — the write fold #### satisfaction helpers diff --git a/libs/shared/src/application/submit.spec.ts b/libs/shared/src/application/submit.spec.ts index 32abe86..1a770d6 100644 --- a/libs/shared/src/application/submit.spec.ts +++ b/libs/shared/src/application/submit.spec.ts @@ -1,5 +1,48 @@ import { describe, it, expect } from 'vitest'; -import { runSubmit } from './submit'; +import { runResult, runSubmit } from './submit'; +import { currentIdempotencyKey } from '@shared/infrastructure/api-client.provider'; + +// currentIdempotencyKey() returns the pending key minted by withIdempotencyKey while +// one is in flight, else a fresh random UUID on every call (see api-client.provider.ts). +// So calling it twice inside the same `fn` tells us, behaviourally, whether a key was +// minted for this call: two reads agreeing means one pending key was reused; two reads +// disagreeing means there was no pending key at all — each call fell back to its own +// random one. This is the seam RB-17 exists to keep separated, so it is asserted +// directly rather than via a mock (relative-import mocking is off-limits under this +// repo's Angular/vitest setup — see role.interceptor.spec.ts). + +describe('runResult', () => { + it('folds a resolved call into ok(value)', async () => { + const r = await runResult(async () => 'BIG-123', 'fallback'); + expect(r).toEqual({ ok: true, value: 'BIG-123' }); + }); + + it('maps a ProblemDetails rejection to err(detail)', async () => { + const r = await runResult(async () => { + throw { detail: 'Aanvraag afgewezen.' }; + }, 'fallback'); + expect(r).toEqual({ ok: false, error: 'Aanvraag afgewezen.' }); + }); + + it('falls back when the rejection has no detail', async () => { + const r = await runResult(async () => { + throw new Error('network'); + }, 'fallback'); + expect(r).toEqual({ ok: false, error: 'fallback' }); + }); + + it('mints no Idempotency-Key — the read fold', async () => { + let first = ''; + let second = ''; + await runResult(async () => { + first = currentIdempotencyKey(); + second = currentIdempotencyKey(); + return 'x'; + }, 'fallback'); + // No pending key: each read falls back to its own fresh random UUID. + expect(first).not.toBe(second); + }); +}); describe('runSubmit', () => { it('folds a resolved call into ok(value)', async () => { @@ -20,4 +63,17 @@ describe('runSubmit', () => { }, 'fallback'); expect(r).toEqual({ ok: false, error: 'fallback' }); }); + + it('mints exactly one Idempotency-Key for the whole call — the write fold', async () => { + let first = ''; + let second = ''; + await runSubmit(async () => { + first = currentIdempotencyKey(); + second = currentIdempotencyKey(); + return 'x'; + }, 'fallback'); + // One pending key reused across both reads inside this logical submit. + expect(first).toBe(second); + expect(first).not.toBe(''); + }); }); diff --git a/libs/shared/src/application/submit.ts b/libs/shared/src/application/submit.ts index 4ad4acf..6d5f59c 100644 --- a/libs/shared/src/application/submit.ts +++ b/libs/shared/src/application/submit.ts @@ -3,26 +3,36 @@ import { problemDetail } from '@shared/infrastructure/api-error'; import { withIdempotencyKey } from '@shared/infrastructure/api-client.provider'; /** - * Run a mutating API call and fold it into a `Result` — the one place the - * try/catch + ProblemDetails-mapping lives, so every `submit-*` command is just - * its own payload mapping. The backend re-validates and returns a 422 - * ProblemDetails on rejection, surfaced here as the error string. + * Run an API call and fold it into a `Result` — the one place the try/catch + + * ProblemDetails-mapping lives, so every adapter method is just its own payload + * mapping. The backend re-validates and returns a 422 ProblemDetails on + * rejection, surfaced here as the error string. * - * Also the one place a logical submit's Idempotency-Key is minted — once per - * `runSubmit` call, not per HTTP attempt — so a retry of this same submit - * dedupes on the backend (see `withIdempotencyKey`). + * This is the **read** half: it mints no Idempotency-Key. Use it for GETs. + * `runSubmit` below wraps it for mutations — never route a read through that + * one, it mints a key for nothing. */ -export async function runSubmit( +export async function runResult( fn: () => Promise, fallback: string, ): Promise> { try { - return ok(await withIdempotencyKey(crypto.randomUUID(), fn)); + return ok(await fn()); } catch (e) { return err(problemDetail(e, fallback)); } } +/** + * `runResult` for a **mutating** call: also mints the Idempotency-Key for this + * logical submit — once per `runSubmit` call, not per HTTP attempt — so a + * retry of this same submit dedupes on the backend (see `withIdempotencyKey`). + * Reads must go through `runResult` instead, which mints nothing. + */ +export function runSubmit(fn: () => Promise, fallback: string): Promise> { + return runResult(() => withIdempotencyKey(crypto.randomUUID(), fn), fallback); +} + // Single shared default for a failed submit; the @@id dedupes it at the // translation layer. export const SUBMIT_FAILED = $localize`:@@submit.failed:Het indienen is niet gelukt. Probeer het later opnieuw.`;