Skip to content

Commit eccb8b2

Browse files
os-zhuangos-samclaude
authored
fix(engine): probe a multiple:true reference field with a spelling its storage answers (#9437)
* fix(engine): probe a multiple:true reference field with a spelling its storage answers (#9362) `cascadeDeleteRelations` built a bare-equality dependents filter for every `lookup` / `master_detail` field aimed at the object being deleted, including the ones declaring `multiple: true`. Such a field stores an array, which every SQL backend here puts in a JSON TEXT column, so bare equality compares the whole serialization against one id; `driver-sql` refuses that spelling with `INVALID_FILTER` / 400 (#7398). Result: any object pointed at by any registered `multiple: true` lookup could not be deleted at all — schema-driven, so an empty referring table did not help. On the stock showcase that is `showcase_account`. The driver's refusal and #8895's discriminate-or-propagate `catch` are both correct and both untouched. The fix is at the probe's construction site: a multi-value field is asked with `$contains`, the membership spelling the refusal prescribes and the one every driver here answers. `$contains` is a substring test, so the pushdown answers a superset and the rows are narrowed exactly afterwards — element-wise, the same reading the dangling-reference audit applies to a stored reference. An id needing JSON escaping is asked for in both stored spellings so the guard cannot fail open on it. No filter or predicate surface is widened, and the single-valued probe is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj * test(engine): gate the #9362 driver double on the FILTER, not on the rows `driver-sql` raises the #7398 JSON-column refusal while COMPILING the predicate, so an empty table refuses exactly as a full one does — which is what makes the card's fault schema-driven. The double evaluated it per row, so with no rows to scan it refused nothing: measured on the reverse-verification lap, the card's own reproduction (a delete refused with an EMPTY referring table) passed with the fix reverted. A double looser than the driver it stands in for turns a green suite into no suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj * test(runtime): keep the #9362 driver counts off the query-options erasure surface `SqlDriver.count`'s query argument is optional and typed, so the four `{} as any` / `{ where: … } as any` casts bought nothing and pushed check:query-options-erasure's test-surface ceiling 240 -> 244. Raising that number is a reviewed edit, not a remedy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj * fix(engine): hold back set_null on a multi-value reference instead of nulling the array Maintainer-ruled option B, shipping with the probe repair in this PR and explicitly a temporary holding position rather than a semantic. Repairing the probe is what makes the `set_null` limb run for a multi-value relationship for the first time in this codebase, and that limb writes `null` over the WHOLE array, dropping every other member. Measured on the real stack: a row holding ["acc_a","acc_b"] re-reads as null once acc_a is deleted. The right semantics is "remove just the deleted member", but the residual shape when the array empties ([] or null) is observable and unpinned; that question is tracked in objectstack#9438. Refusing decides nothing and reverts in one `if`; writing decides it by accident. Shaped as the required-FK escalation directly above it rather than as a new mechanism, and covering the explicitly authored `set_null` for the same reason that one does: `fdef.deleteBehavior || 'set_null'` collapses the absent declaration and the explicit spelling into a single value, so telling them apart would be new machinery — and would leave the explicit spelling running the very write this holds back. No new wire code: `operation-message.ts` already rules this envelope one DELETE_RESTRICTED with more than one sentence. The reason is developer-facing and rides `developerMessage`, naming the hold as temporary and citing the tracking issue literally so its removal is one grep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --------- Co-authored-by: os-steve <sam@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e374b4d commit eccb8b2

4 files changed

Lines changed: 1057 additions & 5 deletions

File tree

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(engine): `cascadeDeleteRelations` probes a `multiple: true` reference field with a spelling its storage can answer, so REST DELETE stops returning 400 for every object such a field points at (#9362)
6+
7+
Any object pointed at by any registered `multiple: true` `lookup` /
8+
`master_detail` field had its data-plane delete refused outright:
9+
10+
```
11+
POST /api/v1/data/showcase_account {"name":"anything","status":"active"} -> 201
12+
DELETE /api/v1/data/showcase_account/<id> -> 400 INVALID_FILTER
13+
```
14+
15+
On the stock showcase that is `showcase_account`, because
16+
`showcase_field_zoo.f_lookups` is `Field.lookup('showcase_account', { multiple: true })`.
17+
The fault is **schema-driven, not data-driven** — the dependents probe runs once
18+
per DECLARED relation, so emptying the referring table changes nothing.
19+
20+
**Mechanism.** The probe built a bare-equality filter for every reference field
21+
aimed at the object being deleted, including the multi-value ones. A
22+
`multiple: true` field stores an array, which every SQL backend here puts in a
23+
JSON TEXT column, so bare equality compares the whole serialization (`["a","b"]`)
24+
against one id and can never hold. `driver-sql` refuses that spelling
25+
(`INVALID_FILTER` / 400) rather than compiling a silently wrong answer.
26+
27+
**Neither of the two correct behaviours around it was touched.** The driver's
28+
refusal stays — it is right, and loosening it would restore the fail-both-ways
29+
comparison it exists to stop. The discriminate-or-propagate `catch` that
30+
surfaces a probe failure instead of inventing "no dependents" stays too: the
31+
probe's filter spelling was never correct, and that tightening only turned a
32+
silent wrong answer into a loud one.
33+
34+
**The fix is at the probe's construction site, and nowhere else.** A multi-value
35+
field is asked with `$contains` — the membership spelling the refusal itself
36+
prescribes, and the one every driver here answers (`driver-sql` and the two
37+
drivers extending it lower it to `LIKE '%v%'` over the serialization,
38+
`driver-mongodb` and `driver-memory` to a `$regex` that matches per element). No
39+
filter or predicate surface is widened: `$contains` was already declared, and the
40+
single-valued probe is byte-identical to what it was.
41+
42+
`$contains` is a SUBSTRING test, so on every one of those backends the pushdown
43+
answers a **superset** — with ids `acc_1` and `acc_10`, a row holding `acc_10`
44+
matches a probe for `acc_1`. The rows are therefore narrowed exactly afterwards,
45+
element-wise, the same reading the dangling-reference audit already applies to a
46+
stored reference. Without that half the fix would make `cascade` delete and
47+
`set_null` clear rows that never referenced the record — worse than the 400. An
48+
id needing JSON escaping is asked for in both stored spellings, so the guard
49+
cannot fail open on it either.
50+
51+
Both directions are pinned, against a driver double that reproduces the JSON
52+
column refusal and against a real `SqlDriver` on better-sqlite3 driven through
53+
the real data-plane delete: the delete succeeds and the row is gone, a live
54+
dependent through the array still refuses with `DELETE_RESTRICTED` / 409, and an
55+
id that is a prefix of another neither inherits its dependents nor loses its own.
56+
57+
## Shipping with it: a TEMPORARY refusal on `set_null` over a multi-value reference
58+
59+
Maintainer-ruled to land in the same change, and **explicitly a holding position
60+
rather than a semantic**: while a `multiple: true` reference field would take the
61+
`set_null` limb, the delete is now refused (`DELETE_RESTRICTED` / 409) instead of
62+
executed.
63+
64+
Repairing the probe is what would make that limb run for the first time in this
65+
codebase — before #8895 the probe swallowed its own failure and skipped the
66+
relation, after #8895 it raised `INVALID_FILTER` and aborted the delete — and the
67+
limb writes `null` over the WHOLE array, discarding every other member. Measured
68+
on the real stack: a row holding `["acc_a","acc_b"]` re-reads as `null` once
69+
`acc_a` is deleted.
70+
71+
The right semantics is "remove just the deleted member", but the residual shape
72+
when the array empties (`[]` or `null`) is observable on the read path and to a
73+
required multi-value validator, and nothing in `FieldSchema` pins it. That
74+
question is tracked in objectstack#9438; refusing loudly until it is answered
75+
decides nothing and reverts in one `if`, while writing would decide it by
76+
accident and cannot be undone for the rows it touched.
77+
78+
**Scope of the refusal, and what it deliberately leaves alone.** It is the
79+
required-FK escalation directly above it, applied to an adjacent case: the same
80+
`behavior` reassignment, reading the same `behavior === 'set_null'`. Because
81+
`fdef.deleteBehavior || 'set_null'` collapses an absent declaration and an
82+
explicitly authored `set_null` into one value, both are covered — the same way
83+
both are already covered by the required-FK escalation, and without adding a
84+
distinction the existing shape does not make. An explicit `cascade` or `restrict`
85+
is untouched, a single-valued `set_null` still clears its foreign key, and a
86+
relation with no dependent rows still deletes: only the disposition changes, so
87+
the P0 above genuinely closes for every other path.
88+
89+
**No new wire code**, per the rule `operation-message.ts` already states for this
90+
envelope — one `DELETE_RESTRICTED` with more than one sentence, splitting the
91+
sentence and never the code. The reason is developer-facing, so it rides
92+
`developerMessage`, which names the refusal as temporary and cites the tracking
93+
issue literally so removing this is one grep. The business message a user reads is
94+
unchanged, because their action is unchanged: clear or reassign the referencing
95+
records.

0 commit comments

Comments
 (0)