refactor(shared): split runResult out of runSubmit (RB-17)
runSubmit did two things at once: fold a call into a Result, and mint an Idempotency-Key for it. Five call sites are reads and had no business minting one — brief.adapter.ts:load, org-template.adapter.ts :list/:load, and stamdata.adapter.ts:list/:load. stamdata.adapter.ts's own docstring already said "Both endpoints are reads … There is no write method" while both called runSubmit; that mismatch is the sharpest evidence, and the reason the baseline's original "~13 mutations" count (derived from the helper's name, not the code) was wrong by five in one direction. Split submit.ts in place: runResult is the try/catch + problemDetail fold with no mint; runSubmit is runResult wrapping withIdempotencyKey. Zero behaviour change for the 8 real mutations (brief save/submit/approve/reject/send/reset, org-template save/publish/rollback) — same fold, same mint, same timing. The five reads now run the fold with no pendingIdempotencyKey touched. submit.spec.ts asserts the split behaviourally via currentIdempotencyKey() (two reads inside the same call agree only when a key was minted and reused) rather than mocking a relative import, matching this repo's existing vitest convention. Verified red without the fix by temporarily reintroducing the mint into runResult. ApplicationsStore.cancel/AdminCasesStore.delete (RB-20) and FeatureFlagStore.set are out of scope and untouched — the latter already calls runSubmit correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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<T>(fn: () => Promise<T>, fallback: string): Promise<Result<string, T>> {
|
||||
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).
|
||||
Reference in New Issue
Block a user