Skip to content

Commit 91c6c28

Browse files
os-zhuangclaude
andauthored
fix(engine): set_null on a multiple:true reference removes the deleted member, emptied set stored as [] per the ruled FieldSchema contract; drop the #9437 interim 409 hold (#9520)
The set_null limb on a set-valued foreign key now filters the deleted id out of the stored array and writes what remains; an emptied set is written as [], never null — consuming the representation FieldSchema pins (packages/spec/src/data/field.zod.ts, multiple/required doc blocks, #9447 maintainer ruling 2026-08-18). The temporary restrict escalation and its TEMPORARY developerMessage limb shipped with #9437 are removed in the same stroke, as ruled. Closes #9438. Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0bb8dbd commit 91c6c28

4 files changed

Lines changed: 217 additions & 147 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(engine): `deleteBehavior: 'set_null'` on a `multiple: true` reference field removes the deleted MEMBER from the stored array instead of nulling the whole slot, and the temporary 409 hold shipped with #9437 is removed (#9438)
6+
7+
On a set-valued foreign key, "set null" now means what it has to mean: the
8+
reference to the deleted record is filtered out of the stored array and the
9+
remaining members are written back untouched. Before the interim hold, this
10+
limb wrote `null` over the WHOLE array — a row holding `["acc_a","acc_b"]`
11+
re-read as `null` after `acc_a` was deleted, silently dropping the live
12+
reference to `acc_b`.
13+
14+
**The residual shape is the ruled one, consumed rather than decided here.**
15+
When the last member is removed, the field is written as **`[]`, never
16+
`null`** — the representation `FieldSchema` pins as verbatim contract
17+
(`packages/spec/src/data/field.zod.ts`, the `multiple` and `required` doc
18+
blocks; #9447, maintainer ruling 2026-08-18, binding for every writer). The
19+
open question that kept this limb held back was exactly that shape; it is
20+
now answered at the spec, and this write consumes the answer.
21+
22+
**The temporary holding position is removed in the same stroke — it was
23+
built to be removed.** #9437 shipped an explicit interim: any delete that
24+
would take the `set_null` limb on a multi-value reference was refused
25+
`DELETE_RESTRICTED` / 409, with a `developerMessage` naming the refusal
26+
TEMPORARY and citing the tracking issue literally. Those deletes now
27+
succeed and remove the member. The refusal envelope for a genuinely
28+
configured `restrict` is unchanged, and the interim's extra sentence is gone
29+
with the interim.
30+
31+
Removal compares whole members (`String(v) !== String(id)`), the same
32+
reading the dependents narrowing already applies — the probe's `$contains`
33+
pushdown is a substring superset, so an id that is a prefix of another
34+
neither loses its own member nor takes its neighbor's. Every other
35+
disposition is untouched: `cascade` still deletes dependents, a
36+
single-valued `set_null` still clears its foreign key to `null`, an
37+
explicit `restrict` still refuses with its own sentence, and the required-FK
38+
escalation stays exactly as it was.
39+
40+
Pinned against the driver double that models the JSON TEXT column and
41+
against a real `SqlDriver` on better-sqlite3 through the real data-plane
42+
delete: member removal with a surviving sibling that still resolves, the
43+
emptying case asserted literally as `[]` and not `null` on a re-read of the
44+
database, and the controls beside them.
45+
46+
Note: `required` on a multi-value lookup now *documents* non-empty-array
47+
semantics (same ruling), but the validator does not yet enforce it — that
48+
enforcement gap is tracked separately in #9476 and is not changed here.

packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts

Lines changed: 95 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,9 @@ const guard: ServiceObject = {
8989
};
9090

9191
/** Multi-value, `set_null` spelled OUT — `||` collapses it with the default. */
92-
const holdExplicit: ServiceObject = {
93-
name: 'mv_hold_explicit',
94-
label: 'Hold (explicit set_null)',
92+
const explicitSetNull: ServiceObject = {
93+
name: 'mv_explicit_set_null',
94+
label: 'Explicit set_null',
9595
fields: {
9696
id: { name: 'id', label: 'ID', type: 'text' as const },
9797
accounts: {
@@ -145,7 +145,7 @@ const opp: ServiceObject = {
145145
const JSON_COLUMNS: Record<string, readonly string[]> = {
146146
mv_zoo: ['f_lookups'],
147147
mv_guard: ['accounts'],
148-
mv_hold_explicit: ['accounts'],
148+
mv_explicit_set_null: ['accounts'],
149149
mv_cascade_multi: ['accounts'],
150150
};
151151

@@ -309,7 +309,7 @@ describe('[#9362] the dependents probe reads a multi-value reference field the w
309309
probes = stub.probes;
310310
engine.registerDriver(stub.driver, true);
311311
await engine.init();
312-
for (const o of [acct, zoo, guard, opp, holdExplicit, cascadeMulti, singleSetNull]) {
312+
for (const o of [acct, zoo, guard, opp, explicitSetNull, cascadeMulti, singleSetNull]) {
313313
engine.registry.registerObject(o, OWNER_PACKAGE);
314314
}
315315
});
@@ -459,22 +459,27 @@ describe('[#9362] the dependents probe reads a multi-value reference field the w
459459
});
460460

461461
/**
462-
* [#9362 -> #9438] The holding position: a `set_null` limb aimed at a
463-
* SET-valued foreign key refuses instead of writing.
462+
* [#9438] `set_null` on a SET-valued foreign key removes the deleted MEMBER
463+
* and writes what remains; the emptied set is written as `[]`, never `null`.
464464
*
465-
* Maintainer-ruled (option B) as a temporary measure to ship alongside the
466-
* probe repair above. The limb it holds back writes `null` over the whole
467-
* array, dropping every other member; the correct semantics is #9438's to
468-
* decide. Refusing decides nothing and is reversible; writing decides it by
469-
* accident and is not.
465+
* The residual-shape sentence is NOT this suite's finding: `FieldSchema` pins
466+
* it (`packages/spec/src/data/field.zod.ts`, the `multiple` and `required`
467+
* doc blocks — #9447, maintainer ruling 2026-08-18, binding for every
468+
* writer). This suite pins that the cascade repair CONSUMES that contract.
470469
*
471-
* The suite is written OVER-FIRE FIRST, because that is the way this guard can
472-
* re-break what the probe repair just fixed: every disposition it must NOT
473-
* touch — single-valued `set_null`, multi-value `cascade`, an already-declared
474-
* `restrict`, and a relation with no dependents at all — is asserted here
475-
* beside the two it must catch.
470+
* The #9437 holding position — refusing these deletes 409 while the residual
471+
* shape was undecided — is REMOVED by the same change, so the success pins
472+
* here are also the revert's pins: under the hold every one of them was a
473+
* `DELETE_RESTRICTED` refusal.
474+
*
475+
* Controls stay OVER-FIRE FIRST, as the hold's suite had them: single-valued
476+
* `set_null`, multi-value `cascade`, an already-declared `restrict`, and a
477+
* relation with no dependents are asserted beside the member-removal pins.
478+
* The single-valued control is non-vacuous in the WIDEN direction (treat
479+
* every field as multi-valued and it goes red on the array write), not under
480+
* a removal ablation.
476481
*/
477-
describe('[#9362 -> #9438] set_null on a multi-value reference refuses instead of nulling the array', () => {
482+
describe('[#9438] set_null on a multi-value reference removes the deleted member', () => {
478483
let engine: ObjectQL;
479484
let stores: Map<string, Map<string, Record<string, unknown>>>;
480485

@@ -484,60 +489,99 @@ describe('[#9362 -> #9438] set_null on a multi-value reference refuses instead o
484489
stores = stub.stores;
485490
engine.registerDriver(stub.driver, true);
486491
await engine.init();
487-
for (const o of [acct, zoo, guard, opp, holdExplicit, cascadeMulti, singleSetNull]) {
492+
for (const o of [acct, zoo, guard, opp, explicitSetNull, cascadeMulti, singleSetNull]) {
488493
engine.registry.registerObject(o, OWNER_PACKAGE);
489494
}
490495
});
491496

492-
// ── FIRES — and the array is still intact afterwards.
497+
// ── The write: member removal, on both spellings of set_null.
493498

494-
it('a DEFAULTED set_null on a multi-value field refuses, and writes nothing', async () => {
499+
it('a DEFAULTED set_null removes the deleted member and keeps the rest', async () => {
495500
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
496501
await engine.insert('mv_acct', { id: 'acc_b', name: 'B' });
497502
await engine.insert('mv_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_a', 'acc_b'] });
498503

499-
const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e);
500-
expect(err.code).toBe('DELETE_RESTRICTED');
501-
expect(err.status).toBe(409);
502-
expect(err.dependentObject).toBe('mv_zoo');
503-
expect(err.dependentCount).toBe(1);
504-
// The whole point: the sibling reference is still there.
505-
expect(stores.get('mv_zoo')?.get('z1')?.f_lookups).toEqual(['acc_a', 'acc_b']);
506-
expect(stores.get('mv_acct')?.has('acc_a')).toBe(true);
504+
await engine.delete('mv_acct', { where: { id: a.id } } as any);
505+
506+
expect(stores.get('mv_acct')?.has('acc_a')).toBe(false);
507+
// The whole point of the card: the sibling reference SURVIVES, and the
508+
// record it points at is still there to resolve.
509+
expect(stores.get('mv_zoo')?.get('z1')?.f_lookups).toEqual(['acc_b']);
510+
expect(stores.get('mv_acct')?.has('acc_b')).toBe(true);
507511
});
508512

509-
it('an EXPLICITLY authored set_null on a multi-value field refuses too', async () => {
513+
it('an EXPLICITLY authored set_null takes the same member removal', async () => {
510514
// `fdef.deleteBehavior || 'set_null'` collapses the absent declaration and
511-
// this one into the same value, which is why one `if` covers both — the
512-
// same reason the required-FK escalation beside it covers both.
515+
// this one into the same value — both spellings take the same write.
513516
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
514-
await engine.insert('mv_hold_explicit', { id: 'h1', accounts: ['acc_a'] });
517+
await engine.insert('mv_acct', { id: 'acc_b', name: 'B' });
518+
await engine.insert('mv_explicit_set_null', { id: 'h1', accounts: ['acc_a', 'acc_b'] });
515519

516-
const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e);
517-
expect(err.code).toBe('DELETE_RESTRICTED');
518-
expect(err.status).toBe(409);
519-
expect(err.dependentObject).toBe('mv_hold_explicit');
520-
expect(stores.get('mv_hold_explicit')?.get('h1')?.accounts).toEqual(['acc_a']);
520+
await engine.delete('mv_acct', { where: { id: a.id } } as any);
521+
522+
expect(stores.get('mv_explicit_set_null')?.get('h1')?.accounts).toEqual(['acc_b']);
521523
});
522524

523-
it('the refusal names the hold as TEMPORARY and points at #9438, on the developer half only', async () => {
525+
it('removing the LAST member writes `[]`, never `null` — the ruled representation', async () => {
524526
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
525527
await engine.insert('mv_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_a'] });
526528

529+
await engine.delete('mv_acct', { where: { id: a.id } } as any);
530+
531+
const stored = stores.get('mv_zoo')?.get('z1')?.f_lookups;
532+
// Assert the literal shape, both ways: the contract sentence is exactly
533+
// "`[]`, never `null`" (field.zod.ts, `multiple` doc block).
534+
expect(stored).toEqual([]);
535+
expect(stored).not.toBeNull();
536+
expect(Array.isArray(stored)).toBe(true);
537+
});
538+
539+
it('removal compares WHOLE members — an id keeps a member it is only a prefix of', async () => {
540+
// The probe pushdown is a substring superset; the write must not be.
541+
const a = await engine.insert('mv_acct', { id: 'acc_1', name: 'one' });
542+
await engine.insert('mv_acct', { id: 'acc_10', name: 'ten' });
543+
await engine.insert('mv_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_1', 'acc_10'] });
544+
545+
await engine.delete('mv_acct', { where: { id: a.id } } as any);
546+
547+
expect(stores.get('mv_zoo')?.get('z1')?.f_lookups).toEqual(['acc_10']);
548+
});
549+
550+
it('every dependent row loses the member, each keeping its own others', async () => {
551+
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
552+
await engine.insert('mv_acct', { id: 'acc_b', name: 'B' });
553+
await engine.insert('mv_zoo', { id: 'z1', name: 'z1', f_lookups: ['acc_a', 'acc_b'] });
554+
await engine.insert('mv_zoo', { id: 'z2', name: 'z2', f_lookups: ['acc_a'] });
555+
await engine.insert('mv_zoo', { id: 'z3', name: 'z3', f_lookups: ['acc_b'] });
556+
557+
await engine.delete('mv_acct', { where: { id: a.id } } as any);
558+
559+
expect(stores.get('mv_zoo')?.get('z1')?.f_lookups).toEqual(['acc_b']);
560+
expect(stores.get('mv_zoo')?.get('z2')?.f_lookups).toEqual([]);
561+
// A row that never referenced the deleted record is not touched.
562+
expect(stores.get('mv_zoo')?.get('z3')?.f_lookups).toEqual(['acc_b']);
563+
});
564+
565+
// ── The refusal channel: plain policy again, no interim sentence.
566+
567+
it("an already-declared multi-value restrict refuses with its OWN sentence — the interim's is gone", async () => {
568+
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
569+
await engine.insert('mv_guard', { id: 'g1', name: 'g', accounts: ['acc_a'] });
570+
527571
const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e);
528-
expect(err.developerMessage).toContain('TEMPORARY');
529-
expect(err.developerMessage).toContain('objectstack#9438');
530-
expect(err.developerMessage).toContain('multiple: true');
531-
// The BUSINESS sentence is the ordinary one — the user's action is the
532-
// same, and #7307 keeps the machine detail off this half.
533-
expect(err.message).not.toContain('9438');
534-
// And the wire code does not split (operation-message.ts's own rule).
535572
expect(err.code).toBe('DELETE_RESTRICTED');
573+
expect(err.status).toBe(409);
574+
expect(err.dependentObject).toBe('mv_guard');
575+
// Configured policy only: the holding-position wording must not outlive
576+
// the hold (it cited its card literally so that this grep is possible).
577+
expect(err.developerMessage).not.toContain('TEMPORARY');
578+
expect(err.developerMessage).not.toContain('9438');
579+
expect(stores.get('mv_guard')?.get('g1')?.accounts).toEqual(['acc_a']);
536580
});
537581

538-
// ── DOES NOT FIRE — four dispositions the hold must leave alone.
582+
// ── Controls the removal must leave alone.
539583

540-
it('a SINGLE-valued set_null still clears the foreign key, exactly as before', async () => {
584+
it('a SINGLE-valued set_null still clears the foreign key to null, exactly as before', async () => {
541585
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
542586
await engine.insert('mv_single', { id: 's1', account: 'acc_a' });
543587

@@ -547,7 +591,7 @@ describe('[#9362 -> #9438] set_null on a multi-value reference refuses instead o
547591
expect(stores.get('mv_single')?.get('s1')?.account).toBeNull();
548592
});
549593

550-
it('a multi-value CASCADE still deletes the dependents (the P0 closes for this path)', async () => {
594+
it('a multi-value CASCADE still deletes the dependents', async () => {
551595
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
552596
await engine.insert('mv_cascade_multi', { id: 'c1', accounts: ['acc_a'] });
553597

@@ -557,23 +601,10 @@ describe('[#9362 -> #9438] set_null on a multi-value reference refuses instead o
557601
expect(stores.get('mv_cascade_multi')?.has('c1')).toBe(false);
558602
});
559603

560-
it('an already-declared multi-value restrict refuses with its OWN sentence, not the hold\'s', async () => {
561-
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
562-
await engine.insert('mv_guard', { id: 'g1', name: 'g', accounts: ['acc_a'] });
563-
564-
const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e);
565-
expect(err.code).toBe('DELETE_RESTRICTED');
566-
expect(err.dependentObject).toBe('mv_guard');
567-
// Configured policy, not a holding position — the two must stay tellable
568-
// apart, which is the whole reason the sentence splits.
569-
expect(err.developerMessage).not.toContain('9438');
570-
expect(err.developerMessage).not.toContain('TEMPORARY');
571-
});
572-
573-
it('a multi-value set_null relation with NO dependent rows still deletes (the card\'s P0 repro)', async () => {
604+
it('a multi-value set_null relation with NO dependent rows still deletes', async () => {
574605
const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' });
575606
expect(stores.get('mv_zoo')?.size ?? 0).toBe(0);
576-
expect(stores.get('mv_hold_explicit')?.size ?? 0).toBe(0);
607+
expect(stores.get('mv_explicit_set_null')?.size ?? 0).toBe(0);
577608

578609
await engine.delete('mv_acct', { where: { id: a.id } } as any);
579610

0 commit comments

Comments
 (0)