From fd4703e0f1fb6439b844548f34acd2cb539da5ee Mon Sep 17 00:00:00 2001 From: os-steve Date: Tue, 18 Aug 2026 00:28:13 +0000 Subject: [PATCH 1/4] fix(engine): probe a multiple:true reference field with a spelling its storage answers (#9362) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../cascade-probe-multivalue-lookup-filter.md | 55 +++ ...ne-cascade-delete-multivalue-probe.test.ts | 384 ++++++++++++++++++ packages/objectql/src/engine.ts | 112 ++++- ...lue-lookup-real-driver.integration.test.ts | 138 +++++++ 4 files changed, 688 insertions(+), 1 deletion(-) create mode 100644 .changeset/cascade-probe-multivalue-lookup-filter.md create mode 100644 packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts create mode 100644 packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts diff --git a/.changeset/cascade-probe-multivalue-lookup-filter.md b/.changeset/cascade-probe-multivalue-lookup-filter.md new file mode 100644 index 0000000000..43709cb3a2 --- /dev/null +++ b/.changeset/cascade-probe-multivalue-lookup-filter.md @@ -0,0 +1,55 @@ +--- +"@objectstack/objectql": patch +--- + +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) + +Any object pointed at by any registered `multiple: true` `lookup` / +`master_detail` field had its data-plane delete refused outright: + +``` +POST /api/v1/data/showcase_account {"name":"anything","status":"active"} -> 201 +DELETE /api/v1/data/showcase_account/ -> 400 INVALID_FILTER +``` + +On the stock showcase that is `showcase_account`, because +`showcase_field_zoo.f_lookups` is `Field.lookup('showcase_account', { multiple: true })`. +The fault is **schema-driven, not data-driven** — the dependents probe runs once +per DECLARED relation, so emptying the referring table changes nothing. + +**Mechanism.** The probe built a bare-equality filter for every reference field +aimed at the object being deleted, including the multi-value ones. A +`multiple: true` field stores an array, which every SQL backend here puts in a +JSON TEXT column, so bare equality compares the whole serialization (`["a","b"]`) +against one id and can never hold. `driver-sql` refuses that spelling +(`INVALID_FILTER` / 400) rather than compiling a silently wrong answer. + +**Neither of the two correct behaviours around it was touched.** The driver's +refusal stays — it is right, and loosening it would restore the fail-both-ways +comparison it exists to stop. The discriminate-or-propagate `catch` that +surfaces a probe failure instead of inventing "no dependents" stays too: the +probe's filter spelling was never correct, and that tightening only turned a +silent wrong answer into a loud one. + +**The fix is at the probe's construction site, and nowhere else.** A multi-value +field is asked with `$contains` — the membership spelling the refusal itself +prescribes, and the one every driver here answers (`driver-sql` and the two +drivers extending it lower it to `LIKE '%v%'` over the serialization, +`driver-mongodb` and `driver-memory` to a `$regex` that matches per element). No +filter or predicate surface is widened: `$contains` was already declared, and the +single-valued probe is byte-identical to what it was. + +`$contains` is a SUBSTRING test, so on every one of those backends the pushdown +answers a **superset** — with ids `acc_1` and `acc_10`, a row holding `acc_10` +matches a probe for `acc_1`. The rows are therefore narrowed exactly afterwards, +element-wise, the same reading the dangling-reference audit already applies to a +stored reference. Without that half the fix would make `cascade` delete and +`set_null` clear rows that never referenced the record — worse than the 400. An +id needing JSON escaping is asked for in both stored spellings, so the guard +cannot fail open on it either. + +Both directions are pinned, against a driver double that reproduces the JSON +column refusal and against a real `SqlDriver` on better-sqlite3 driven through +the real data-plane delete: the delete succeeds and the row is gone, a live +dependent through the array still refuses with `DELETE_RESTRICTED` / 409, and an +id that is a prefix of another neither inherits its dependents nor loses its own. diff --git a/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts new file mode 100644 index 0000000000..528784f0dd --- /dev/null +++ b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts @@ -0,0 +1,384 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#9362] `cascadeDeleteRelations` must probe a `multiple: true` reference + * field with the spelling that field's storage can answer. + * + * ## The reported failure + * + * ``` + * POST /api/v1/data/showcase_account {"name":"anything","status":"active"} -> 201 + * DELETE /api/v1/data/showcase_account/ -> 400 INVALID_FILTER + * ``` + * + * The probe built a BARE EQUALITY filter for every `lookup` / `master_detail` + * field pointing 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 + * (`["a","b"]`) against one id. `driver-sql` refuses that spelling outright + * (`INVALID_FILTER` / 400, #7398) — correctly, and that refusal is untouched by + * this fix — and #8895's discriminate-or-propagate `catch` (also correct, also + * untouched) let it through to the caller. Result: any object pointed at by any + * registered `multiple: true` lookup could not be deleted at all. It is + * SCHEMA-driven: the probe runs per DECLARED relation, so emptying the + * dependent table changes nothing, which is the first thing the suite below + * pins. + * + * ## The double + * + * `makeJsonColumnDriver` is a driver double that models the ONE behaviour this + * card turns on: a field declared `multiple: true` is stored and queried as a + * JSON TEXT column, so bare equality (and `$in`) against it raises the same + * `INVALID_FILTER` / 400 refusal `driver-sql` raises, while `$contains` matches + * as a SUBSTRING of the serialization — which is exactly what `LIKE '%v%'` + * does there. It is deliberately not looser than the real driver on either + * half: a double that answered bare equality would make every assertion below + * pass without the fix. + * + * ## Both directions, always + * + * A probe that finds NOTHING would also turn the 400 into a 200 — and would + * silently delete referenced records, which is worse than the bug. So every + * happy-path assertion here is paired with a guard assertion on the same + * relationship, and the `restrict` refusal is asserted through its full + * ADR-0112 envelope (`code` AND `status`), never a bare `toThrow()`. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; + +const OWNER_PACKAGE = 'test-9362'; + +const acct: ServiceObject = { + name: 'mv_acct', + label: 'Account', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +/** The showcase shape: `Field.lookup('showcase_account', { multiple: true })`. */ +const zoo: ServiceObject = { + name: 'mv_zoo', + label: 'Field Zoo', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + // deleteBehavior defaults to `set_null` + f_lookups: { + name: 'f_lookups', label: 'Lookups', type: 'lookup' as const, + reference: 'mv_acct', multiple: true, + }, + }, +}; + +/** The same multi-value relationship, declared `restrict`: the guard direction. */ +const guard: ServiceObject = { + name: 'mv_guard', + label: 'Guard', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + accounts: { + name: 'accounts', label: 'Accounts', type: 'lookup' as const, + reference: 'mv_acct', multiple: true, deleteBehavior: 'restrict' as const, + }, + }, +}; + +/** A single-valued lookup on the same target — the unchanged-behaviour control. */ +const opp: ServiceObject = { + name: 'mv_opp', + label: 'Opportunity', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + account: { + name: 'account', label: 'Account', type: 'lookup' as const, + reference: 'mv_acct', deleteBehavior: 'restrict' as const, + }, + }, +}; + +/** Which fields this double stores as a JSON TEXT column, per object. */ +const JSON_COLUMNS: Record = { + mv_zoo: ['f_lookups'], + mv_guard: ['accounts'], +}; + +/** + * The #7398 refusal, reproduced in the double with the envelope the real driver + * gives it — `INVALID_FILTER` / 400, the ADR-0112 class-1 shape. + */ +function jsonColumnRefusal(field: string, op: string): Error { + const err: any = new Error( + `A constraint in this filter WAS NOT APPLIED: "${field}" is stored as a JSON TEXT column and ` + + `"${op}" compares that whole serialized text against a single value. Use "$contains" for membership.`, + ); + err.code = 'INVALID_FILTER'; + err.status = 400; + return err; +} + +function makeJsonColumnDriver() { + const stores = new Map>>(); + const probes: Array<{ object: string; where: unknown }> = []; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + + const isJson = (object: string, field: string) => JSON_COLUMNS[object]?.includes(field) === true; + + /** One `{ field: }` entry, evaluated the way the real backends do. */ + const matchesField = ( + object: string, row: Record, field: string, spec: unknown, + ): boolean => { + const stored = row[field]; + if (spec !== null && typeof spec === 'object' && !Array.isArray(spec)) { + const [op, comparand] = Object.entries(spec as Record)[0] ?? []; + if (op === '$contains') { + // `LIKE '%v%'` over the serialization — a SUBSTRING test, exactly as + // driver-sql lowers it on a JSON column. Substring, not membership: + // that is why the engine narrows the rows afterwards. + if (isJson(object, field)) { + return JSON.stringify(stored ?? null).includes(String(comparand)); + } + return typeof stored === 'string' && stored.includes(String(comparand)); + } + if (op === '$eq' || op === '$in') { + if (isJson(object, field)) throw jsonColumnRefusal(field, op!); + const wanted = op === '$in' ? (comparand as unknown[]) : [comparand]; + return wanted.some((w) => (stored ?? null) === (w ?? null)); + } + throw jsonColumnRefusal(field, String(op)); + } + // Bare equality — the spelling the probe used to build for every field. + if (isJson(object, field)) throw jsonColumnRefusal(field, 'bare equality'); + return (stored ?? null) === (spec ?? null); + }; + + const matches = (object: string, 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(object, row, sub))) return false; + continue; + } + if (k === '$and') { + if (!(v as any[]).every((sub) => matches(object, row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + if (!matchesField(object, row, k, v)) return false; + } + return true; + }; + + const driver: any = { + name: 'json-column', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(o: string, ast: any) { + probes.push({ object: o, where: ast?.where }); + return Array.from(storeFor(o).values()).filter((r) => matches(o, r, ast?.where)); + }, + async findOne(o: string, ast: any) { + for (const r of storeFor(o).values()) if (matches(o, r, ast?.where)) return r; + return null; + }, + async create(o: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(o).set(id, row); + return row; + }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); const cur = s.get(id); + if (!cur) throw new Error(`nf ${o}/${id}`); + const up = { ...cur, ...data, id }; s.set(id, up); return up; + }, + async upsert(o: string, data: Record) { + const id = data.id as string | undefined; + return id && storeFor(o).has(id) ? this.update(o, id, data) : this.create(o, data); + }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { + return Array.from(storeFor(o).values()).filter((r) => matches(o, r, ast?.where)).length; + }, + async bulkCreate(o: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(o, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores, probes }; +} + +const rows = (stores: Map>>, o: string) => + stores.get(o)?.size ?? 0; + +describe('[#9362] the dependents probe reads a multi-value reference field the way it is stored', () => { + let engine: ObjectQL; + let stores: Map>>; + let probes: Array<{ object: string; where: unknown }>; + + beforeEach(async () => { + engine = new ObjectQL(); + const stub = makeJsonColumnDriver(); + stores = stub.stores; + probes = stub.probes; + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of [acct, zoo, guard, opp]) engine.registry.registerObject(o, OWNER_PACKAGE); + }); + + // ── The card's own reproduction. + + it('deletes a record whose only multi-value referrer table is EMPTY (the schema-driven 400)', async () => { + const a = await engine.insert('mv_acct', { name: 'anything' }); + expect(rows(stores, 'mv_zoo')).toBe(0); + expect(rows(stores, 'mv_guard')).toBe(0); + + await engine.delete('mv_acct', { where: { id: a.id } } as any); + + expect(rows(stores, 'mv_acct')).toBe(0); + // The relation WAS probed — a fix that stopped probing multi-value + // relations would also pass the line above, and would be the worse bug. + expect(probes.some((p) => p.object === 'mv_zoo')).toBe(true); + expect(probes.some((p) => p.object === 'mv_guard')).toBe(true); + }); + + it('the multi-value probe is spelled `$contains`, the single-valued one stays bare equality', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_x', name: 'anything' }); + await engine.delete('mv_acct', { where: { id: a.id } } as any); + + expect(probes.find((p) => p.object === 'mv_zoo')?.where) + .toEqual({ f_lookups: { $contains: 'acc_x' } }); + expect(probes.find((p) => p.object === 'mv_guard')?.where) + .toEqual({ accounts: { $contains: 'acc_x' } }); + // Unchanged: a scalar foreign key is still asked the scalar question. + expect(probes.find((p) => p.object === 'mv_opp')?.where).toEqual({ account: 'acc_x' }); + }); + + // ── The other direction: the guard must still refuse. + + it('a live dependent through the multi-value field still REFUSES the delete', async () => { + const a = await engine.insert('mv_acct', { name: 'referenced' }); + await engine.insert('mv_guard', { name: 'g', accounts: [a.id] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentObject).toBe('mv_guard'); + expect(err.dependentCount).toBe(1); + expect(rows(stores, 'mv_acct')).toBe(1); + }); + + it('the refusal counts the referencing rows, and only those, when the array holds several ids', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_1', name: 'one' }); + const b = await engine.insert('mv_acct', { id: 'acc_2', name: 'two' }); + await engine.insert('mv_guard', { id: 'g1', name: 'both', accounts: [a.id, b.id] }); + await engine.insert('mv_guard', { id: 'g2', name: 'other', accounts: [b.id] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentCount).toBe(1); + }); + + // ── The narrowing: `$contains` is a SUBSTRING test, so the pushdown + // over-matches and the exact answer has to be taken on the rows. + + it('an id that is a PREFIX of another does not inherit the other id\'s dependents', async () => { + const a1 = await engine.insert('mv_acct', { id: 'acc_1', name: 'one' }); + await engine.insert('mv_acct', { id: 'acc_10', name: 'ten' }); + // Only `acc_10` is referenced. `$contains: 'acc_1'` matches this row's + // serialization anyway — the narrowing is what stops a spurious 409. + await engine.insert('mv_guard', { id: 'g1', name: 'g', accounts: ['acc_10'] }); + + await engine.delete('mv_acct', { where: { id: a1.id } } as any); + + expect(stores.get('mv_acct')?.has('acc_1')).toBe(false); + expect(stores.get('mv_acct')?.has('acc_10')).toBe(true); + // …and the guard on the id that IS referenced still fires. + const err: any = await engine.delete('mv_acct', { where: { id: 'acc_10' } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + }); + + it('a cascade through a multi-value field removes the rows that reference, and no others', async () => { + const cascadeZoo: ServiceObject = { + name: 'mv_cascade', + label: 'Cascade', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + accounts: { + name: 'accounts', label: 'Accounts', type: 'lookup' as const, + reference: 'mv_acct', multiple: true, deleteBehavior: 'cascade' as const, + }, + }, + }; + engine.registry.registerObject(cascadeZoo, OWNER_PACKAGE); + (JSON_COLUMNS as Record).mv_cascade = ['accounts']; + try { + await engine.insert('mv_acct', { id: 'acc_1', name: 'one' }); + await engine.insert('mv_acct', { id: 'acc_10', name: 'ten' }); + await engine.insert('mv_cascade', { id: 'c1', accounts: ['acc_1'] }); + await engine.insert('mv_cascade', { id: 'c2', accounts: ['acc_10'] }); + + await engine.delete('mv_acct', { where: { id: 'acc_1' } } as any); + + // `c2` references `acc_10`, which the substring pushdown also matched. + // Deleting it would be data loss caused by the fix itself. + expect(stores.get('mv_cascade')?.has('c1')).toBe(false); + expect(stores.get('mv_cascade')?.has('c2')).toBe(true); + } finally { + delete (JSON_COLUMNS as Record).mv_cascade; + } + }); + + // ── An id that needs JSON escaping is asked for in BOTH stored spellings, so + // the guard cannot fail OPEN on it. + + it('an id containing a quote is still found through the escaped serialization', async () => { + const weird = 'acc"1'; + await engine.insert('mv_acct', { id: weird, name: 'quoted' }); + await engine.insert('mv_guard', { id: 'g1', name: 'g', accounts: [weird] }); + + const err: any = await engine.delete('mv_acct', { where: { id: weird } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentCount).toBe(1); + expect(probes.find((p) => p.object === 'mv_guard')?.where).toEqual({ + $or: [ + { accounts: { $contains: 'acc"1' } }, + { accounts: { $contains: 'acc\\"1' } }, + ], + }); + }); + + // ── #8895 stays exactly as it landed: a probe that could not RUN propagates. + + it('a probe failure that is not the JSON-column refusal still propagates (#8895 unchanged)', async () => { + const a = await engine.insert('mv_acct', { name: 'anything' }); + const injected = Object.assign(new Error('connection terminated unexpectedly'), { + code: 'ECONNRESET', + }); + const realFind = (engine as any).drivers.get('json-column').find; + (engine as any).drivers.get('json-column').find = async (o: string, ast: any) => { + if (o === 'mv_zoo') throw injected; + return realFind.call((engine as any).drivers.get('json-column'), o, ast); + }; + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toBe(injected); + expect(rows(stores, 'mv_acct')).toBe(1); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index b7e679e2c4..a9be474a3b 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -10107,6 +10107,97 @@ export class ObjectQL implements IObjectQLEngine { ); } + /** + * [#9362] The dependents probe's `where`, spelled for the KIND of reference + * field it is aimed at. + * + * A single-valued `lookup` / `master_detail` stores a scalar foreign key, and + * bare equality is the right question about it. A field declaring + * `multiple: true` stores an ARRAY — "Stores as Array/JSON" + * (`FieldSchema.multiple`) — and every SQL backend in this repo puts that + * array in a JSON TEXT column. Aiming bare equality at THAT column compares + * the whole serialization (`["a","b"]`) against one id, which can never hold; + * `driver-sql` refuses the spelling outright (`INVALID_FILTER` / 400, #7398), + * and that refusal is correct and stays. Until this method existed the probe + * built bare equality for both kinds, so every object pointed at by any + * registered `multiple: true` lookup had its delete refused with a 400 — on + * the stock showcase, `showcase_account`, with an EMPTY dependent table: + * the fault is schema-driven, not data-driven, because the probe runs per + * DECLARED relation. + * + * The multi-value spelling is `$contains`, which is what the refusal itself + * prescribes and the one membership spelling every driver here answers: + * `driver-sql` (and `driver-sqlite-wasm` / `driver-turso`, which extend it) + * lowers it to `LIKE '%v%'` over the serialization, `driver-mongodb` to + * `$regex` over the array, and `driver-memory` to a mingo `$regex`, which + * matches per ELEMENT. No public filter surface is widened: `$contains` is + * already declared, and this is the only construction site that changes. + * + * `$contains` is a SUBSTRING test, so on every one of those backends it + * answers a SUPERSET: with ids `acc_1` and `acc_10`, a row holding `acc_10` + * also matches a probe for `acc_1`. That is why the caller narrows the rows + * exactly through {@link ObjectQL.storedReferenceIncludes} — over-matching + * here would make `cascade` DELETE and `set_null` clear rows that never + * referenced this record, which is worse than the 400 being fixed. The + * pushdown's only job is to keep the probe from reading the whole table. + * + * The `$or` limb covers the other direction — a FALSE NEGATIVE, which on an + * integrity guard is the fail-OPEN that #8895 ruled out. An id needing JSON + * escaping (a quote, a backslash) appears in a SQL backend's serialized text + * in its ESCAPED form, so a probe for the raw id would miss the row that + * holds it; a document/in-memory backend compares the element itself and + * needs the RAW form. Both are asked whenever they differ, and the exact + * narrowing discards whatever the extra limb over-matched. Identical for an + * ordinary id, which is every id this engine generates. + */ + private referenceProbeFilter( + fieldName: string, + fdef: { multiple?: unknown }, + id: string | number, + ): Record { + if (fdef?.multiple !== true) return { [fieldName]: id }; + const raw = String(id); + // The BODY of the JSON string form — `JSON.stringify('a"b')` is `"a\"b"`, + // and the quotes are the serialization's, not the id's. + const escaped = JSON.stringify(raw).slice(1, -1); + if (escaped === raw) return { [fieldName]: { $contains: raw } }; + return { + $or: [ + { [fieldName]: { $contains: raw } }, + { [fieldName]: { $contains: escaped } }, + ], + }; + } + + /** + * [#9362] Does this STORED reference value point at `id`? The exact answer + * the `$contains` pushdown in {@link ObjectQL.referenceProbeFilter} can only + * approximate. + * + * Element-wise over the array, comparing `String(v)` — the same reading + * `dangling-reference-audit.ts` already applies to a stored reference + * (`Array.isArray(raw) ? raw : [raw]`), so the two integrity surfaces cannot + * disagree about what "this row references that record" means. An EXPANDED + * record in the slot is a read shape rather than an id write, and is skipped + * there for that reason; it is skipped here too. + * + * A non-array value is still compared rather than dismissed. A + * `multiple: true` slot holding a bare scalar is off-shape, and the two + * dispositions available are not symmetric: reading it as "references + * nothing" would drop a `restrict` refusal and let a referenced record be + * deleted — the fail-OPEN direction #8895 ruled out for this same guard — + * while reading it as a reference at most refuses a delete loudly. + */ + private static storedReferenceIncludes(stored: unknown, id: string | number): boolean { + const wanted = String(id); + const values = Array.isArray(stored) ? stored : [stored]; + for (const v of values) { + if (v == null || typeof v === 'object') continue; + if (String(v) === wanted) return true; + } + return false; + } + /** * Apply referential delete behavior for relations pointing AT this record, * before it is removed. For every registered object with a `master_detail` @@ -10184,9 +10275,17 @@ export class ObjectQL implements IObjectQLEngine { behavior = 'restrict'; } + // [#9362] The probe's filter is spelled for the KIND of reference field + // it is aimed at, and a multi-value one is narrowed EXACTLY afterwards. + // See `referenceProbeFilter` / `storedReferenceIncludes` for why both + // halves are needed. + const multiValued = fdef.multiple === true; let dependents: any[]; try { - dependents = await this.find(childName, { where: { [fieldName]: id }, context } as any); + dependents = await this.find( + childName, + { where: this.referenceProbeFilter(fieldName, fdef, id), context } as any, + ); } catch (error) { // [#8895] Discriminate by error TYPE — this probe IS the referential // guard, so `continue` is only truthful for the one failure that @@ -10224,6 +10323,17 @@ export class ObjectQL implements IObjectQLEngine { if (isMissingTableError(error)) continue; throw error; } + // [#9362] The multi-value pushdown above is a SUPERSET, so the exact + // answer is taken here, on the rows themselves. Everything below — + // the `restrict` count in the 409 envelope, the `cascade` recursion, + // the `set_null` write — reads `dependents`, so narrowing anywhere + // later would leave one of them acting on a row that never referenced + // this record. + if (multiValued && dependents) { + dependents = dependents.filter((row) => + ObjectQL.storedReferenceIncludes(row?.[fieldName], id), + ); + } if (!dependents || dependents.length === 0) continue; if (behavior === 'restrict') { diff --git a/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts new file mode 100644 index 0000000000..3dc2597fb0 --- /dev/null +++ b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts @@ -0,0 +1,138 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#9362] The card's reproduction, on the REAL stack — a real `ObjectQL` over a + * real `SqlDriver` on better-sqlite3, driven through the real + * `ObjectStackProtocolImplementation`'s data-plane delete (the method + * `DELETE /api/v1/data/:object/:id` serves). + * + * ``` + * POST /api/v1/data/showcase_account {"name":"anything","status":"active"} -> 201 + * DELETE /api/v1/data/showcase_account/ -> 400 INVALID_FILTER + * ``` + * + * Nothing below builds that refusal: `f_lookups` is declared exactly as the + * showcase declares it — `Field.lookup('showcase_account', { multiple: true })` + * — the real driver really does store it in a JSON TEXT column, and the real + * `#7398` gate really does refuse the bare-equality probe + * `cascadeDeleteRelations` used to build for it. The sibling unit suite + * (`packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts`) pins + * the probe's SPELLING against a double; this file is what proves the spelling + * the fix chose is one this driver actually answers, and that the row is really + * gone from the database afterwards. + * + * The dependent table is EMPTY in the first case, deliberately: the fault is + * schema-driven, so a fixture with rows in it would prove less, not more. + * + * Both directions, as always on a referential guard: a fix that made the probe + * find nothing would turn the 400 into a 200 and silently delete referenced + * records. The `restrict` case asserts the full ADR-0112 envelope (`code` AND + * `status`) and re-reads the row. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SqlDriver } from '@objectstack/driver-sql'; + +const ACCOUNT = { name: 'zz_account', fields: { name: { type: 'text' } } }; + +/** The showcase's own shape: a multi-value lookup at the object being deleted. */ +const FIELD_ZOO = { + name: 'zz_field_zoo', + fields: { + name: { type: 'text' }, + f_lookups: { type: 'lookup', reference: 'zz_account', multiple: true }, + }, +}; + +/** The same relationship declared `restrict` — the guard direction. */ +const GUARD = { + name: 'zz_guard', + fields: { + name: { type: 'text' }, + accounts: { + type: 'lookup', reference: 'zz_account', + multiple: true, deleteBehavior: 'restrict', + }, + }, +}; + +const OWNER_PACKAGE = 'com.objectstack.test.9362'; + +describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup — real driver', () => { + let dir: string | null = null; + let engine: ObjectQL | null = null; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + engine = null; + if (dir) { rmSync(dir, { recursive: true, force: true }); dir = null; } + }); + + async function rig(objects: unknown[]) { + dir = mkdtempSync(join(tmpdir(), 'os-9362-real-')); + const real = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await real.initObjects(objects as any); + engine = new ObjectQL(); + engine.registerDriver(real as any, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o as any, OWNER_PACKAGE); + const protocol: any = new ObjectStackProtocolImplementation(engine as any); + return { protocol, real }; + } + + it('deletes the record and really removes the row (the card\'s 3/3 reproduction)', async () => { + const { protocol, real } = await rig([ACCOUNT, FIELD_ZOO]); + const created: any = await engine!.insert('zz_account', { name: 'anything' }); + expect(typeof created.id).toBe('string'); + // Schema-driven: the referring table has no rows at all. + expect(await real.count('zz_field_zoo', {} as any)).toBe(0); + + const res = await protocol.deleteData({ object: 'zz_account', id: created.id }); + expect(res).toMatchObject({ object: 'zz_account', id: created.id, success: true }); + + // Read the row count out of the DRIVER, not through the engine. + expect(await real.count('zz_account', {} as any)).toBe(0); + }); + + it('still refuses the delete when a row really does reference it through the array', async () => { + const { protocol, real } = await rig([ACCOUNT, GUARD]); + const a: any = await engine!.insert('zz_account', { name: 'referenced' }); + await engine!.insert('zz_guard', { name: 'g', accounts: [a.id] }); + + const err: any = await protocol + .deleteData({ object: 'zz_account', id: a.id }) + .catch((e: any) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentObject).toBe('zz_guard'); + expect(err.dependentCount).toBe(1); + expect(await real.count('zz_account', {} as any)).toBe(1); + }); + + it('a referenced id does not lend its dependents to an id it is a prefix of', async () => { + const { protocol, real } = await rig([ACCOUNT, GUARD]); + await engine!.insert('zz_account', { id: 'acc_1', name: 'one' }); + await engine!.insert('zz_account', { id: 'acc_10', name: 'ten' }); + // `LIKE '%acc_1%'` matches this row's serialization too. + await engine!.insert('zz_guard', { name: 'g', accounts: ['acc_10'] }); + + const res = await protocol.deleteData({ object: 'zz_account', id: 'acc_1' }); + expect(res).toMatchObject({ id: 'acc_1', success: true }); + expect(await real.count('zz_account', { where: { id: 'acc_10' } } as any)).toBe(1); + + const err: any = await protocol + .deleteData({ object: 'zz_account', id: 'acc_10' }) + .catch((e: any) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + }); +}); From fc538c583161b97cd263a57ea3b8498ba537655e Mon Sep 17 00:00:00 2001 From: os-steve Date: Tue, 18 Aug 2026 00:42:46 +0000 Subject: [PATCH 2/4] test(engine): gate the #9362 driver double on the FILTER, not on the rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- ...ne-cascade-delete-multivalue-probe.test.ts | 40 +++++++++++++++++-- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts index 528784f0dd..0bf849f2ec 100644 --- a/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts +++ b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts @@ -134,6 +134,38 @@ function makeJsonColumnDriver() { const isJson = (object: string, field: string) => JSON_COLUMNS[object]?.includes(field) === true; + /** + * The #7398 gate, where the real driver has it: on the FILTER, before a + * single row is read. `driver-sql` raises this while COMPILING the predicate + * (`assertOperatorAppliesToColumn`, reached from `applyFilters`), so an empty + * table refuses exactly as a full one does — which is what makes the card's + * fault schema-driven rather than data-driven. + * + * Evaluating it per row instead would make this double LOOSER than the driver + * it stands in for, and the card's own reproduction — a delete refused with + * an EMPTY referring table — would pass without the fix. Measured: it did, + * on the first draft of this file. + */ + const assertCompilable = (object: string, where: any): void => { + if (!where || typeof where !== 'object') return; + for (const [k, v] of Object.entries(where)) { + if (k === '$or' || k === '$and') { + for (const sub of v as any[]) assertCompilable(object, sub); + continue; + } + if (k.startsWith('$')) continue; + if (!isJson(object, k)) continue; + if (v !== null && typeof v === 'object' && !Array.isArray(v)) { + const op = Object.keys(v as Record)[0] ?? ''; + // `$contains` is the one membership spelling a JSON column answers; + // every scalar comparison is refused. + if (op !== '$contains') throw jsonColumnRefusal(k, op); + continue; + } + throw jsonColumnRefusal(k, 'bare equality'); + } + }; + /** One `{ field: }` entry, evaluated the way the real backends do. */ const matchesField = ( object: string, row: Record, field: string, spec: unknown, @@ -151,14 +183,11 @@ function makeJsonColumnDriver() { return typeof stored === 'string' && stored.includes(String(comparand)); } if (op === '$eq' || op === '$in') { - if (isJson(object, field)) throw jsonColumnRefusal(field, op!); const wanted = op === '$in' ? (comparand as unknown[]) : [comparand]; return wanted.some((w) => (stored ?? null) === (w ?? null)); } - throw jsonColumnRefusal(field, String(op)); + return false; } - // Bare equality — the spelling the probe used to build for every field. - if (isJson(object, field)) throw jsonColumnRefusal(field, 'bare equality'); return (stored ?? null) === (spec ?? null); }; @@ -185,9 +214,11 @@ function makeJsonColumnDriver() { async execute() { return null; }, async find(o: string, ast: any) { probes.push({ object: o, where: ast?.where }); + assertCompilable(o, ast?.where); return Array.from(storeFor(o).values()).filter((r) => matches(o, r, ast?.where)); }, async findOne(o: string, ast: any) { + assertCompilable(o, ast?.where); for (const r of storeFor(o).values()) if (matches(o, r, ast?.where)) return r; return null; }, @@ -209,6 +240,7 @@ function makeJsonColumnDriver() { }, async delete(o: string, id: string) { return storeFor(o).delete(id); }, async count(o: string, ast: any) { + assertCompilable(o, ast?.where); return Array.from(storeFor(o).values()).filter((r) => matches(o, r, ast?.where)).length; }, async bulkCreate(o: string, rows: Record[]) { From c29939be52dda36c7abafaed74fbfd25c8ed784e Mon Sep 17 00:00:00 2001 From: os-steve Date: Tue, 18 Aug 2026 01:05:49 +0000 Subject: [PATCH 3/4] test(runtime): keep the #9362 driver counts off the query-options erasure surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- ...lete-multivalue-lookup-real-driver.integration.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts index 3dc2597fb0..6c93516fdd 100644 --- a/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts +++ b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts @@ -94,13 +94,13 @@ describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup const created: any = await engine!.insert('zz_account', { name: 'anything' }); expect(typeof created.id).toBe('string'); // Schema-driven: the referring table has no rows at all. - expect(await real.count('zz_field_zoo', {} as any)).toBe(0); + expect(await real.count('zz_field_zoo')).toBe(0); const res = await protocol.deleteData({ object: 'zz_account', id: created.id }); expect(res).toMatchObject({ object: 'zz_account', id: created.id, success: true }); // Read the row count out of the DRIVER, not through the engine. - expect(await real.count('zz_account', {} as any)).toBe(0); + expect(await real.count('zz_account')).toBe(0); }); it('still refuses the delete when a row really does reference it through the array', async () => { @@ -115,7 +115,7 @@ describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup expect(err.status).toBe(409); expect(err.dependentObject).toBe('zz_guard'); expect(err.dependentCount).toBe(1); - expect(await real.count('zz_account', {} as any)).toBe(1); + expect(await real.count('zz_account')).toBe(1); }); it('a referenced id does not lend its dependents to an id it is a prefix of', async () => { @@ -127,7 +127,7 @@ describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup const res = await protocol.deleteData({ object: 'zz_account', id: 'acc_1' }); expect(res).toMatchObject({ id: 'acc_1', success: true }); - expect(await real.count('zz_account', { where: { id: 'acc_10' } } as any)).toBe(1); + expect(await real.count('zz_account', { where: { id: 'acc_10' } })).toBe(1); const err: any = await protocol .deleteData({ object: 'zz_account', id: 'acc_10' }) From eee5f89b2b69dc84ddafaa9d579862b9a775f5d3 Mon Sep 17 00:00:00 2001 From: os-steve Date: Tue, 18 Aug 2026 02:53:59 +0000 Subject: [PATCH 4/4] fix(engine): hold back set_null on a multi-value reference instead of nulling the array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../cascade-probe-multivalue-lookup-filter.md | 40 +++++ ...ne-cascade-delete-multivalue-probe.test.ts | 168 +++++++++++++++++- packages/objectql/src/engine.ts | 78 +++++++- ...lue-lookup-real-driver.integration.test.ts | 65 +++++++ 4 files changed, 342 insertions(+), 9 deletions(-) diff --git a/.changeset/cascade-probe-multivalue-lookup-filter.md b/.changeset/cascade-probe-multivalue-lookup-filter.md index 43709cb3a2..95ea494ee0 100644 --- a/.changeset/cascade-probe-multivalue-lookup-filter.md +++ b/.changeset/cascade-probe-multivalue-lookup-filter.md @@ -53,3 +53,43 @@ column refusal and against a real `SqlDriver` on better-sqlite3 driven through the real data-plane delete: the delete succeeds and the row is gone, a live dependent through the array still refuses with `DELETE_RESTRICTED` / 409, and an id that is a prefix of another neither inherits its dependents nor loses its own. + +## Shipping with it: a TEMPORARY refusal on `set_null` over a multi-value reference + +Maintainer-ruled to land in the same change, and **explicitly a holding position +rather than a semantic**: while a `multiple: true` reference field would take the +`set_null` limb, the delete is now refused (`DELETE_RESTRICTED` / 409) instead of +executed. + +Repairing the probe is what would make that limb run for the first time in this +codebase — before #8895 the probe swallowed its own failure and skipped the +relation, after #8895 it raised `INVALID_FILTER` and aborted the delete — and the +limb writes `null` over the WHOLE array, discarding 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 on the read path and to a +required multi-value validator, and nothing in `FieldSchema` pins it. That +question is tracked in objectstack#9438; refusing loudly until it is answered +decides nothing and reverts in one `if`, while writing would decide it by +accident and cannot be undone for the rows it touched. + +**Scope of the refusal, and what it deliberately leaves alone.** It is the +required-FK escalation directly above it, applied to an adjacent case: the same +`behavior` reassignment, reading the same `behavior === 'set_null'`. Because +`fdef.deleteBehavior || 'set_null'` collapses an absent declaration and an +explicitly authored `set_null` into one value, both are covered — the same way +both are already covered by the required-FK escalation, and without adding a +distinction the existing shape does not make. An explicit `cascade` or `restrict` +is untouched, a single-valued `set_null` still clears its foreign key, and a +relation with no dependent rows still deletes: only the disposition changes, so +the P0 above genuinely closes for every other path. + +**No new wire code**, per the rule `operation-message.ts` already states for this +envelope — one `DELETE_RESTRICTED` with more than one sentence, splitting the +sentence and never the code. The reason is developer-facing, so it rides +`developerMessage`, which names the refusal as temporary and cites the tracking +issue literally so removing this is one grep. The business message a user reads is +unchanged, because their action is unchanged: clear or reassign the referencing +records. diff --git a/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts index 0bf849f2ec..684dc64d07 100644 --- a/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts +++ b/packages/objectql/src/engine-cascade-delete-multivalue-probe.test.ts @@ -88,6 +88,45 @@ const guard: ServiceObject = { }, }; +/** Multi-value, `set_null` spelled OUT — `||` collapses it with the default. */ +const holdExplicit: ServiceObject = { + name: 'mv_hold_explicit', + label: 'Hold (explicit set_null)', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + accounts: { + name: 'accounts', label: 'Accounts', type: 'lookup' as const, + reference: 'mv_acct', multiple: true, deleteBehavior: 'set_null' as const, + }, + }, +}; + +/** Multi-value + explicit `cascade` — the escalation must NOT reach it. */ +const cascadeMulti: ServiceObject = { + name: 'mv_cascade_multi', + label: 'Cascade (multi)', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + accounts: { + name: 'accounts', label: 'Accounts', type: 'lookup' as const, + reference: 'mv_acct', multiple: true, deleteBehavior: 'cascade' as const, + }, + }, +}; + +/** SINGLE-valued, defaulted `set_null` — the limb that must still run. */ +const singleSetNull: ServiceObject = { + name: 'mv_single', + label: 'Single (set_null)', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + account: { + name: 'account', label: 'Account', type: 'lookup' as const, + reference: 'mv_acct', + }, + }, +}; + /** A single-valued lookup on the same target — the unchanged-behaviour control. */ const opp: ServiceObject = { name: 'mv_opp', @@ -106,6 +145,8 @@ const opp: ServiceObject = { const JSON_COLUMNS: Record = { mv_zoo: ['f_lookups'], mv_guard: ['accounts'], + mv_hold_explicit: ['accounts'], + mv_cascade_multi: ['accounts'], }; /** @@ -268,7 +309,9 @@ describe('[#9362] the dependents probe reads a multi-value reference field the w probes = stub.probes; engine.registerDriver(stub.driver, true); await engine.init(); - for (const o of [acct, zoo, guard, opp]) engine.registry.registerObject(o, OWNER_PACKAGE); + for (const o of [acct, zoo, guard, opp, holdExplicit, cascadeMulti, singleSetNull]) { + engine.registry.registerObject(o, OWNER_PACKAGE); + } }); // ── The card's own reproduction. @@ -414,3 +457,126 @@ describe('[#9362] the dependents probe reads a multi-value reference field the w expect(rows(stores, 'mv_acct')).toBe(1); }); }); + +/** + * [#9362 -> #9438] The holding position: a `set_null` limb aimed at a + * SET-valued foreign key refuses instead of writing. + * + * Maintainer-ruled (option B) as a temporary measure to ship alongside the + * probe repair above. The limb it holds back writes `null` over the whole + * array, dropping every other member; the correct semantics is #9438's to + * decide. Refusing decides nothing and is reversible; writing decides it by + * accident and is not. + * + * The suite is written OVER-FIRE FIRST, because that is the way this guard can + * re-break what the probe repair just fixed: every disposition it must NOT + * touch — single-valued `set_null`, multi-value `cascade`, an already-declared + * `restrict`, and a relation with no dependents at all — is asserted here + * beside the two it must catch. + */ +describe('[#9362 -> #9438] set_null on a multi-value reference refuses instead of nulling the array', () => { + let engine: ObjectQL; + let stores: Map>>; + + beforeEach(async () => { + engine = new ObjectQL(); + const stub = makeJsonColumnDriver(); + stores = stub.stores; + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of [acct, zoo, guard, opp, holdExplicit, cascadeMulti, singleSetNull]) { + engine.registry.registerObject(o, OWNER_PACKAGE); + } + }); + + // ── FIRES — and the array is still intact afterwards. + + it('a DEFAULTED set_null on a multi-value field refuses, and writes nothing', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_acct', { id: 'acc_b', name: 'B' }); + await engine.insert('mv_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_a', 'acc_b'] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentObject).toBe('mv_zoo'); + expect(err.dependentCount).toBe(1); + // The whole point: the sibling reference is still there. + expect(stores.get('mv_zoo')?.get('z1')?.f_lookups).toEqual(['acc_a', 'acc_b']); + expect(stores.get('mv_acct')?.has('acc_a')).toBe(true); + }); + + it('an EXPLICITLY authored set_null on a multi-value field refuses too', async () => { + // `fdef.deleteBehavior || 'set_null'` collapses the absent declaration and + // this one into the same value, which is why one `if` covers both — the + // same reason the required-FK escalation beside it covers both. + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_hold_explicit', { id: 'h1', accounts: ['acc_a'] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.dependentObject).toBe('mv_hold_explicit'); + expect(stores.get('mv_hold_explicit')?.get('h1')?.accounts).toEqual(['acc_a']); + }); + + it('the refusal names the hold as TEMPORARY and points at #9438, on the developer half only', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_a'] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.developerMessage).toContain('TEMPORARY'); + expect(err.developerMessage).toContain('objectstack#9438'); + expect(err.developerMessage).toContain('multiple: true'); + // The BUSINESS sentence is the ordinary one — the user's action is the + // same, and #7307 keeps the machine detail off this half. + expect(err.message).not.toContain('9438'); + // And the wire code does not split (operation-message.ts's own rule). + expect(err.code).toBe('DELETE_RESTRICTED'); + }); + + // ── DOES NOT FIRE — four dispositions the hold must leave alone. + + it('a SINGLE-valued set_null still clears the foreign key, exactly as before', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_single', { id: 's1', account: 'acc_a' }); + + await engine.delete('mv_acct', { where: { id: a.id } } as any); + + expect(stores.get('mv_acct')?.has('acc_a')).toBe(false); + expect(stores.get('mv_single')?.get('s1')?.account).toBeNull(); + }); + + it('a multi-value CASCADE still deletes the dependents (the P0 closes for this path)', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_cascade_multi', { id: 'c1', accounts: ['acc_a'] }); + + await engine.delete('mv_acct', { where: { id: a.id } } as any); + + expect(stores.get('mv_acct')?.has('acc_a')).toBe(false); + expect(stores.get('mv_cascade_multi')?.has('c1')).toBe(false); + }); + + it('an already-declared multi-value restrict refuses with its OWN sentence, not the hold\'s', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + await engine.insert('mv_guard', { id: 'g1', name: 'g', accounts: ['acc_a'] }); + + const err: any = await engine.delete('mv_acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.dependentObject).toBe('mv_guard'); + // Configured policy, not a holding position — the two must stay tellable + // apart, which is the whole reason the sentence splits. + expect(err.developerMessage).not.toContain('9438'); + expect(err.developerMessage).not.toContain('TEMPORARY'); + }); + + it('a multi-value set_null relation with NO dependent rows still deletes (the card\'s P0 repro)', async () => { + const a = await engine.insert('mv_acct', { id: 'acc_a', name: 'A' }); + expect(stores.get('mv_zoo')?.size ?? 0).toBe(0); + expect(stores.get('mv_hold_explicit')?.size ?? 0).toBe(0); + + await engine.delete('mv_acct', { where: { id: a.id } } as any); + + expect(stores.get('mv_acct')?.has('acc_a')).toBe(false); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index a9be474a3b..dfa7c83bd7 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -10275,11 +10275,54 @@ export class ObjectQL implements IObjectQLEngine { behavior = 'restrict'; } - // [#9362] The probe's filter is spelled for the KIND of reference field - // it is aimed at, and a multi-value one is narrowed EXACTLY afterwards. - // See `referenceProbeFilter` / `storedReferenceIncludes` for why both - // halves are needed. + // [#9362] Declared here rather than at the probe because BOTH the + // escalation below and the probe's filter spelling turn on it. const multiValued = fdef.multiple === true; + + // [#9362 -> #9438] TEMPORARY HOLDING POSITION. Delete this block, and + // the `multiValueHold` limb of the refusal below, when #9438 lands. + // + // `set_null` on a SET-valued foreign key has no settled meaning yet. + // The limb it would run writes `null` over the WHOLE array, discarding + // every other member — references that have nothing to do with the + // record being deleted. Measured on the real stack: a row holding + // `["acc_a","acc_b"]` re-reads as `null` after `acc_a` is deleted. + // + // That limb has never executed in this codebase: before #8895 the + // dependents probe swallowed its own failure and skipped the relation, + // after #8895 it raised `INVALID_FILTER` and aborted the delete. The + // probe repair one method over is what would make it run, so this is + // not a behaviour being taken away — it is one being held back. + // + // The RIGHT semantics is "remove the deleted member", but the residual + // shape when the array empties (`[]` or `null`) is observable — on the + // read path and to a required multi-value validator — and nothing in + // `FieldSchema` pins it. #9438 answers that; until it does, refusing + // LOUDLY decides nothing, while writing decides it by accident. A + // delete that is refused can still be performed by clearing the + // references first; data dropped by a successful 200 cannot be undone. + // + // Shaped as the required-FK escalation directly above, deliberately + // rather than as a new mechanism: same `behavior` reassignment, same + // one `if`. It reads `behavior === 'set_null'`, which is exactly what + // that escalation reads — `fdef.deleteBehavior || 'set_null'` collapses + // an ABSENT declaration and an explicitly authored `set_null` into one + // value, so both are covered here for the same reason both are covered + // there. Distinguishing them would be new machinery, and would leave + // the explicit spelling running the very write this holds back — the + // author of `set_null` on a set-valued field cannot have chosen "clear + // the whole set", because that semantic has never existed to choose. + // + // An explicit `cascade` or `restrict` is untouched, and a relation with + // no dependent rows still deletes: this only changes the DISPOSITION, + // so the `restrict` branch below is still reached only when the probe + // actually found referencing rows. + let multiValueHold = false; + if (behavior === 'set_null' && multiValued) { + behavior = 'restrict'; + multiValueHold = true; + } + let dependents: any[]; try { dependents = await this.find( @@ -10379,10 +10422,29 @@ export class ObjectQL implements IObjectQLEngine { { locale: msgCtx.locale, translate: msgCtx.translate }, ), ); - err.developerMessage = - `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ${fieldName}` + - `${required ? ` (${fieldName} is required, so it cannot be cleared)` : ''}. ` + - `Delete or reassign them first, or set deleteBehavior:'cascade' on ${childName}.${fieldName}.`; + // [#9362 -> #9438] The hold's reason is DEVELOPER-facing and rides + // `developerMessage` alone — the split #7307 already made on this + // envelope, applied to a third reason. The business `message` is + // unchanged because the USER's action is unchanged: clear or reassign + // the referencing records. Why the wire code does not split either: + // `operation-message.ts` states the rule for this exact envelope — + // "one wire code with two sentences ... splitting the SENTENCE, never + // the code ... `DELETE_RESTRICTED` stays one member of the ADR-0112 + // vocabulary that clients match on". A code minted for a holding + // position would also have to be RETIRED under ADR-0087 when #9438 + // lands, which is the opposite of reverting in one line. `#9438` is + // spelled literally so removing this is one grep. + err.developerMessage = multiValueHold + ? `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ` + + `${fieldName}, which is a multi-value reference (multiple: true) taking the default/declared ` + + `deleteBehavior:'set_null'. This refusal is TEMPORARY and is not a policy you configured: clearing ` + + `that field would null the WHOLE array and drop the other references it holds, and the correct ` + + `semantics ("remove just this member", and what the field holds once empty) is still open — ` + + `objectstack#9438. Until it lands: delete or reassign the referencing records first, or set ` + + `deleteBehavior:'cascade' on ${childName}.${fieldName} if the dependents should go with the parent.` + : `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ${fieldName}` + + `${required ? ` (${fieldName} is required, so it cannot be cleared)` : ''}. ` + + `Delete or reassign them first, or set deleteBehavior:'cascade' on ${childName}.${fieldName}.`; err.code = 'DELETE_RESTRICTED'; err.status = 409; err.object = object; diff --git a/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts index 6c93516fdd..80be1dd4e5 100644 --- a/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts +++ b/packages/runtime/src/cascade-delete-multivalue-lookup-real-driver.integration.test.ts @@ -61,6 +61,27 @@ const GUARD = { }, }; +/** Multi-value + explicit `cascade` — the P0 must close for this path. */ +const CASCADE_MULTI = { + name: 'zz_cascade', + fields: { + name: { type: 'text' }, + accounts: { + type: 'lookup', reference: 'zz_account', + multiple: true, deleteBehavior: 'cascade', + }, + }, +}; + +/** SINGLE-valued, defaulted `set_null` — the limb that must keep running. */ +const SINGLE = { + name: 'zz_single', + fields: { + name: { type: 'text' }, + account: { type: 'lookup', reference: 'zz_account' }, + }, +}; + const OWNER_PACKAGE = 'com.objectstack.test.9362'; describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup — real driver', () => { @@ -135,4 +156,48 @@ describe('[#9362] REST DELETE on an object targeted by a multiple:true lookup expect(err.code).toBe('DELETE_RESTRICTED'); expect(err.status).toBe(409); }); + + // ── [#9362 -> #9438] The holding position, on the real stack. + + it('a multi-value set_null refuses instead of nulling the array, and the array survives', async () => { + const { protocol, real } = await rig([ACCOUNT, FIELD_ZOO]); + const a: any = await engine!.insert('zz_account', { id: 'acc_a', name: 'A' }); + await engine!.insert('zz_account', { id: 'acc_b', name: 'B' }); + await engine!.insert('zz_field_zoo', { id: 'z1', name: 'z', f_lookups: ['acc_a', 'acc_b'] }); + + const err: any = await protocol + .deleteData({ object: 'zz_account', id: a.id }) + .catch((e: any) => e); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + expect(err.developerMessage).toContain('objectstack#9438'); + + // Read the stored array back out of the DATABASE. Before the hold this + // re-read as `null`, taking `acc_b` with it. + const [row]: any[] = await real.find('zz_field_zoo', { where: { id: 'z1' } }); + expect(row.f_lookups).toEqual(['acc_a', 'acc_b']); + expect(await real.count('zz_account')).toBe(2); + }); + + it('a multi-value CASCADE still deletes end to end — the P0 closes for that path', async () => { + const { protocol, real } = await rig([ACCOUNT, CASCADE_MULTI]); + const a: any = await engine!.insert('zz_account', { id: 'acc_a', name: 'A' }); + await engine!.insert('zz_cascade', { id: 'c1', name: 'c', accounts: ['acc_a'] }); + + const res = await protocol.deleteData({ object: 'zz_account', id: a.id }); + expect(res).toMatchObject({ id: 'acc_a', success: true }); + expect(await real.count('zz_account')).toBe(0); + expect(await real.count('zz_cascade')).toBe(0); + }); + + it('a SINGLE-valued set_null still clears the foreign key — the hold does not over-fire', async () => { + const { protocol, real } = await rig([ACCOUNT, SINGLE]); + const a: any = await engine!.insert('zz_account', { id: 'acc_a', name: 'A' }); + await engine!.insert('zz_single', { id: 's1', name: 's', account: 'acc_a' }); + + const res = await protocol.deleteData({ object: 'zz_account', id: a.id }); + expect(res).toMatchObject({ id: 'acc_a', success: true }); + const [row]: any[] = await real.find('zz_single', { where: { id: 's1' } }); + expect(row.account).toBeNull(); + }); });