From b10b603aad14bdfece43c1f72a4540e8f23d7b3b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:18:21 +0000 Subject: [PATCH] fix(docs,objectql): make the explicit-vs-defaulted set_null claim true and pin the measured behaviour The escalation in cascadeDeleteRelations tests the RESOLVED deleteBehavior, after `deleteBehavior || 'set_null'` has erased the difference between an absent value and an authored one. So an explicitly written `deleteBehavior: 'set_null'` on a required lookup escalates to restrict exactly like the default. Four documentation surfaces and the engine's own comment said or implied otherwise; nothing pinned either reading. Measured, then described: docs and comment corrected, current behaviour pinned by fixtures. No engine behaviour change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- content/docs/api/data-api.mdx | 7 +- content/docs/data-modeling/field-types.mdx | 4 +- content/docs/deployment/troubleshooting.mdx | 13 +- content/docs/protocol/objectql/types.mdx | 28 ++- .../src/engine-cascade-delete.test.ts | 191 +++++++++++++++++- packages/objectql/src/engine.ts | 33 ++- 6 files changed, 255 insertions(+), 21 deletions(-) diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index 5e129d0757..e5fb6d80ae 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -223,7 +223,12 @@ Delete a record. **Response**: `{ object: "account", id: "1", success: true }` Every relation pointing at the deleted record honours its own `deleteBehavior` -(`cascade` / `set_null` / `restrict`). On a `multiple: true` reference, `set_null` +(`cascade` / `set_null` / `restrict`) — with one substitution: on a +`required: true` lookup, `set_null` is escalated to `restrict` and the delete is +refused with `409 DELETE_RESTRICTED`. That happens whether the `set_null` was +defaulted or written out explicitly (see +[Required foreign keys](/docs/protocol/objectql/types#lookup)). On a +`multiple: true` reference where `set_null` does run, it removes just the deleted id from the array and keeps the rest, and a reference set emptied that way reads back as `[]` — never `null`, so a client that branches on `null` for "no link" misses the emptied case (the `multiple` doc block in diff --git a/content/docs/data-modeling/field-types.mdx b/content/docs/data-modeling/field-types.mdx index 5fcd2fc5df..9c61cbfa88 100644 --- a/content/docs/data-modeling/field-types.mdx +++ b/content/docs/data-modeling/field-types.mdx @@ -315,7 +315,7 @@ Reference to a record in another object (foreign key). |:---|:---|:---|:---| | `reference` | `string` | **required** | Target object name (snake_case) | | `referenceFilters` | `string[]` | — | **Removed** (#2377, ADR-0049) — no longer a recognized field property (unknown keys are stripped by the schema). Use structured `lookupFilters` + `dependsOn` instead; see [Relationships](/docs/data-modeling/relationships) | -| `deleteBehavior` | `'restrict' \| 'cascade' \| 'set_null'` | `'set_null'` | Behavior when referenced record is deleted (a *required* lookup left at the default `set_null` is escalated to `restrict`, since a NOT NULL foreign key cannot be cleared). On a `multiple: true` lookup `set_null` removes only the deleted **member** — the other members are kept, and a set emptied that way is stored as `[]`, never `null` | +| `deleteBehavior` | `'restrict' \| 'cascade' \| 'set_null'` | `'set_null'` | Behavior when referenced record is deleted. On a *required* lookup `set_null` is escalated to `restrict`, since a NOT NULL foreign key cannot be cleared — **whether the `set_null` was defaulted or written out explicitly**, and on a `multiple: true` required lookup too. `cascade` and `restrict` are the values honored as written. Where `set_null` does run, a `multiple: true` lookup loses only the deleted **member** — the other members are kept, and a set emptied that way is stored as `[]`, never `null` | ```typescript { name: 'company', label: 'Company', type: 'lookup', reference: 'account' } @@ -337,7 +337,7 @@ Parent-child relationship (cascading delete by default). |:---|:---|:---|:---| | `reference` | `string` | **required** | Target (master) object name | | `referenceFilters` | `string[]` | — | **Removed** (#2377, ADR-0049) — no longer a recognized field property (unknown keys are stripped by the schema). Use structured `lookupFilters` + `dependsOn` instead; see [Relationships](/docs/data-modeling/relationships) | -| `deleteBehavior` | `'restrict' \| 'cascade' \| 'set_null'` | `'cascade'` | Behavior when parent is deleted (master-detail cascades unless set to `restrict`) | +| `deleteBehavior` | `'restrict' \| 'cascade' \| 'set_null'` | `'cascade'` | Behavior when parent is deleted. `restrict` is the only value that deviates: master-detail cascades on everything else, so an explicit `set_null` here is **not** honored — the child is deleted with the parent | | `inlineEdit` | `boolean \| 'grid' \| 'form'` | — | Edit child records inline on the parent create/edit form (`true` = auto-pick, `'grid'`, or `'form'`) | | `inlineColumns` | `array` | — | Optional explicit inline grid columns | | `inlineAmountField` | `string` | — | Numeric child field used for the inline running total | diff --git a/content/docs/deployment/troubleshooting.mdx b/content/docs/deployment/troubleshooting.mdx index b49b42ff41..f8f0871452 100644 --- a/content/docs/deployment/troubleshooting.mdx +++ b/content/docs/deployment/troubleshooting.mdx @@ -204,17 +204,22 @@ client.data.find('project_task', { /* query */ }); ### "Cannot delete record: delete restricted" -**Cause:** The record has dependent child records via `lookup` or `master_detail` fields with `deleteBehavior: 'restrict'`. +**Cause:** The record has dependent child records via a `lookup` or `master_detail` field, and that field resolves to `restrict`. Two routes get there: + +1. The field declares `deleteBehavior: 'restrict'`. +2. The field is a `required: true` lookup whose behavior is `set_null`. A required foreign key cannot be cleared, so `set_null` is escalated to `restrict` — including when `set_null` is written out explicitly, and including a `required: true` lookup with `multiple: true`. Check the refusal's `developerMessage`: the escalated route says `( is required, so it cannot be cleared)`. **Fix:** -1. Delete the dependent records first -2. Change `deleteBehavior` to `'cascade'` (deletes children) or `'set_null'` (clears reference) +1. Delete or reassign the dependent records first +2. Change `deleteBehavior` to `'cascade'` (deletes children), or make the child's reference optional and use `'set_null'` (clears reference) ```typescript // Option A: Cascade delete (children are deleted with parent) { name: 'project', type: 'master_detail', reference: 'project', deleteBehavior: 'cascade' } -// Option B: Set null (children keep existing, reference cleared) +// Option B: Set null (children keep existing, reference cleared). +// The field must NOT be required — on a required lookup `set_null` is +// escalated to `restrict` and writing it explicitly changes nothing. { name: 'project', type: 'lookup', reference: 'project', deleteBehavior: 'set_null' } ``` diff --git a/content/docs/protocol/objectql/types.mdx b/content/docs/protocol/objectql/types.mdx index 1649e47f63..55d3f68a45 100644 --- a/content/docs/protocol/objectql/types.mdx +++ b/content/docs/protocol/objectql/types.mdx @@ -636,13 +636,27 @@ const opportunities = await engine.find('opportunity', { > `packages/spec/src/data/field.zod.ts` (rendered in the > [Field reference](/docs/references/data/field)). -> **Required foreign keys.** A `required: true` lookup cannot be nulled, so the -> *default* `set_null` automatically escalates to `restrict` on such a field — -> deleting the parent is refused with `409 DELETE_RESTRICTED` (the response -> carries `dependentObject` and `dependentCount`) instead of a confusing -> "<field> is required" validation error. To delete the children along with -> the parent, set `deleteBehavior: cascade` explicitly. An explicit `set_null` -> or `cascade` is always honored as written. +> **Required foreign keys.** A `required: true` lookup cannot be nulled, so +> `set_null` escalates to `restrict` on such a field — deleting the parent is +> refused with `409 DELETE_RESTRICTED` (the response carries `dependentObject` +> and `dependentCount`) instead of a confusing "<field> is required" +> validation error. To delete the children along with the parent, set +> `deleteBehavior: cascade` explicitly. +> +> The escalation applies to **any** `set_null` on a required lookup — the +> default and one written out as `deleteBehavior: set_null` alike. The engine +> tests the *resolved* behavior, so it cannot tell the two apart: writing +> `set_null` explicitly on a required lookup does not opt out of the refusal, +> and it does not change the outcome in any way. `cascade` and `restrict` are +> the two values that are honored as written. On a `multiple: true` required +> lookup the refusal comes first as well, before the member-removal rule below +> applies — so the parent delete is refused even when the child's set holds +> other members. +> +> On `master_detail` the same reading applies from the other side: `restrict` +> is the only value that deviates from `cascade`, so an explicit +> `deleteBehavior: set_null` on a master-detail reference is *not* honored — +> the child is cascaded away. > > The refusal carries **two** messages, for two audiences. `error` is written for > the person who clicked delete: it is rendered in the caller's locale from the diff --git a/packages/objectql/src/engine-cascade-delete.test.ts b/packages/objectql/src/engine-cascade-delete.test.ts index 3608f22809..b8f50738c5 100644 --- a/packages/objectql/src/engine-cascade-delete.test.ts +++ b/packages/objectql/src/engine-cascade-delete.test.ts @@ -8,10 +8,32 @@ * default to `set_null`, issuing an UPDATE that cleared the required FK — which * the child's validator rejected with a misleading " is required" 400 * naming a field that isn't even on the object being deleted (CRM e2e gap). - * A required FK can't be nulled, so the defaulted `set_null` now escalates to - * `restrict`: the delete is refused with a clear dependent-count message + * A required FK can't be nulled, so `set_null` escalates to `restrict`: the + * delete is refused with a clear dependent-count message * (`DELETE_RESTRICTED`, 409). Explicit `cascade`/`restrict` and optional * (nullable) lookups are unaffected. + * + * ## [#9625] What "explicit" does and does not buy you + * + * The escalation tests the RESOLVED behavior, one statement after + * `deleteBehavior || 'set_null'` has already erased the difference between an + * absent value and an authored one. So an explicitly written + * `deleteBehavior: 'set_null'` on a required lookup escalates exactly like the + * default — measured, not inferred, and pinned below. + * + * That was an UNPINNED divergence, which is why it survived: this file covered + * a defaulted `set_null` (escalates) and an explicit `cascade` (honored) and + * nothing between them, so the docs sentence claiming an explicit `set_null` is + * "always honored as written" contradicted the engine with every gate green. + * Two more shapes are pinned alongside it for the same reason — a required + * `multiple: true` lookup is refused even when member removal would leave the + * set non-empty, and a `master_detail` declaring an explicit `set_null` is + * silently resolved to `cascade`. + * + * These pin CURRENT behaviour. Whether the multi-value refusal should judge + * emptiness instead of presence, and whether the spec should reject `set_null` + * on a `master_detail` at publish time rather than dropping it at delete time, + * are open questions carded separately — not decided by this suite. */ import { describe, it, expect, beforeEach } from 'vitest'; @@ -55,17 +77,94 @@ const taskCascade = { account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true, deleteBehavior: 'cascade' }, }, }; +// [#9625] The fixture the divergence existed for: a required FK carrying an +// EXPLICITLY WRITTEN `set_null`. Before this file pinned it, coverage had the +// defaulted `set_null` (escalates) and an explicit `cascade` (honored) and +// nothing in between, so both readings of "does writing it out opt me out?" +// were compatible with a green suite. +const quoteExplicitSetNull = { + name: 'quote', + label: 'Quote', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + account: { + name: 'account', type: 'lookup' as const, reference: 'acct', + required: true, deleteBehavior: 'set_null', + }, + }, +}; +// [#9625] Required + `multiple: true`: the escalation runs BEFORE the +// member-removal branch and keys on `required` alone, so the refusal lands +// even when removal would leave the set non-empty. +const rosterRequiredMulti = { + name: 'roster', + label: 'Roster', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + accounts: { + name: 'accounts', type: 'lookup' as const, reference: 'acct', + required: true, multiple: true, deleteBehavior: 'set_null', + }, + }, +}; +// [#9625] The control for the pair above — same shape, `required` dropped. +// Without it, a suite that only asserted the refusal could not tell +// "refused because required" from "refused because multi-value". +const watchlistOptionalMulti = { + name: 'watchlist', + label: 'Watchlist', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + accounts: { + name: 'accounts', type: 'lookup' as const, reference: 'acct', + multiple: true, deleteBehavior: 'set_null', + }, + }, +}; +// [#9625] The neighbouring resolution that collapses the same two facts: +// `master_detail` maps every non-`restrict` value onto `cascade`, so an +// explicit `set_null` here is dropped without a word. +const lineExplicitSetNull = { + name: 'line', + label: 'Line Item', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + parent: { + name: 'parent', type: 'master_detail' as const, reference: 'acct', + deleteBehavior: 'set_null', + }, + }, +}; function makeStubDriver() { const stores = new Map>>(); const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; let nextId = 0; + // [#9625] `$contains` and `$or` are answered because `referenceProbeFilter` + // spells a `multiple: true` reference probe that way (#9362) — a double + // that ignored them would report "no dependents" for every multi-value + // relation and turn the refusals asserted below into silent successes, + // which is the fail-OPEN direction #8895 ruled out for this guard. + // `$contains` is answered as MEMBERSHIP over the stored array, matching + // what the engine narrows to afterwards via `storedReferenceIncludes`. + const matchOne = (stored: unknown, spec: unknown): boolean => { + if (spec !== null && typeof spec === 'object' && !Array.isArray(spec)) { + const [op, cmp] = Object.entries(spec as Record)[0] ?? []; + if (op === '$contains') { + const values = Array.isArray(stored) ? stored : [stored]; + return values.some((v) => v != null && typeof v !== 'object' && String(v) === String(cmp)); + } + if (op === '$eq') return (stored ?? null) === ((cmp as any) ?? null); + return false; + } + return (stored ?? null) === ((spec as any) ?? null); + }; const matches = (row: Record, where: any): boolean => { if (!where || typeof where !== 'object') return true; for (const [k, v] of Object.entries(where)) { + if (k === '$or') { if (!(v as any[]).some((sub) => matches(row, sub))) return false; continue; } if (k.startsWith('$')) continue; - const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; - if ((row[k] ?? null) !== (exp ?? null)) return false; + if (!matchOne(row[k], v)) return false; } return true; }; @@ -99,7 +198,10 @@ describe('cascadeDeleteRelations — required FK escalates set_null → restrict const { driver } = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - for (const o of [acct, oppRequired, noteOptional, taskCascade]) engine.registry.registerObject(o); + for (const o of [ + acct, oppRequired, noteOptional, taskCascade, + quoteExplicitSetNull, rosterRequiredMulti, watchlistOptionalMulti, lineExplicitSetNull, + ]) engine.registry.registerObject(o); }); it('refuses to delete a parent with a REQUIRED-FK child (DELETE_RESTRICTED, 409) and leaves both rows', async () => { @@ -147,6 +249,85 @@ describe('cascadeDeleteRelations — required FK escalates set_null → restrict expect(await engine.findOne('task', { where: { id: t.id } })).toBeNull(); }); + // ── [#9625] Explicit vs defaulted `set_null` on a required lookup ────── + // + // The escalation two lines above the probe reads the RESOLVED behavior, by + // which point `deleteBehavior: 'set_null'` and an absent `deleteBehavior` + // are the same string. These pin that consequence in both directions: the + // explicit spelling escalates exactly like the default (first two), and + // the values that really are honored as written still are (`cascade` + // above, and the optional multi-value control below). + + it('[#9625] escalates an EXPLICITLY written deleteBehavior:set_null on a required lookup, exactly like the default', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + const q = await engine.insert('quote', { account: a.id }); + + // ADR-0112 envelope — `code` AND `status`, never a bare toThrow(): an + // unescalated engine would fail this by throwing the child validator's + // "account is required" 400 instead, which a bare toThrow() accepts. + const err = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toMatchObject({ + code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'quote', dependentCount: 1, + }); + // The refusal is attributed to `required`, not to an authored + // `restrict` — this is the sentence that tells an author why writing + // `set_null` did not take effect. + expect(err.developerMessage).toContain('account is required, so it cannot be cleared'); + + // Nothing moved: the parent survives and the FK was never cleared. + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy(); + expect((await engine.findOne('quote', { where: { id: q.id } }) as any).account).toBe(a.id); + }); + + it('[#9625] refuses a required MULTI-VALUE lookup even when member removal would leave the set non-empty', async () => { + // The escalation runs before the multi-value branch and keys on + // `required` alone, so the other live member does not save the delete. + // Pinned as CURRENT behaviour, deliberately not changed here: `[]` + // still satisfies `required` in the record validator (#9476), so the + // blanket refusal is what stops an emptied required set landing + // silently. + const a = await engine.insert('acct', { name: 'Acme' }); + const b = await engine.insert('acct', { name: 'Beta' }); + const r = await engine.insert('roster', { accounts: [a.id, b.id] }); + + const err = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toMatchObject({ + code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'roster', dependentCount: 1, + }); + // The set is untouched — no member removal ran. + expect((await engine.findOne('roster', { where: { id: r.id } }) as any).accounts).toEqual([a.id, b.id]); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy(); + }); + + it('[#9625] control: the same multi-value shape WITHOUT required removes the member and deletes the parent', async () => { + // Pairs with the test above: it is `required`, not multi-valued-ness, + // that produces the refusal. Without this the suite could not tell the + // two causes apart, and a change that refused every multi-value delete + // would sit green. + const a = await engine.insert('acct', { name: 'Acme' }); + const b = await engine.insert('acct', { name: 'Beta' }); + const w = await engine.insert('watchlist', { accounts: [a.id, b.id] }); + + await engine.delete('acct', { where: { id: a.id } } as any); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); + expect((await engine.findOne('watchlist', { where: { id: w.id } }) as any).accounts).toEqual([b.id]); + }); + + it('[#9625] a master_detail declaring an explicit deleteBehavior:set_null still cascades', async () => { + // The neighbouring resolution with the same blind spot: `restrict` is + // the only value that deviates, so `set_null` is accepted by + // `FieldSchema` on this type and then dropped here. Pinned so the + // silent coercion is a documented fact rather than an absence. + const a = await engine.insert('acct', { name: 'Acme' }); + const l = await engine.insert('line', { parent: a.id }); + + await engine.delete('acct', { where: { id: a.id } } as any); + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); + // Cascaded away — NOT kept with a nulled `parent`, which is what + // honoring the declared `set_null` would have produced. + expect(await engine.findOne('line', { where: { id: l.id } })).toBeNull(); + }); + it('[#3023] tags the referential set_null write with __referentialFieldClear so the owner guard can exempt it', async () => { // The cascade FK clear is an engine-internal integrity write. It must // carry the server-set marker plugin-security's ownership-anchor guard diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 43a344ac72..65e8a90e6d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -10262,6 +10262,14 @@ export class ObjectQL implements IObjectQLEngine { // child FK is typically required, so set_null would be invalid). Only // an explicit `restrict` deviates. A plain lookup honors its // configured deleteBehavior (default set_null). + // + // [#9625] "Only an explicit `restrict` deviates" is the whole of it: + // every other value a master_detail can declare — including an + // explicit `deleteBehavior: 'set_null'`, which `FieldSchema` accepts + // on this type — resolves to `cascade` here, silently. Measured and + // pinned (`engine-cascade-delete.test.ts`); whether the spec should + // reject the combination at publish time instead of the engine + // dropping it at delete time is a judgement, carded separately. let behavior: string = fdef.type === 'master_detail' ? (fdef.deleteBehavior === 'restrict' ? 'restrict' : 'cascade') @@ -10274,8 +10282,29 @@ export class ObjectQL implements IObjectQLEngine { // author's intent: a required FK means the child can't exist detached, // so deleting the parent must be RESTRICTed (SQL's default for a // NOT NULL FK). Authors who want the children gone set - // deleteBehavior:'cascade' explicitly. This only escalates the - // *defaulted* set_null; an explicit cascade/restrict is untouched. + // deleteBehavior:'cascade' explicitly. + // + // [#9625] The test reads the RESOLVED `behavior`, one statement above, + // which no longer records how the value got there. So it escalates + // BOTH the defaulted set_null and one the author wrote out as + // `deleteBehavior: 'set_null'` — the two are indistinguishable here by + // construction, and both are measured escalating + // (`engine-cascade-delete.test.ts`). Only `cascade` and `restrict` are + // untouched. The predecessor of this comment claimed the escalation + // applied to the *defaulted* set_null alone; that was true of + // `cascade`/`restrict` and false of an explicit `set_null`, which is + // the half-truth #9625 measured. Say it the way the code behaves: the + // escalation is a property of the RESOLVED behavior plus `required`, + // not of what the author typed. + // + // It also runs BEFORE the `multiValued` branch below and keys on + // `required` alone, so a `multiple: true` required lookup is refused + // even when the child's set holds other members and member removal + // would leave it non-empty — a state the #9447 ruling accepts. + // Measured, pinned as current behaviour, and carded separately rather + // than changed here: today `[]` still satisfies `required` in the + // record validator (#9476), so this blanket refusal is what keeps an + // emptied required set from landing silently. if (behavior === 'set_null' && fdef.required === true) { behavior = 'restrict'; }