|
8 | 8 | * default to `set_null`, issuing an UPDATE that cleared the required FK — which |
9 | 9 | * the child's validator rejected with a misleading "<field> is required" 400 |
10 | 10 | * naming a field that isn't even on the object being deleted (CRM e2e gap). |
11 | | - * A required FK can't be nulled, so the defaulted `set_null` now escalates to |
12 | | - * `restrict`: the delete is refused with a clear dependent-count message |
| 11 | + * A required FK can't be nulled, so `set_null` escalates to `restrict`: the |
| 12 | + * delete is refused with a clear dependent-count message |
13 | 13 | * (`DELETE_RESTRICTED`, 409). Explicit `cascade`/`restrict` and optional |
14 | 14 | * (nullable) lookups are unaffected. |
| 15 | + * |
| 16 | + * ## [#9625] What "explicit" does and does not buy you |
| 17 | + * |
| 18 | + * The escalation tests the RESOLVED behavior, one statement after |
| 19 | + * `deleteBehavior || 'set_null'` has already erased the difference between an |
| 20 | + * absent value and an authored one. So an explicitly written |
| 21 | + * `deleteBehavior: 'set_null'` on a required lookup escalates exactly like the |
| 22 | + * default — measured, not inferred, and pinned below. |
| 23 | + * |
| 24 | + * That was an UNPINNED divergence, which is why it survived: this file covered |
| 25 | + * a defaulted `set_null` (escalates) and an explicit `cascade` (honored) and |
| 26 | + * nothing between them, so the docs sentence claiming an explicit `set_null` is |
| 27 | + * "always honored as written" contradicted the engine with every gate green. |
| 28 | + * Two more shapes are pinned alongside it for the same reason — a required |
| 29 | + * `multiple: true` lookup is refused even when member removal would leave the |
| 30 | + * set non-empty, and a `master_detail` declaring an explicit `set_null` is |
| 31 | + * silently resolved to `cascade`. |
| 32 | + * |
| 33 | + * These pin CURRENT behaviour. Whether the multi-value refusal should judge |
| 34 | + * emptiness instead of presence, and whether the spec should reject `set_null` |
| 35 | + * on a `master_detail` at publish time rather than dropping it at delete time, |
| 36 | + * are open questions carded separately — not decided by this suite. |
15 | 37 | */ |
16 | 38 |
|
17 | 39 | import { describe, it, expect, beforeEach } from 'vitest'; |
@@ -55,17 +77,94 @@ const taskCascade = { |
55 | 77 | account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true, deleteBehavior: 'cascade' }, |
56 | 78 | }, |
57 | 79 | }; |
| 80 | +// [#9625] The fixture the divergence existed for: a required FK carrying an |
| 81 | +// EXPLICITLY WRITTEN `set_null`. Before this file pinned it, coverage had the |
| 82 | +// defaulted `set_null` (escalates) and an explicit `cascade` (honored) and |
| 83 | +// nothing in between, so both readings of "does writing it out opt me out?" |
| 84 | +// were compatible with a green suite. |
| 85 | +const quoteExplicitSetNull = { |
| 86 | + name: 'quote', |
| 87 | + label: 'Quote', |
| 88 | + fields: { |
| 89 | + id: { name: 'id', type: 'text' as const, primaryKey: true }, |
| 90 | + account: { |
| 91 | + name: 'account', type: 'lookup' as const, reference: 'acct', |
| 92 | + required: true, deleteBehavior: 'set_null', |
| 93 | + }, |
| 94 | + }, |
| 95 | +}; |
| 96 | +// [#9625] Required + `multiple: true`: the escalation runs BEFORE the |
| 97 | +// member-removal branch and keys on `required` alone, so the refusal lands |
| 98 | +// even when removal would leave the set non-empty. |
| 99 | +const rosterRequiredMulti = { |
| 100 | + name: 'roster', |
| 101 | + label: 'Roster', |
| 102 | + fields: { |
| 103 | + id: { name: 'id', type: 'text' as const, primaryKey: true }, |
| 104 | + accounts: { |
| 105 | + name: 'accounts', type: 'lookup' as const, reference: 'acct', |
| 106 | + required: true, multiple: true, deleteBehavior: 'set_null', |
| 107 | + }, |
| 108 | + }, |
| 109 | +}; |
| 110 | +// [#9625] The control for the pair above — same shape, `required` dropped. |
| 111 | +// Without it, a suite that only asserted the refusal could not tell |
| 112 | +// "refused because required" from "refused because multi-value". |
| 113 | +const watchlistOptionalMulti = { |
| 114 | + name: 'watchlist', |
| 115 | + label: 'Watchlist', |
| 116 | + fields: { |
| 117 | + id: { name: 'id', type: 'text' as const, primaryKey: true }, |
| 118 | + accounts: { |
| 119 | + name: 'accounts', type: 'lookup' as const, reference: 'acct', |
| 120 | + multiple: true, deleteBehavior: 'set_null', |
| 121 | + }, |
| 122 | + }, |
| 123 | +}; |
| 124 | +// [#9625] The neighbouring resolution that collapses the same two facts: |
| 125 | +// `master_detail` maps every non-`restrict` value onto `cascade`, so an |
| 126 | +// explicit `set_null` here is dropped without a word. |
| 127 | +const lineExplicitSetNull = { |
| 128 | + name: 'line', |
| 129 | + label: 'Line Item', |
| 130 | + fields: { |
| 131 | + id: { name: 'id', type: 'text' as const, primaryKey: true }, |
| 132 | + parent: { |
| 133 | + name: 'parent', type: 'master_detail' as const, reference: 'acct', |
| 134 | + deleteBehavior: 'set_null', |
| 135 | + }, |
| 136 | + }, |
| 137 | +}; |
58 | 138 |
|
59 | 139 | function makeStubDriver() { |
60 | 140 | const stores = new Map<string, Map<string, Record<string, unknown>>>(); |
61 | 141 | const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; |
62 | 142 | let nextId = 0; |
| 143 | + // [#9625] `$contains` and `$or` are answered because `referenceProbeFilter` |
| 144 | + // spells a `multiple: true` reference probe that way (#9362) — a double |
| 145 | + // that ignored them would report "no dependents" for every multi-value |
| 146 | + // relation and turn the refusals asserted below into silent successes, |
| 147 | + // which is the fail-OPEN direction #8895 ruled out for this guard. |
| 148 | + // `$contains` is answered as MEMBERSHIP over the stored array, matching |
| 149 | + // what the engine narrows to afterwards via `storedReferenceIncludes`. |
| 150 | + const matchOne = (stored: unknown, spec: unknown): boolean => { |
| 151 | + if (spec !== null && typeof spec === 'object' && !Array.isArray(spec)) { |
| 152 | + const [op, cmp] = Object.entries(spec as Record<string, unknown>)[0] ?? []; |
| 153 | + if (op === '$contains') { |
| 154 | + const values = Array.isArray(stored) ? stored : [stored]; |
| 155 | + return values.some((v) => v != null && typeof v !== 'object' && String(v) === String(cmp)); |
| 156 | + } |
| 157 | + if (op === '$eq') return (stored ?? null) === ((cmp as any) ?? null); |
| 158 | + return false; |
| 159 | + } |
| 160 | + return (stored ?? null) === ((spec as any) ?? null); |
| 161 | + }; |
63 | 162 | const matches = (row: Record<string, unknown>, where: any): boolean => { |
64 | 163 | if (!where || typeof where !== 'object') return true; |
65 | 164 | for (const [k, v] of Object.entries(where)) { |
| 165 | + if (k === '$or') { if (!(v as any[]).some((sub) => matches(row, sub))) return false; continue; } |
66 | 166 | if (k.startsWith('$')) continue; |
67 | | - const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; |
68 | | - if ((row[k] ?? null) !== (exp ?? null)) return false; |
| 167 | + if (!matchOne(row[k], v)) return false; |
69 | 168 | } |
70 | 169 | return true; |
71 | 170 | }; |
@@ -99,7 +198,10 @@ describe('cascadeDeleteRelations — required FK escalates set_null → restrict |
99 | 198 | const { driver } = makeStubDriver(); |
100 | 199 | engine.registerDriver(driver, true); |
101 | 200 | await engine.init(); |
102 | | - for (const o of [acct, oppRequired, noteOptional, taskCascade]) engine.registry.registerObject(o); |
| 201 | + for (const o of [ |
| 202 | + acct, oppRequired, noteOptional, taskCascade, |
| 203 | + quoteExplicitSetNull, rosterRequiredMulti, watchlistOptionalMulti, lineExplicitSetNull, |
| 204 | + ]) engine.registry.registerObject(o); |
103 | 205 | }); |
104 | 206 |
|
105 | 207 | 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 |
147 | 249 | expect(await engine.findOne('task', { where: { id: t.id } })).toBeNull(); |
148 | 250 | }); |
149 | 251 |
|
| 252 | + // ── [#9625] Explicit vs defaulted `set_null` on a required lookup ────── |
| 253 | + // |
| 254 | + // The escalation two lines above the probe reads the RESOLVED behavior, by |
| 255 | + // which point `deleteBehavior: 'set_null'` and an absent `deleteBehavior` |
| 256 | + // are the same string. These pin that consequence in both directions: the |
| 257 | + // explicit spelling escalates exactly like the default (first two), and |
| 258 | + // the values that really are honored as written still are (`cascade` |
| 259 | + // above, and the optional multi-value control below). |
| 260 | + |
| 261 | + it('[#9625] escalates an EXPLICITLY written deleteBehavior:set_null on a required lookup, exactly like the default', async () => { |
| 262 | + const a = await engine.insert('acct', { name: 'Acme' }); |
| 263 | + const q = await engine.insert('quote', { account: a.id }); |
| 264 | + |
| 265 | + // ADR-0112 envelope — `code` AND `status`, never a bare toThrow(): an |
| 266 | + // unescalated engine would fail this by throwing the child validator's |
| 267 | + // "account is required" 400 instead, which a bare toThrow() accepts. |
| 268 | + const err = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); |
| 269 | + expect(err).toMatchObject({ |
| 270 | + code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'quote', dependentCount: 1, |
| 271 | + }); |
| 272 | + // The refusal is attributed to `required`, not to an authored |
| 273 | + // `restrict` — this is the sentence that tells an author why writing |
| 274 | + // `set_null` did not take effect. |
| 275 | + expect(err.developerMessage).toContain('account is required, so it cannot be cleared'); |
| 276 | + |
| 277 | + // Nothing moved: the parent survives and the FK was never cleared. |
| 278 | + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy(); |
| 279 | + expect((await engine.findOne('quote', { where: { id: q.id } }) as any).account).toBe(a.id); |
| 280 | + }); |
| 281 | + |
| 282 | + it('[#9625] refuses a required MULTI-VALUE lookup even when member removal would leave the set non-empty', async () => { |
| 283 | + // The escalation runs before the multi-value branch and keys on |
| 284 | + // `required` alone, so the other live member does not save the delete. |
| 285 | + // Pinned as CURRENT behaviour, deliberately not changed here: `[]` |
| 286 | + // still satisfies `required` in the record validator (#9476), so the |
| 287 | + // blanket refusal is what stops an emptied required set landing |
| 288 | + // silently. |
| 289 | + const a = await engine.insert('acct', { name: 'Acme' }); |
| 290 | + const b = await engine.insert('acct', { name: 'Beta' }); |
| 291 | + const r = await engine.insert('roster', { accounts: [a.id, b.id] }); |
| 292 | + |
| 293 | + const err = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); |
| 294 | + expect(err).toMatchObject({ |
| 295 | + code: 'DELETE_RESTRICTED', status: 409, dependentObject: 'roster', dependentCount: 1, |
| 296 | + }); |
| 297 | + // The set is untouched — no member removal ran. |
| 298 | + expect((await engine.findOne('roster', { where: { id: r.id } }) as any).accounts).toEqual([a.id, b.id]); |
| 299 | + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeTruthy(); |
| 300 | + }); |
| 301 | + |
| 302 | + it('[#9625] control: the same multi-value shape WITHOUT required removes the member and deletes the parent', async () => { |
| 303 | + // Pairs with the test above: it is `required`, not multi-valued-ness, |
| 304 | + // that produces the refusal. Without this the suite could not tell the |
| 305 | + // two causes apart, and a change that refused every multi-value delete |
| 306 | + // would sit green. |
| 307 | + const a = await engine.insert('acct', { name: 'Acme' }); |
| 308 | + const b = await engine.insert('acct', { name: 'Beta' }); |
| 309 | + const w = await engine.insert('watchlist', { accounts: [a.id, b.id] }); |
| 310 | + |
| 311 | + await engine.delete('acct', { where: { id: a.id } } as any); |
| 312 | + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); |
| 313 | + expect((await engine.findOne('watchlist', { where: { id: w.id } }) as any).accounts).toEqual([b.id]); |
| 314 | + }); |
| 315 | + |
| 316 | + it('[#9625] a master_detail declaring an explicit deleteBehavior:set_null still cascades', async () => { |
| 317 | + // The neighbouring resolution with the same blind spot: `restrict` is |
| 318 | + // the only value that deviates, so `set_null` is accepted by |
| 319 | + // `FieldSchema` on this type and then dropped here. Pinned so the |
| 320 | + // silent coercion is a documented fact rather than an absence. |
| 321 | + const a = await engine.insert('acct', { name: 'Acme' }); |
| 322 | + const l = await engine.insert('line', { parent: a.id }); |
| 323 | + |
| 324 | + await engine.delete('acct', { where: { id: a.id } } as any); |
| 325 | + expect(await engine.findOne('acct', { where: { id: a.id } })).toBeNull(); |
| 326 | + // Cascaded away — NOT kept with a nulled `parent`, which is what |
| 327 | + // honoring the declared `set_null` would have produced. |
| 328 | + expect(await engine.findOne('line', { where: { id: l.id } })).toBeNull(); |
| 329 | + }); |
| 330 | + |
150 | 331 | it('[#3023] tags the referential set_null write with __referentialFieldClear so the owner guard can exempt it', async () => { |
151 | 332 | // The cascade FK clear is an engine-internal integrity write. It must |
152 | 333 | // carry the server-set marker plugin-security's ownership-anchor guard |
|
0 commit comments