Repository navigation
Commit c8c803c
fix(objectql): a record the same cascade deletes never refuses its sibling's delete (#22377)
Fixes #22305
Clause-②: no
## What was wrong
A by-id delete runs `ObjectQL.cascadeDeleteRelations`. It walks
`_registry.getAllObjects()` in registration order and recurses
depth-first through the public `delete()`. Each level judged its own
`restrict` refusals without knowing about the cascade that called it.
The measured shape is hotcrm's account. An account cascades
(`master_detail`) to its contacts and to its contracts. Each contract
holds a required lookup to a contact, and that lookup's default
`set_null` escalates to `restrict`. With the contacts registered first,
the walk deleted a contact first, and the contact's own walk refused on
a contract that the same account delete was about to remove. With the
contracts registered first, the identical delete succeeded.
## The rule (triage ruling 6062588548, verbatim)
> - **Direction:** a child that is itself in the cascade set never
restricts a sibling in the same set. Pin: the account-with-contracts
shape deletes. Control: a restrict on a record outside the cascade still
refuses.
## The fix
`cascadeDeleteRelations` now runs in two phases. Both phases read
relations and rows one way: `cascadeRelationBehavior` decides which
relations point at an object, and `probeReferencingRows` reads the
referencing rows (system identity, the same filter, the same
missing-table and multi-value handling).
1. `collectCascadeDeleteSet` is read-only. It walks breadth-first across
every relation whose resolved behaviour is `cascade`, starting from the
root, and collects every record the delete will remove. The root is in
the set. A visited check stops it on a cycle.
2. `walkReferencingRelations` is the existing walk, in the existing
order. Three things change:
- `restrict` and `set_null` consider only rows outside the set. That
covers an authored `restrict` and both halves of the required `set_null`
escalation.
- `set_null` writes nothing to a row in the set.
- The walk never cascades into a row it has already entered.
The set is carried by an `AsyncLocalStorage` on the engine
(`cascadeDeleteSets`), the same kind of carrier as `txStore`. A nested
`delete()` for a member joins the set. Any other delete, such as one a
hook makes, is the root of its own cascade, as before. The set is not
stored on the request context, for two reasons: a caller that passed no
context still passes none, and no request input can add a record to the
set.
## Base vs head, measured
Rig: a real `ObjectQL` over a real `SqlDriver` (better-sqlite3), called
through `ObjectStackProtocolImplementation.deleteData`, the method
`DELETE /api/v1/data/:object/:id` serves. Base is `e87070ed49`. Head is
`4e096ce935`, whose `engine.ts` is byte-identical to the final head's.
The objectql `dist/` was rebuilt for each leg. `ablation-dist-preflight`
proves which build each leg read: the marker `cascadeDeleteSets` is
absent from base's `dist/` and present in head's.
| Case | Base | Head |
|---|---|---|
| hotcrm shape, contacts registered first | `409 DELETE_RESTRICTED`,
`dependentObject` `zz_contract`, count 2. Nothing deleted. | Succeeds.
The account, 2 contacts and 3 contracts are all gone. |
| hotcrm shape, contracts registered first | Succeeds, all gone |
Succeeds, all gone |
| Control: an outside required lookup (`zz_invoice` to a contact),
contacts first | `409` naming `zz_contract`. The defect hid the real
refusal. | `409 DELETE_RESTRICTED`, `dependentObject` `zz_invoice`,
count 1. Rolled back; all 6 rows remain. |
| Control: same, contracts first | `409` naming `zz_invoice`, count 1.
Rolled back. | Same |
| `set_null` inside the set (`zz_memo` to a contact), contacts first |
Succeeds, but issues 1 UPDATE on the memo before deleting it | Succeeds,
0 UPDATEs |
| `set_null` inside the set, memos first | Succeeds, 0 UPDATEs |
Succeeds, 0 UPDATEs |
| Cycle: a self-referencing cascade whose two rows point at each other |
The walk re-enters without end; the test's runaway guard stopped it at
61 `beforeDelete` dispatches. Nothing deleted. | Succeeds. Both rows
deleted, 2 `beforeDelete` |
| Cycle: two objects that cascade into each other | Same runaway |
Succeeds. Both rows deleted, 2 `beforeDelete` |
| A row that two cascade paths reach (an item tree under an account) |
`404 RECORD_NOT_FOUND` on the second path | Succeeds. Each row deleted
once. |
The REST door was measured once on the real HTTP stack (`bootStack`,
admin, `DELETE /api/v1/data/zzr_account/:id`):
- Base, contacts first: `409`, body `"code":"DELETE_RESTRICTED"`, naming
`zzr_contract`.
- Base, contracts first: `200`.
- Head, both orders: `200`, and every row is gone.
## Atomicity and side effects
- **Atomicity.** A cascade on one datasource is one transaction
(`planCascadeAtomicity` answers `'atomic'`), and it stays one: the set
is collected inside that transaction. In the outside-restrict control,
contracts first, head deletes 4 rows before the refusal, and the
rollback restores every one of them.
- The cross-datasource path (`'split'`) is unchanged and stays
non-atomic, as `warnCascadeNotAtomic` declares. On that path, a later
refusal can also leave behind a member whose restrict was skipped. The
code comment states this.
- **Hooks.** Every deleted record fires `beforeDelete` and `afterDelete`
exactly once, in both orders (pinned).
- **Elevation records (#12166).** Each deleted record files one record
per referenced object, never two (pinned). The dedupe set is shared by
both phases.
- **`set_null` on a member.** Base issued the extra UPDATE only with the
contacts registered first: update hooks fired and an audit update row
was written, then the record was deleted. Head never issues it. Both
orders now leave the trail that base left when the memos were registered
first.
## Cost
Phase 1 adds one extra probe per cascading relation per member.
`restrict` and `set_null` relations are not read in phase 1.
| Shape | Base | Head |
|---|---|---|
| Wide cascade: 1 account, 20 contacts, 3 contracts each (81 records) |
22 probe reads (contracts first; contacts first refused after 3) | 24
probe reads, both orders |
| hotcrm shape (6 records) | 4 probe reads | 6 probe reads |
In phase 1, the relation scan runs once per object, not once per record.
## Pins and reverse verification (at `faa4ac206a`)
`packages/objectql/src/engine-cascade-delete-sibling-restrict.test.ts`
has 21 cases, all green at head.
| Run | Red | Green | Red cases |
|---|---|---|---|
| Base `engine.ts` (hash-proven swap and restore) | 12 | 9 | — |
| Ablation: the member filter disabled | 9 | 12 | hotcrm (contacts
first); hooks; elevation; the three outside controls (contacts first);
`set_null` member; authored restrict between members; root membership |
| Ablation: the entered-row skip deleted | 3 | 18 | both cycles; the
two-path tree |
Both ablations went through `scripts/ablation-replace.mjs` in wrap mode.
Each restore is proven by blob hash: `bd05e02c8654` after restore equals
the HEAD blob.
## Clause-② evidence
The built declarations were compared, base vs head:
- `index.d.ts` and `core.d.ts` are identical apart from the shared
chunk's file hash.
- The shared chunk gains six `private` member names and their doc
comments: `cascadeDeleteSets`, `collectCascadeDeleteSet`,
`cascadeRelationBehavior`, `fileReferenceCheckElevation`,
`probeReferencingRows`, `walkReferencingRelations`.
- No export, public or protected member, or type changes.
## Also in this PR
- `packages/objectql/src/federated-injected-column-readers.test.ts`:
this ledger names the readers of injected columns by function. Its two
`cascadeDeleteRelations` rows now name `cascadeRelationBehavior`, where
those calls moved.
- `content/docs/api/data-api.mdx`: the `DELETE /data/:object/:id`
section said relations honour their `deleteBehavior` "with one
substitution". That sentence would be false after this change, so the
section now states the cascade-set rule. This file is outside the
dispatched file surface.
- `.changeset/22305-cascade-sibling-restrict.md`: a `patch` for
`@objectstack/objectql`. Changesets pre mode is on
(`.changeset/pre.json`).
## Tests
All runs used the shared verify lock.
- `@objectstack/objectql` at `faa4ac206a`:
- full suite (`pnpm --filter @objectstack/objectql test`): 388 files,
7628 tests passed;
- `typecheck`: green, including `check:test-typecheck` (the new test
file adds no debt).
- At `4e096ce935`. The two later commits touch only objectql test files,
which none of these suites reads.
- `@objectstack/rest`: 263 files, 4951 passed, 326 skipped.
- `@objectstack/runtime`: 340 files, 4776 passed, 19 skipped.
- Dogfood shard 1/3: 76 files, 563 passed.
- Dogfood shard 2/3: 76 files, 540 passed, 1 skipped.
- Dogfood shard 3/3: 75 files passed and 1 skipped; 669 tests passed, 8
skipped.
- Gates: `node scripts/pm/dispatch-gates.mjs --commands` was derived at
`faa4ac206a` (97 commands). All 97 were run on that head and each exited
0, after the dists that `check:skill-examples` and
`check:dual-build-cjs-loads` read were built. `--ran` reconciles 97
derived, 97 run, 0 NOT-MEASURED, 0 UNRUN.
- Lint, narrowed to the three touched TypeScript files at `faa4ac206a`:
`eslint --no-inline-config --format json` reports 3 files, 0 errors and
0 warnings. The narrowing is a measurement, not a skip: `eslint
--print-config` resolves a config (5 rules) for each file, so they are
in the linted population. The config enables no type-aware linting
(`parserOptions.project` is null), so this diff cannot change the result
for any untouched file. The repo-wide `pnpm lint` is left to CI.
## Acceptance notes
- `content/docs/data-modeling/field-types.mdx` says `cascade` and
`restrict` are "the values honored as written". That sentence describes
how a field's behaviour resolves, and it is still true per field. The
cascade-set rule is a delete-time rule and is documented in
`data-api.mdx` (above). No other doc page states a refusal this change
removes.
- `ObjectQL.MAX_CASCADE_DEPTH` (10) is not threaded through the
recursion: `delete()` calls `cascadeDeleteRelations` with the default
depth 0, so the bound never fires. That was true before this PR, and it
is why a data cycle recursed without end. The entered-row skip now ends
a cycle within one cascade. The dead bound is left as it was.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01EUBvqtauTDmHi2ZgY759p2)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 117d34d commit c8c803c
5 files changed
Lines changed: 947 additions & 166 deletions
File tree
- .changeset
- content/docs/api
- packages/objectql/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
257 | 257 | | |
258 | 258 | | |
259 | 259 | | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
260 | 270 | | |
261 | 271 | | |
262 | 272 | | |
| |||
0 commit comments