Skip to content

Commit 31a6ca7

Browse files
fix(objectql)!: a restrict reference refuses the delete of the record it protects even when the referencing row also cascades from it (#26718) (#26864)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XKKhNWtvAHeAwPmZfHrR6PA)_ Fixes #26718 Clause-②: no (narrowing) Metadata-protocol: none Skills-docs: none — the docs and skills already say a `restrict` refuses the parent delete while a referencing row exists (`content/docs/data-modeling/field-types.mdx`, `skills/objectstack-data/rules/relationships.md`); this makes the runtime honour that on one more path, and nothing taught changes. ## 维护者速读 - **改了什么**:按 id 删除父记录时,若某条明细既通过 master_detail 归属该父记录、又用 `deleteBehavior: 'restrict'` 引用同一父记录(或级联更深一层的孙记录这样引用),现在先判定 restrict,返回 409 `DELETE_RESTRICTED`,什么都不删。 - **为什么改**:以前级联把这条明细算进“本次要删的集合”,集合内的行一律不触发 restrict,于是明细被静默级联删除,作者声明“不可删”的数据丢失(p1 数据丢失)。 - **风险与回滚**:只收窄这一种删除;集合外的 restrict、普通级联、set_null、必填 lookup 的升级 restrict 行为不变。回滚即撤销本 PR。#22305 定下的“同一父记录下兄弟之间的 restrict 不拦”保持原样,是否也要拦见 Acceptance notes 的待决问题。 - **先看**:`packages/objectql/src/engine.ts` 的 `refuseRestrictsInsideOwnCascade`(新增的写前判定)与 `collectCascadeDeleteSet` 里记录级联边的几行;`packages/runtime/src/cascade-delete-restrict-same-parent-real-driver.integration.test.ts`。 - **两分钟验证**:`pnpm --filter @objectstack/runtime exec vitest run --project local src/cascade-delete-restrict-same-parent-real-driver.integration.test.ts`(先 build objectql),预期 7 条全绿;在 main 上同一文件 4 红。 - **需要你做的**:无,席位落地。 ## What changes `ObjectQL.cascadeDeleteRelations` (the by-id delete's referential walk, `packages/objectql/src/engine.ts`) gains a read-only step between its two phases. After it collects the cascade set and before its first write, `refuseRestrictsInsideOwnCascade` judges every AUTHORED `restrict` (the resolved behaviour of `cascadeRelationBehavior`: a `lookup`, `tree` or `master_detail` that declares `deleteBehavior: 'restrict'`) held by a member that the cascade reaches only THROUGH the record that restrict protects. Such a row refuses the delete with the walk's existing `409 DELETE_RESTRICTED` envelope, built by one helper (`deleteRestrictedError`, lifted unchanged out of the walk), so the two refusals cannot drift. "Reaches through" is read from the cascade edges the collection walk already probes, now recorded on the set (`CascadeDeleteSet.cascadeEdges`, `cascadeReachOf`). No new probe kind and no new read of the registry: the step probes a restrict relation only when its object has a member in the reach of the protected record. ## Why — where the ordering lives and why the cascade won Re-checked on `origin/main` `c8583f80`. The card's suspected site holds: the walk is `cascadeDeleteRelations` → `collectCascadeDeleteSet` (phase 1) → `walkReferencingRelations` (phase 2) in `packages/objectql/src/engine.ts`. Two things let the cascade win: 1. **The #22305 exemption.** Phase 2 judged `restrict` only against rows OUTSIDE the cascade set (`dependents.filter(… !cascadeSetHas(cascadeSet.members, …))`). The child is in the parent's set by its own master_detail, so its restrict lookup was never asked, in either field order. 2. **Order inside the walk.** Each relation was judged as the field loop reached it, so with the master_detail declared first the child was already deleted when the restrict relation was probed. Judging the restricts before the first write removes both. The #22305 rule itself stays for a row the cascade reaches through ANOTHER record (two details of one account, one restricting the other): the account's delete removes both. ## Tests - `packages/runtime/src/cascade-delete-restrict-same-parent-real-driver.integration.test.ts` (new): real `ObjectQL` over a real `SqlDriver` (better-sqlite3 on disk), through the protocol's `deleteData` (the method `DELETE /api/v1/data/:object/:id` serves) and `deleteManyData` (the bulk door). The card's shape in both field orders, the bulk door, a grandchild holding the restrict; controls: the two fields at different parents, a plain master_detail, the parent deleting once the child is gone. Row counts are read out of the driver. - `packages/objectql/src/engine-cascade-delete-sibling-restrict.test.ts` (new section, the #22305 file and its rollback-capable driver): the same shapes plus a grandchild restricting the member above it (refused from the root and directly alike), each asserting no write reached the driver; two controls. Every pre-existing #22305 pin in the file is unchanged and green. ## Acceptance notes - **Open question for the seat (not decided here).** The triage ruling reads "a restrict on any path refuses the delete". The #22305 triage ruling, pinned in `engine-cascade-delete-sibling-restrict.test.ts` ("an AUTHORED restrict between two members does not refuse"), exempts an authored restrict held by a SIBLING member: deleting an account removes a quote and the contact the quote restricts, both details of the account. This PR keeps that pin and refuses only the rows reached through the record the restrict protects, which covers the card's shape, the grandchild case and the reverse field order. Whether the sibling case should refuse too is a reversal of the #22305 pin and is left to the seat; it is a one-line change in the new step (drop the reach filter for authored restricts). Measured producers: hotcrm (`afdf9a0`) declares authored restricts only on `crm_event_attendee.crm_event` and `crm_campaign_member.crm_campaign`, whose targets are never cascade members together with those rows, and this repo ships one, `sys_business_unit.parent_business_unit_id`; none of them meets either rule's changed case. - **The bulk door shares the walk.** `deleteManyData` deletes by id, row by row, through `engine.delete`, and refuses the same row (pinned). The engine's PREDICATE delete (`multi: true` with a non-id `where`) runs no referential walk at all, by its documented contract (the `cascadeDeleteRelations` docblock, and `engine-cascade-delete-atomic.test.ts` "opens no transaction when … the delete is by predicate"); it is unchanged here and not reached by this card. - **Required-lookup escalation unchanged.** A required lookup's `set_null`, refused as `restrict` because it cannot be cleared, still steps aside for a row the cascade deletes (#22305 "a member holding a required lookup to the ROOT does not refuse the root", green). - **Cost.** One extra probe per authored restrict relation per member, only when the restricting object has a member in that member's cascade reach; the walk already probed the same relation. - No save-time refusal of the combination was added (that would be a `packages/spec` card). No `ui/**`, no `packages/spec/**`. - Overlapping open PR measured before opening: #26792 (engine split cut 16, the validate body) touches `engine.ts` in a different region. ## Verification All readings on head `ba42fbd96c` (branch merged with `origin/main` `0ea90c7a18`) unless named otherwise. <details><summary>Red first on main, green after (real driver)</summary> `pnpm --filter @objectstack/runtime exec vitest run --project local src/cascade-delete-restrict-same-parent-real-driver.integration.test.ts` - On `c8583f80` (main, test file only): `Tests 4 failed | 3 passed (7)`. The card shape answered success in both field orders (`expected { object: 'zz_widget_part', … } to be an instance of Error`), the bulk door answered `success: true`, and the grandchild case answered no code. The three controls were green. - On `ba42fbd96c`: `Tests 7 passed (7)`. </details> <details><summary>Ablation (fix committed first, restored by blob hash)</summary> `node scripts/ablation-replace.mjs` replaced the `refuseRestrictsInsideOwnCascade` call in `cascadeDeleteRelations` with a no-op (anchor 1 → 0, blob `024eb5bf8ca8` → `08a2e3488091`) and ran `engine-cascade-delete-sibling-restrict.test.ts`: `Tests 4 failed | 23 passed (27)`. The four that went red are the new non-control cases; both new controls and every #22305 pin stayed green. Restored: blob after restore = blob at HEAD = `024eb5bf8ca8`, `git diff HEAD` empty. The objectql suite imports `./engine.js` from source, so no build was involved. </details> <details><summary>Package suites, typecheck, lint</summary> - `pnpm --filter @objectstack/objectql test`: `Test Files 475 passed (475)`, `Tests 8849 passed (8849)` (on `40aace604d`, the same objectql source as the head). - `engine-cascade-delete-sibling-restrict.test.ts` 27/27 and the new runtime file 7/7 again on `ba42fbd96c`. - `pnpm --filter @objectstack/objectql typecheck && pnpm --filter @objectstack/runtime typecheck`: exit 0 (both include `check:test-typecheck`). - `pnpm exec eslint --no-inline-config` over the three changed source files: exit 0. </details> <details><summary>Derived gates</summary> `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derives 63 families on this diff (unchanged after the merge). All 63 ran on `ba42fbd96c`, every one exit 0; `--ran` reconciles `63 run, 0 NOT-MEASURED, 0 UNRUN`. One read needed a rerun: `check:watch-hint-literal` first exited 1 on `ENOENT … packages/spec/tsup.config.bundled_….mjs`, a temp file a concurrent spec build removed mid-scan; rerun after the build, exit 0. `check-plugin-teardown-shape.mjs --self-test` needs the unshallowed clone (exit 3 on the shallow one, exit 0 after `git fetch --unshallow`). </details> <details><summary>Dogfood battery (narrowing arm)</summary> On `ba42fbd96c`, after `pnpm --workspace-concurrency=2 --filter '@objectstack/dogfood^...' build` (exit 0), `pnpm --filter @objectstack/dogfood test` under the verify lock: `VERDICT command-exit 0`, `Test Files 314 passed | 3 skipped (317)`, `Tests 2609 passed | 20 skipped (2629)`, no failure. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (No `packages/spec` change.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. (eslint over the changed files; repo-wide lint is CI's.) - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (Not touched.) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_01FEkQ5mmLAr61euRoHc183L)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 031d97a commit 31a6ca7

4 files changed

Lines changed: 587 additions & 80 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@objectstack/objectql': patch
3+
---
4+
5+
fix(objectql)!: a `deleteBehavior: 'restrict'` reference refuses the delete of the record it protects even when the referencing row would be cascade-deleted by that same delete (#26718)
6+
7+
Clause-②: no (narrowing) — a by-id delete (`delete` by id, `DELETE /api/v1/data/:object/:id`, and the bulk `deleteManyData` door, which deletes by id row by row) refuses with 409 `DELETE_RESTRICTED` where it succeeded: when a row the delete would cascade-delete only through the record it restricts holds an authored `restrict` reference to that record. Nothing is widened.
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped, and no export or type changes: an authored deleteBehavior restrict is now honoured on a row that also cascades from the record it protects. The refusal is the existing DELETE_RESTRICTED, so there is nothing for a migration to rewrite and no tombstone. -->
11+
12+
**BREAKING** accept-set narrowing at the by-id delete (`@objectstack/objectql`), served by every delete door that deletes by id.
13+
14+
- **Before.** A child holding a `master_detail` and a `deleteBehavior: 'restrict'` lookup to the same parent row: deleting the parent answered success, the cascade deleted the child, and the restrict was never asked. The same held for a grandchild under that parent holding the restrict. The delete collects every record its cascade removes first, and judged a `restrict` only against rows outside that set; the child is in the set by its own master_detail.
15+
- **Now.** Before the first write, the delete judges every authored `restrict` (on a `lookup`, a `tree` or a `master_detail`) held by a row it would remove only because the record that restrict protects is removed. Such a row refuses the delete with the existing `409 DELETE_RESTRICTED`, naming the restricting object (`dependentObject`) and field, and nothing is deleted. The answer no longer depends on the order the two fields are declared in.
16+
- **Unchanged.** A restrict held by a row outside the delete's cascade still refuses, as before. A row the cascade reaches through another record is still exempt: deleting an account still deletes a contract and the contact the contract restricts when both are details of that account. A required lookup's `set_null`, which is refused as `restrict` because it cannot be cleared, still steps aside for a row the cascade deletes. A plain `master_detail` with no restrict still cascades, and `set_null` still clears.
17+
- **Who sees it.** An app with a detail that also restricts its own master (directly or from further down the cascade): deleting that master used to destroy the detail, and is now refused while the detail references it.
18+
19+
**The remedy.** Delete the restricting detail (or clear its restrict reference) first, then delete the parent. If the detail is meant to go with its parent, drop the `restrict` from the second reference.

‎packages/objectql/src/engine-cascade-delete-sibling-restrict.test.ts‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,3 +485,108 @@ describe('[#22305] a cycle in the data terminates', () => {
485485
expect(driver.writes.filter((w: Write) => w.op === 'delete' && w.object === 'zz_item')).toHaveLength(2);
486486
});
487487
});
488+
489+
// ── [#26718] A restrict held by a row the cascade reaches THROUGH its target ─
490+
//
491+
// The rule above exempts a member from refusing a delete of ANOTHER member.
492+
// It never exempted a row the cascade reaches only because the record it
493+
// restricts is deleted: a child holding a master_detail AND an authored
494+
// restrict lookup to the same parent was in the parent's set by its own
495+
// master_detail, so its restrict was never asked and the cascade deleted it.
496+
// Every such restrict is now judged before the first write (triage on
497+
// #26718); the real-driver, protocol-door face of the same pins is
498+
// `packages/runtime/src/cascade-delete-restrict-same-parent-real-driver.integration.test.ts`.
499+
500+
/** A detail of the account that also restricts the account itself. */
501+
const pledge = (mdFirst: boolean) => {
502+
const md = { name: 'account', type: 'master_detail' as const, reference: 'zz_account' };
503+
const guard = { name: 'guard', type: 'lookup' as const, reference: 'zz_account', deleteBehavior: 'restrict' };
504+
return {
505+
name: 'zz_pledge',
506+
label: 'Pledge',
507+
fields: mdFirst ? { id, name, account: md, guard } : { id, name, guard, account: md },
508+
};
509+
};
510+
/** A detail of a contact, restricting the account or the contact above it. */
511+
const note = {
512+
name: 'zz_note',
513+
label: 'Note',
514+
fields: {
515+
id, name,
516+
contact: { name: 'contact', type: 'master_detail' as const, reference: 'zz_contact' },
517+
guard_account: { name: 'guard_account', type: 'lookup' as const, reference: 'zz_account', deleteBehavior: 'restrict' },
518+
guard_contact: { name: 'guard_contact', type: 'lookup' as const, reference: 'zz_contact', deleteBehavior: 'restrict' },
519+
},
520+
};
521+
522+
describe('[#26718] a restrict on a row the cascade reaches through the record it protects refuses', () => {
523+
it.each([
524+
['master_detail declared first', true],
525+
['restrict lookup declared first', false],
526+
] as Array<[string, boolean]>)('a child restricting its own master refuses, and nothing is written (%s)', async (_l, mdFirst) => {
527+
const { engine, driver } = await makeEngine([account, pledge(mdFirst)]);
528+
const acc = await engine.insert('zz_account', { name: 'Acme' });
529+
await engine.insert('zz_pledge', { name: 'p', account: acc.id, guard: acc.id });
530+
driver.writes.length = 0;
531+
532+
await expect(engine.delete('zz_account', { where: { id: acc.id } } as any))
533+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, object: 'zz_account', dependentObject: 'zz_pledge' });
534+
expect(driver.writes).toEqual([]);
535+
expect(driver.rowsOf('zz_account')).toHaveLength(1);
536+
expect(driver.rowsOf('zz_pledge')).toHaveLength(1);
537+
});
538+
539+
it('a grandchild restricting the root refuses the root, and nothing is written', async () => {
540+
const { engine, driver } = await makeEngine([account, contact, note]);
541+
const acc = await engine.insert('zz_account', { name: 'Acme' });
542+
const c = await engine.insert('zz_contact', { name: 'c', account: acc.id });
543+
await engine.insert('zz_note', { name: 'n', contact: c.id, guard_account: acc.id });
544+
driver.writes.length = 0;
545+
546+
await expect(engine.delete('zz_account', { where: { id: acc.id } } as any))
547+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, object: 'zz_account', dependentObject: 'zz_note' });
548+
expect(driver.writes).toEqual([]);
549+
expect(driver.rowsOf('zz_contact')).toHaveLength(1);
550+
expect(driver.rowsOf('zz_note')).toHaveLength(1);
551+
});
552+
553+
it('a grandchild restricting the member above it refuses the root delete that reaches both', async () => {
554+
const { engine, driver } = await makeEngine([account, contact, note]);
555+
const acc = await engine.insert('zz_account', { name: 'Acme' });
556+
const c = await engine.insert('zz_contact', { name: 'c', account: acc.id });
557+
await engine.insert('zz_note', { name: 'n', contact: c.id, guard_contact: c.id });
558+
driver.writes.length = 0;
559+
560+
await expect(engine.delete('zz_account', { where: { id: acc.id } } as any))
561+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, object: 'zz_contact', dependentObject: 'zz_note' });
562+
expect(driver.writes).toEqual([]);
563+
// …the same answer deleting that member directly gives.
564+
await expect(engine.delete('zz_contact', { where: { id: c.id } } as any))
565+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'zz_note' });
566+
expect(driver.rowsOf('zz_note')).toHaveLength(1);
567+
});
568+
569+
it('control: the same restrict pointing at a record the delete does not reach is not asked', async () => {
570+
const { engine, driver } = await makeEngine([account, pledge(true)]);
571+
const acc = await engine.insert('zz_account', { name: 'Acme' });
572+
const other = await engine.insert('zz_account', { name: 'Other' });
573+
await engine.insert('zz_pledge', { name: 'p', account: acc.id, guard: other.id });
574+
575+
await engine.delete('zz_account', { where: { id: acc.id } } as any);
576+
577+
expect(driver.rowsOf('zz_pledge')).toHaveLength(0);
578+
expect(driver.rowsOf('zz_account').map((r: Row) => r.id)).toEqual([other.id]);
579+
});
580+
581+
it('control: deleting the restricted record of a pledge owned elsewhere still refuses', async () => {
582+
const { engine, driver } = await makeEngine([account, pledge(true)]);
583+
const owner = await engine.insert('zz_account', { name: 'Owner' });
584+
const guarded = await engine.insert('zz_account', { name: 'Guarded' });
585+
await engine.insert('zz_pledge', { name: 'p', account: owner.id, guard: guarded.id });
586+
587+
await expect(engine.delete('zz_account', { where: { id: guarded.id } } as any))
588+
.rejects.toMatchObject({ code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'zz_pledge' });
589+
expect(driver.rowsOf('zz_account')).toHaveLength(2);
590+
expect(driver.rowsOf('zz_pledge')).toHaveLength(1);
591+
});
592+
});

0 commit comments

Comments
 (0)