From 6a4a0ad435f6abd290a14c34c1aac3c35f5db284 Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Wed, 5 Aug 2026 15:20:55 +0200 Subject: [PATCH] docs: add WP-68, aggregate invariants + status modelling Architecture review found the context boundaries, FP/TEA idioms and read/write separation sound, and rejected explicit CQRS as the fix for anything found. It located four real defects clustered in one place: the backend's aggregate roots don't guard their own invariants, and the aanvraag status lifecycle is a computed string living in the contracts layer instead of the domain. Full Decisions block pre-made so implementation can proceed without re-litigating scope. Co-Authored-By: Claude Opus 5 --- docs/project/backlog/README.md | 1 + .../backlog/WP-68-ddd-aggregate-hardening.md | 284 ++++++++++++++++++ 2 files changed, 285 insertions(+) create mode 100644 docs/project/backlog/WP-68-ddd-aggregate-hardening.md diff --git a/docs/project/backlog/README.md b/docs/project/backlog/README.md index c65597a..a570932 100644 --- a/docs/project/backlog/README.md +++ b/docs/project/backlog/README.md @@ -118,6 +118,7 @@ for its existing violations, so every WP ends green. | [WP-65](WP-65-behandelportal-beoordeling.md) | Behandelportal: zaak detail + beoordeling (decision) screen | 11 · Behandelportal | done | | [WP-66](WP-66-behandelportal-openzaak-write.md) | Wire the decision into OpenZaak | 11 · Behandelportal | done | | [WP-67](WP-67-monorepo-behandelportal.md) | Merge behandelportal into this repo as a monorepo | 11 · Behandelportal | done | +| [WP-68](WP-68-ddd-aggregate-hardening.md) | Aggregate invariants + status modelling (architecture review) | 12 · DDD hardening | todo | Sequencing dependencies (stated in the WPs too): 01 before 10–15 (axe covers story churn); 03/04 before 05–09 (boundaries stop new violations during refactors); 06 before 07 (typed diff --git a/docs/project/backlog/WP-68-ddd-aggregate-hardening.md b/docs/project/backlog/WP-68-ddd-aggregate-hardening.md new file mode 100644 index 0000000..65b88c7 --- /dev/null +++ b/docs/project/backlog/WP-68-ddd-aggregate-hardening.md @@ -0,0 +1,284 @@ +# WP-68 — Aggregate invariants + status modelling (architecture review remediation) + +Status: todo +Phase: 12 — DDD hardening + +## Why + +An architecture review on 2026-08-05 (bounded contexts, aggregates, CQRS, DDD/BDD test +alignment, measured against this repo's own documented pattern) found the context boundaries, +the FP/TEA idioms and the read/write separation to be sound — and found four defects clustered +in one place: **the backend's aggregate roots do not guard their own invariants, and the +aanvraag status lifecycle is a computed string living in the contracts layer.** + +The four in this WP, in dependency order: + +- **F1 — `submit` links client-supplied `documentId`s with no ownership check.** + `Program.cs:353-356` takes document ids straight from the request body and hands them to + `ApplicationStore.Submit` and `documents.LinkToZaak`; `DocumentStore.Link` has no `owner` + parameter and performs no check (`DocumentStore.cs:113-125`). Same for `SyncDraft` + (`Program.cs:317`). A caller who knows a foreign document GUID can attach another citizen's + upload to their own aanvraag — where it appears on the behandelaar's beoordeling screen with + its filename (`Program.cs:434`) and is POSTed to OpenZaak as a zaakinformatieobject on + _their_ zaak — and flips the victim's `Linked = true`, which permanently blocks the victim's + own delete (`DeleteOwned` → `DeleteResult.Linked`). ADR-0001 is explicit that the FE holds + no authority; this trusts it anyway. + +- **F3 — the aanvraag status lifecycle is a computed string in `Contracts/`.** Three + compounding facts: the status is derived in `Contracts/Mappers.ToStatusDto` + (`Mappers.cs:44-63`), not in the domain; `Concept` is **not** a member of + `AanvraagStatusTag` (`ApplicationStore.cs:14`) but a magic string the mapper emits; and the + write path reads its own guard back out of the read DTO — + `a.ToStatusDto(now).Tag` → compare `"Concept"` → `Enum.Parse` + (`Program.cs:466-468`). This violates the repo's non-negotiable #3 ("make illegal states + unrepresentable") on the backend's most important type: the status is + `enum + one string that is not in the enum`, so `Enum.Parse` is a runtime throw waiting for + a new tag. It is also the one genuine CQRS symptom in the codebase — a command deriving its + invariant from a read projection — and it is _why_ F2 exists: there is no domain object that + could have owned the guard. + +- **F2 — the besluit invariant is checked outside the write transaction.** + `Program.cs:469` calls `BeoordelingRules.CanDecide`; the write happens later in + `ApplicationStore.RecordBesluit` (`ApplicationStore.cs:278-291`), which takes the lock and + assigns unconditionally. Two concurrent besluiten both pass the check and both write, so the + second silently overwrites a terminal decision the rule exists to freeze. The codebase + already documents the correct pattern three methods earlier — `CreateConcept`: _"Race-free: + the existence check and the insert share the single write gate."_ This is an internal + inconsistency, not a missing concept. + +- **F6 — a besluit rule with no home in `Domain/`.** "Toelichting verplicht bij Afwijzen / + MeerInfoOpvragen" lives inline at `Program.cs:473`, although `BeoordelingRules`' own + doc-comment says the decision-recording rules were meant to land there. It therefore has no + unit test, only the endpoint test `Afwijzen_requires_a_toelichting`. + +Plus one documentation correction (**F5**, see Decisions — the enforcement itself is deferred +to WP-69, because it needs a wire change). + +The review's remaining findings are listed under "Follow-ups" and are **not** this WP's scope. + +## Read first + +- `CLAUDE.md` §"The decisions" #3 (make illegal states unrepresentable) and #4 (BFF-lite) +- [ADR-0001 — BFF-lite + decision DTOs](../../reference/architecture/0001-bff-lite-decision-dtos.md) +- `backend/src/BigRegister.Api/Data/ApplicationStore.cs` (the `Aanvraag` entity, the store's + lock discipline, `AanvraagStatusTag`, `RecordBesluit`) +- `backend/src/BigRegister.Api/Contracts/Mappers.cs` (`ToStatusDto` — the logic to move) +- `backend/src/BigRegister.Api/Program.cs` lines 300-500 (draft sync, submit, beoordeling GET, + besluit POST) +- `backend/src/BigRegister.Api/Zgw/ZgwZaakMapper.cs` (**the second producer of the status + DTO** — easy to miss) +- `backend/src/BigRegister.Api/Data/DocumentStore.cs` (`Link`, `DeleteOwned`, the existing + `DeleteResult` enum this WP copies) +- `backend/src/BigRegister.Api/Domain/Beoordeling/BeoordelingRules.cs` + +## Prerequisite + +**Commit or stash the working tree first.** At review time it carried the WP-66 id-mismatch fix +across 11 modified files plus the untracked `backend/tests/BigRegister.Tests/BeoordelingIdMismatchTests.cs`. +Do not start a cross-cutting refactor on top of uncommitted work. + +## Decisions + +Pre-made — do not relitigate. + +### F3 — the status type + +1. **Move `AanvraagStatusTag` and `Besluit`** out of `Data/ApplicationStore.cs` into + `Domain/Applications/` (namespace `BigRegister.Domain.Applications`). Move + `ApplicationStore.ProcessingWindow` there too — `StatusAt` needs it, and `Data → Domain` is + the legal direction. +2. **Add `Concept` as the first member of `AanvraagStatusTag`.** Keep `Ingediend` even though + nothing produces it today (verified: neither `ToStatusDto` nor `ZgwZaakMapper` emits it) — + `BeoordelingRules.CanDecide` accepts it, the FE's `BeoordelingStatus` union declares it, + `statusLabel` has a `$localize` id for it, and `Only_open_statuses_are_decidable` tests it. + Deleting it would ripple into `messages.en.xlf`. Mark it reserved with a comment instead. +3. **New `Domain/Applications/AanvraagStatus.cs`**: a `sealed record` carrying + `AanvraagStatusTag Tag` plus the same optional payload fields the DTO has + (`StepIndex`, `StepCount`, `Referentie`, `Manual`, `Reden`), constructed **only** via static + factories — `Concept(stepIndex, stepCount)`, `InBehandeling(referentie, manual)`, + `Goedgekeurd(referentie)`, `Afgewezen(referentie, reden)`, + `MeerInfoGevraagd(referentie, reden)`. + **Rejected: a full abstract-record union** (one subrecord per tag). It is the purer + modelling, but it forces exhaustive switches at four call sites and a per-case mapper for a + marginal gain over "the factories are the only construction path". Not worth the diff here. +4. **`Aanvraag.StatusAt(DateTimeOffset now)`** — an instance method on the entity carrying the + logic currently in `ToStatusDto` **verbatim**, including the "a recorded decision wins over + the auto-approve computation" ordering. +5. **`Mappers.ToStatusDto` becomes a one-line projection** of `a.StatusAt(now)`: + `new(s.Tag.ToString(), s.StepIndex, s.StepCount, s.Referentie, s.Manual, s.Reden)`. +6. **`AanvraagStatusDto` is unchanged — `Tag` stays a `string`.** This is the safety property + that makes F3 an internal refactor: **no wire change, no `gen:api` drift, no frontend + change, no `messages.en.xlf` change.** Do not "improve" the DTO in this WP. +7. **`ZgwZaakMapper` is the second producer** and must be converted too, or the string literals + survive: `ToSummaryDto` and `ToCreatedStatusDto` build `AanvraagStatus` via the factories and + project through the same one-liner. Its coarse behaviour must not change (open/no einddatum → + `InBehandeling` with `Manual: true`; closed → `Goedgekeurd`) — `ZgwZaakMapperTests` is the net. +8. **The besluit endpoint stops going through the DTO**: `var status = a.StatusAt(now);` + compare `status.Tag == AanvraagStatusTag.Concept`, pass `status.Tag` to `CanDecide`. The + `Enum.Parse` at `Program.cs:468` is deleted. +9. **One `Enum.Parse` may remain** — the beoordeling GET at `Program.cs:438`, which parses a tag + off a DTO returned by the `IZaakSource` seam. That is a genuine wire→domain trust boundary, + not a smell. Keep exactly one, make it non-throwing for an unknown tag, and comment it as the + seam boundary. **Changing `IZaakSource` to return domain types is out of scope.** + +### F2 — the besluit guard + +`ApplicationStore.RecordBesluit(string id, Besluit besluit, string? toelichting, DateTimeOffset now)` +returns `(RecordBesluitOutcome Outcome, Aanvraag? Aanvraag)` with +`enum RecordBesluitOutcome { Ok, NotFound, Conflict }` — mirroring the existing +`DocumentStore.DeleteResult` precedent rather than inventing a new result idiom. Inside the +lock: find, `StatusAt(now)`, `CanDecide` → `Conflict` if refused, then write. **The endpoint +drops its own pre-check** and maps the outcome to 200/404/409, so there is one source of truth +for the transition. The endpoint keeps its id-resolution and its `Concept` → 404 (both need the +`IZaakSource` lookup the store cannot see). + +### F1 — document ownership + +New `DocumentStore.ForeignIds(IEnumerable ids, string owner)` returning the ids that do +**not** resolve to a document owned by `owner` (returning the offending ids, not a bool, so the +ProblemDetails can name them). Called in `POST /applications/{id}/submit` **before** any write, +and in the draft-sync endpoint (`Program.cs:317`); non-empty → 400 ProblemDetails. + +Endpoint-level check only. `IDocumentSource.LinkToZaak` keeps its current signature (two +implementations, and the endpoint has now validated its input) — add a comment saying so. +"A document already linked to a different aanvraag of the same owner" is **not** covered here; +note it as a follow-up, do not build it. + +### F5 — narrowed to a doc fix + +`IntakePolicy`'s XML doc-comment claims _"the backend re-validates on submit as the +authority"_. It does not: the constant's only consumer is `Program.cs:155`, which echoes it, and +both submit paths apply `SubmissionRules.RejectZeroUren` only. Verified cause: **neither +`SubmitApplicationRequest(DiplomaHerkomst, Uren, Documents)` nor `IntakeRequest(int Uren)` +carries a scholing answer at all**, so the server cannot re-validate without a contract change, +and the wizard's answers (`scholingGevolgd`, `punten` — `intake.machine.ts:26,37`) never reach +it. Reading them out of the opaque `Draft` JSON is rejected: the backend's documented posture is +that the draft is opaque (`AppDbContext` header comment). + +**In this WP: correct the doc-comment to state the gap, and nothing else.** The enforcement is +WP-69 (a real FE+BE slice: request fields, `IntakePolicy.RejectMissingScholing`, wizard payload, +`gen:api`). + +## Files + +- `Domain/Applications/AanvraagStatus.cs` (new — tag enum, `Besluit`, `ProcessingWindow`, the + status record + factories) +- `Data/ApplicationStore.cs` (`Aanvraag.StatusAt`, `RecordBesluit` signature + in-lock guard, + enums moved out) +- `Contracts/Mappers.cs` (`ToStatusDto` reduced to a projection) +- `Zgw/ZgwZaakMapper.cs` (both producers converted) +- `Data/DocumentStore.cs` (`ForeignIds`) +- `Domain/Beoordeling/BeoordelingRules.cs` (`RequiresToelichting`) +- `Domain/Intake/IntakePolicy.cs` (doc-comment only) +- `Program.cs` (submit + draft-sync ownership checks; besluit endpoint simplified) +- `tests/BigRegister.Tests/` — `RuleTests.cs` (new `AanvraagStatusTests` nested class + + `RequiresToelichting`), `ApplicationTests.cs` (ownership), `BeoordelingTests.cs` (concurrency) + +No migration: no persisted column changes (`BesluitStatus` already stores `Besluit`, whose +member names are unchanged). + +## Steps + +1. Commit/stash the WP-66 working tree (see Prerequisite). +2. **F1** — `DocumentStore.ForeignIds` + the two endpoint checks + tests. Independent of the + rest; land it first so the correctness fix is not blocked by the refactor. +3. **F3** — the status type, in Decisions order 1→9. `dotnet test` green with + `AanvraagStatusTag_covers_the_published_lifecycle`, + `AutoApprovable_flips_to_goedgekeurd_after_the_window` and `ZgwZaakMapperTests` **unchanged** + — those three are the regression net for the refactor. +4. **F2** — `RecordBesluitOutcome`, guard moved inside the lock, endpoint maps the outcome. +5. **F6** — `BeoordelingRules.RequiresToelichting` + unit test; endpoint calls it. +6. **T3** — the lifecycle spec that F3 makes expressible: one `[Theory]` over + (status × besluit) → allowed/denied, asserting among others that Afgewezen → Goedgekeurd is + refused as a _domain_ statement, not only at the endpoint. +7. **F5** — correct the `IntakePolicy` doc-comment; open WP-69 for the enforcement. +8. Run the full gate (see Verification). + +## Acceptance criteria + +- [ ] Submitting (or draft-syncing) an aanvraag with a `documentId` owned by another citizen is + rejected with 400, and the other citizen's document remains deletable + (`DeleteResult.Ok`). +- [ ] `AanvraagStatusTag` contains `Concept`; no code compares a status against a string + literal. `grep -rn '"Concept"' backend/src` returns no comparison sites. +- [ ] `Enum.Parse` appears **at most once** in `backend/src`, at the + `IZaakSource` seam (`Program.cs` beoordeling GET), and does not throw on an unknown tag. +- [ ] `Mappers.ToStatusDto` contains no lifecycle logic — it projects `Aanvraag.StatusAt(now)`. +- [ ] `ZgwZaakMapper` constructs no `AanvraagStatusDto` from string literals. +- [ ] `npm run gen:api` leaves **no diff** in `backend/swagger.json` or + `libs/shared/src/infrastructure/api-client.ts` (proof F3 changed no wire shape). +- [ ] Two concurrent `POST /beoordeling/{id}/besluit` against an already-terminal aanvraag yield + exactly one 200 and one 409; the recorded besluit is the first one. +- [ ] `BeoordelingRules.RequiresToelichting` exists, is unit-tested, and is the only place the + rule lives. +- [ ] A `[Theory]` covers the (status × besluit) transition table at domain level. +- [ ] `IntakePolicy`'s doc-comment no longer claims server-side re-validation; WP-69 exists. + +## Verification + +```bash +cd backend && dotnet test # while iterating +npm run gen:api && git diff --exit-code backend/swagger.json libs/shared/src/infrastructure/api-client.ts +npm run ci # the full gate before pushing +npm run e2e # after F1/F2/F3 — needs the backend + `npm start` running +``` + +The three existing tests named in step 3 must pass **unmodified**; if a refactor step needs one +of them changed, the refactor changed behaviour and is wrong. + +## Out of scope + +Deliberately excluded — each is a separate WP if wanted: + +- **F4** — backend layer enforcement. `Domain/Beoordeling/BeoordelingRules.cs` and + `Domain/Authorization/Authz.cs` import `BigRegister.Api.Data` (and `Authz` also + `.Contracts`, returning `BriefDecisionsDto`), with nothing in CI checking direction — the FE + has `dep:check`, the backend has only `dotnet format` + `dotnet test`. This WP's step 3 + removes the `BeoordelingRules` violation as a side effect; the `Authz` one and the ~6-line + reflection convention test are WP-70. +- **F5 enforcement** → WP-69 (see Decisions). +- **F7** — `ApplicationStore.Submit` and `DocumentStore.Link` take separate locks with no + transaction and no compensation; a link failure leaves a submitted aanvraag whose documents + are still deletable. Same failure class WP-60 closed for ZGW and left open locally. Fix is to + route it through the existing divergence flag + audit row, not to merge the aggregates. +- **F8** — pushing invariants from the static stores onto `Aanvraag` as instance methods + (`TryRecordBesluit`). This WP does the two that matter; the general move can wait. +- **F9** — `Authz` spans five contexts and its four admin gates are byte-identical + `role == Admin` checks with **no direct unit test** and no test denying `Approver`. +- **F10** — splitting `Program.cs` (917 lines, 50 endpoints). **Deliberately deferred and + flagged as risky:** `OrgAdmin`, `StamdataAdmin`, `Beoordelen`, `Submit` and `AuditAuthz` are + non-static **local functions** (`Program.cs:756+`) that every endpoint lambda closes over, so + splitting means converting all of them to static helpers with explicit dependencies across + all 50 registrations — with the deliberate authz ordering (Forbidden before Conflict) as the + thing that breaks silently. Lowest value of the review's findings; do it alone, with tests as + the net, or not at all. +- **F11** — three FE adapter fetch idioms; two loaders `throw` instead of returning `Result`; + `runSubmit` (which mints an `Idempotency-Key`) is used for **reads** in `brief.adapter.ts:56`, + `org-template.adapter.ts:39,51`, `stamdata.adapter.ts:27,42`. Fix is `runQuery`/`runCommand` + over one shared try/catch, ~10 lines. +- **T2** — ~54 FE `it()` titles are named after `Msg` tags (`'SetField updates the draft'`, + `'SubmitConfirmed maps Submitting to Submitted'`), against `bdd.mdx` rule 3. Titles only. +- **T5** — named coverage gaps: `OrgTemplateRules.RejectDraft` (both identity branches, no + margin boundary test), the four `Authz` admin gates, `DocumentRules.CategoriesFor`'s + `herregistratie`/`org-template` branches, `SubmissionRules.NewReference`, FE + `isStatusConsistent` (tested on the backend, never on the FE), the FE herregistratie window + boundary, and the FE/BE margin constants which mirror each other with no contract test. +- **T6** — trust-boundary `describe` naming has three dialects; 7 `parse*` specs use none. +- **ADR-0006 "CQS without CQRS"** — the review's learning deliverable: the read/write + separation already present, why the emit-and-enforce rule (one function feeding both the + decision flag and the enforcement) makes a read/write stack split actively harmful here, and + WP-60's deferred outbox as the documented trigger that would change the answer. Prose only, + no runtime code. +- Anything CQRS-mechanical: MediatR, handler classes, a separate read store, event sourcing, + repositories/unit-of-work, Gherkin/Reqnroll. All explicitly rejected by the review. + +## Risks + +- **Scope creep on F3.** The temptation is to "fix" `AanvraagStatusDto` into a proper wire union + while in there. That turns a zero-diff internal refactor into an FE + `messages.en.xlf` + + `gen:api` change. The acceptance criterion "`gen:api` leaves no diff" exists to catch it. +- **Missing the second producer.** `ZgwZaakMapper` is easy to overlook because it lives under + `Zgw/`, not `Contracts/`. If it is missed, the string literals survive and the finding is only + half fixed. +- **Over-modelling.** A full abstract-record status union, or a repository/unit-of-work layer to + "properly" own the aggregate, would be a bigger diff than the defects justify — see Decisions.