Skip to content

fix(zgw): a guard that cannot run its check refuses the request (decision 224) - #3854

Merged
rubenvdlinde merged 3 commits into
developmentfrom
build/dq-l11-25
Oct 15, 2026
Merged

rubenvdlinde merged 3 commits into
developmentfrom
build/dq-l11-25

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Decision 224 (Ruben, 12 Oct): guards refuse when unsure.

Stacked on #3836 (which contains #3832 to #3835). Land that stack first; this PR then shows one commit.

What changes

Three guards let a request through when they could not run their check. They now refuse.

Guard Before, when the lookup failed Now
Case type delete check the case type was deleted 503 destroy_check_not_completed
drc-004 (the ZaakInformatieObject or BesluitInformatieObject exists) the link was stored 503 check-not-completed
drc-007 (no relation points at the document) the document was deleted 503 check-not-completed

"The lookup ran and found nothing" still passes, or gets the rule's own refusal. Only "the lookup could not run" is new.

The refusal names the rule in its detail, carries no invalidParams, and is logged at error with the rule and the object. Nothing is stored or deleted.

Two more, and why

  • drc-003 changes its answer. It already refused, but with 400 unique: "this combination already exists", about a relation nobody found. It now gives the same 503. One line to put back if you want the old words.
  • The controller has its own copy of the document delete check (DrcController, drc-008a: OIO, ZIO and BIO). It also passed on a failed search, so a failed ZIO or BIO search still let the delete through. It gives the same 503 now.

Why 503 and not 400

The ZGW error model has two shapes. A ValidatieFout (400 with invalidParams) says the request is wrong and names the field to fix. A Fout is everything else. A check that did not run says nothing about the request, so a 400 would send the client off to fix a body that was fine. drc-003's unique did exactly that.

Of the 5xx statuses the ZGW specifications declare 500 on every operation, not 503. I chose 503 anyway, for one reason: these endpoints already answer 503 with a detail when OpenRegister is absent (ZgwService::unavailableResponse), and a store that cannot be read is the same situation. It also tells the client the request can be sent again unchanged.

If you prefer the declared 500, it is one constant: ZgwRuleResults::NOT_VERIFIED_STATUS.

Callers

Every caller of the rules turns the refusal into a response with its status: ZgwService::handleCreate, handleUpdate and handleDestroy, DrcController, ZrcController, BrcController and BrcDocumentLinkHandler. No job, command or listener calls these rules, so there is no background path that could swallow the refusal. After a refused document delete nothing cascades: the gebruiksrechten and the files stay.

Debt list

catch-return-null.allowlist.json: two sites left (DrcDocumentRelations::findOioRelationsForDocument, ZtcPublishAndDeleteChecks::loadFinalStatusSlugs). Ceiling 256 to 254, against development after train 5 (re-measured on 14 Oct: still 256).

Spec

New change openspec/changes/guards-refuse-when-unsure with REQ-ZBR-020 and REQ-ZBR-021.

Not in this PR

  • A rule whose schema SETTING is empty still skips its lookup, as drc-003 always did.
  • A failed ZGW delete still answers 404 when the store was down (Q-dossiq-L11-2, answered: its own PR, stacked on this one).
  • The same "failed read counts as none" in identification uniqueness and reference resolution (Q-dossiq-L11-3, answered: its own PR, stacked on this one).
  • The weekend deadline reading in the work queue: Q-dossiq-L11-4.

Verified

On e782455, every composer check:strict stage, one at a time in the lane's check worktree (exit codes in dq-l11-logs/r6/s25.rc):

  • lint 0, refusal-grounds snapshot 0, phpcs 0, phpmd 0 (whole tree), psalm 0, phpstan 0.
  • PHPUnit --no-coverage: 8,487 tests, 2 failures. Both are StubApiDriftTest (DecisionChangeRequestedEvent, DecisionVisibilityRequestedEvent), the stub drift train 4 fixes. Not in this diff.
  • The touched suites under a coverage driver with fail-on-risky (pcov, 216 tests): no risky test.
  • Six mutations, one per guard, each turns the new tests red: drc-004, drc-007, the delete check, its guard in ZgwBusinessRulesService, the controller's check, and drc-003 claiming a duplicate.
  • Hydra gates on the diff: 79 of 80 applicable ran, 8 failed, none in this diff (22 and 53 need node_modules; 23, 43, 51, 55, 104, 107 inherited). Gates 16, 19 and 46 pass.
  • openspec validate guards-refuse-when-unsure: valid.

Not run: npm run lint and vitest. No JavaScript, no l10n and no register change in this PR.

Live pass (decision 139): with the store reachable, an objectinformatieobject create, a document delete and a case type delete answer as before.

Merged with development (14 Oct)

Train 5 (#3855) landed, so development is merged in (8407d47, then again at 429e86c). The only conflict was catch-return-null.allowlist.json: development's entries and about lines kept, this PR's two entries removed, its about line added, ceiling 256 to 254. tests/Unit/Architecture: 104 tests, OK. The full stages ran once on the head of the PR stacked on this one (#3858), which contains this branch; see there.

Decision 224. The case type delete check, drc-004 and drc-007 let a request
through when their lookup failed. They now refuse with 503
check-not-completed, name the rule, and log the rule and the object.
drc-003 gives the same answer instead of claiming a duplicate, and the
controller's relation check before a document delete (drc-008a) follows.

Two sites leave the catch-return-null list, ceiling 263 to 261.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

CI read once: the PR is CONFLICTING, so only 4 checks ran and their green says nothing. One file conflicts, with development and with train 5 (#3855) alike: tests/Unit/Architecture/catch-return-null.allowlist.json. Resolution after train 5 lands: merge development, keep development's entries and about lines, remove the two entries this PR removes (DrcDocumentRelations::findOioRelationsForDocument, ZtcPublishAndDeleteChecks::loadFinalStatusSlugs), add this PR's about line, and set the ceiling to development's number minus 2. Not merged now: the stack below this PR is inside the train and merging the train branch here would tie this PR to an unlanded head.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/dossiq @ 3d01962

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ❌
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
check-vue3-compile ✅
test-l10n ✅
format ❌
check-schema-l10n ✅
check-l10n-js ✅
check-debt-ratchet ❌
check-types-e2e ✅
composer ✅ ✅ 109/109
npm ✅ ✅ 659/659
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌ 8640/8640 · 3 skipped (no test failed, the job failed for another reason — see its log)
Newman ⏭️
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ❌

Quality workflow — 2026-10-14 20:15 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit b99db82 into development Oct 15, 2026
44 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant