Merge RB-17 — split runResult out of runSubmit
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