From af9ed06586c1ffd25aff5c60af7f9343c9d84287 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 04:47:50 +0000 Subject: [PATCH 1/2] fix(objectql): surface a failed cascade-delete dependents probe instead of skipping the guard (#8895) --- ...ngine-cascade-delete-probe-failure.test.ts | 274 ++++++++++++++++++ packages/objectql/src/engine.ts | 38 ++- 2 files changed, 310 insertions(+), 2 deletions(-) create mode 100644 packages/objectql/src/engine-cascade-delete-probe-failure.test.ts diff --git a/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts b/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts new file mode 100644 index 0000000000..3330a904e2 --- /dev/null +++ b/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts @@ -0,0 +1,274 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8895] `cascadeDeleteRelations` must not fail OPEN when its dependents probe + * fails. + * + * The probe (`find(child, { where: { fk: id } })`) IS the referential-integrity + * guard: its result decides whether a `restrict` relation refuses the delete and + * whether `set_null`/`cascade` run at all. It used to sit behind a bare + * `catch { continue; }`, so ANY probe failure — a connection drop, a timeout, a + * permission denial, a query error — was indistinguishable from "this child has + * no rows": the `restrict` refusal never fired, the delete reported success, and + * nothing was logged. A guard that could not be EVALUATED silently passed. + * + * The repair is discrimination, not deletion of the `catch`: only an + * unprovisioned child TABLE is truthful emptiness (it cannot hold a referencing + * row), and everything else propagates. + * + * Every expectation below is written against LITERALS — the exact injected error + * object, its literal message, the literal `DELETE_RESTRICTED` / 409 envelope, + * literal row counts — never against a value re-derived from the code under + * test. And each refusal assertion is paired with a positive control in this + * same describe (the probe SUCCEEDING and refusing, the probe SUCCEEDING and + * allowing, and — for the benign branch — proof that the injected throw actually + * fired), so a harness that had stopped exercising the seam at all could not + * pass vacuously. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const acct = { + name: 'acct', + label: 'Account', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + }, +}; +// required lookup → the defaulted set_null escalates to `restrict` +const oppRestrict = { + name: 'opp', + label: 'Opportunity', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true }, + }, +}; +// explicit cascade → the probe decides whether children are REMOVED +const taskCascade = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', type: 'text' as const }, + account: { + name: 'account', + type: 'lookup' as const, + reference: 'acct', + required: true, + deleteBehavior: 'cascade', + }, + }, +}; + +/** + * Stub driver + a per-object read-failure injector. + * + * `failReads.set('opp', err)` makes every `find`/`count` on `opp` throw exactly + * `err` — the object identity is what the assertions below check, so nothing has + * to guess at how the engine re-wraps a driver error. `readCalls` records which + * objects were actually read, which is what turns "the delete succeeded" into + * "the delete succeeded AND the probe really threw". + */ +function makeStubDriver() { + const stores = new Map>>(); + const failReads = new Map(); + const readCalls: string[] = []; + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + const gate = (o: string) => { + readCalls.push(o); + if (failReads.has(o)) throw failReads.get(o); + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(o: string, ast: any) { + gate(o); + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + gate(o); + for (const r of storeFor(o).values()) if (matches(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) { + gate(o); + return Array.from(storeFor(o).values()).filter((r) => matches(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, failReads, readCalls }; +} + +/** Row count read straight out of the stub's store — never through the engine. */ +const rows = (stores: Map>>, object: string) => + stores.get(object)?.size ?? 0; + +describe('[#8895] cascadeDeleteRelations — a failed dependents probe must not skip the guard', () => { + let engine: ObjectQL; + let stores: Map>>; + let failReads: Map; + let readCalls: string[]; + + beforeEach(async () => { + engine = new ObjectQL(); + const stub = makeStubDriver(); + stores = stub.stores; + failReads = stub.failReads; + readCalls = stub.readCalls; + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of [acct, oppRestrict, taskCascade]) engine.registry.registerObject(o); + }); + + // ── POSITIVE CONTROLS — the probe RUNS, so the guard's two real answers + // are both observable in this harness. Without these, every refusal + // assertion below could pass on a harness that no longer cascades at all. + + it('control: a probe that RUNS and finds a dependent refuses the delete (DELETE_RESTRICTED, 409)', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('opp', { name: 'Deal', account: a.id }); + + const err: any = await engine.delete('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('opp'); + expect(err.dependentCount).toBe(1); + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'opp')).toBe(1); + }); + + it('control: a probe that RUNS and finds nothing lets the delete through', async () => { + const a = await engine.insert('acct', { name: 'Empty' }); + await engine.delete('acct', { where: { id: a.id } } as any); + expect(rows(stores, 'acct')).toBe(0); + }); + + // ── THE FIX — a probe that could not run must surface, not invent "none". + + it('a probe that fails with a CONNECTION error surfaces that error and deletes nothing', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('opp', { name: 'Deal', account: a.id }); + + const injected = Object.assign(new Error('connection terminated unexpectedly'), { + code: 'ECONNRESET', + }); + failReads.set('opp', injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + // The caller receives the PROBE's own failure, envelope intact — this + // fix mints no new code and no new response field. + expect(err).toBe(injected); + expect(err.message).toBe('connection terminated unexpectedly'); + expect(err.code).toBe('ECONNRESET'); + // …and emphatically NOT the pre-fix outcome: a silent success. + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'opp')).toBe(1); + }); + + it('a probe that fails with a missing COLUMN on an existing relation surfaces it (the superstring case)', async () => { + // Postgres phrases this failure as `column "x" of relation "opp" does + // not exist` — which CONTAINS a complete, legal missing-table phrase + // (`relation "opp" does not exist`). The table is there; the read still + // did not happen. `isMissingTableError`'s front-exclusion is what keeps + // this loud, and this pin is what keeps a future hand-rolled code test + // from quietly reading it as benign. + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('opp', { name: 'Deal', account: a.id }); + + const injected = Object.assign( + new Error('column "amount" of relation "opp" does not exist'), + { code: '42703' }, + ); + failReads.set('opp', injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toBe(injected); + expect(err.message).toBe('column "amount" of relation "opp" does not exist'); + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'opp')).toBe(1); + }); + + it('a failed probe on a CASCADE relation surfaces too — no parent deleted over unread children', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('task', { title: 'Follow up', account: a.id }); + + const injected = Object.assign(new Error('query timed out'), { code: 'ETIMEDOUT' }); + failReads.set('task', injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toBe(injected); + expect(err.message).toBe('query timed out'); + // Pre-fix this deleted the parent and left the child pointing at a row + // that no longer exists. + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'task')).toBe(1); + }); + + // ── THE ONE BENIGN CASE — an unprovisioned child table cannot hold a + // referencing row, so "no dependents" is the truth and the delete runs. + + it('an UNPROVISIONED child table (sqlite phrasing) is truthful emptiness: the delete proceeds', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + failReads.set('opp', new Error('no such table: opp')); + + readCalls.length = 0; + await engine.delete('acct', { where: { id: a.id } } as any); + + expect(rows(stores, 'acct')).toBe(0); + // Proof the benign branch was actually EXERCISED — the probe ran and + // threw. Without this, the passing delete above would be consistent + // with a harness that never probes `opp` at all. + expect(readCalls).toContain('opp'); + }); + + it('an UNPROVISIONED child table (postgres 42P01) is truthful emptiness: the delete proceeds', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + failReads.set('opp', Object.assign(new Error('relation "opp" does not exist'), { + code: '42P01', + })); + + readCalls.length = 0; + await engine.delete('acct', { where: { id: a.id } } as any); + + expect(rows(stores, 'acct')).toBe(0); + expect(readCalls).toContain('opp'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index ad3381c6dc..694ef38b49 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -10081,8 +10081,42 @@ export class ObjectQL implements IObjectQLEngine { let dependents: any[]; try { dependents = await this.find(childName, { where: { [fieldName]: id }, context } as any); - } catch { - continue; + } catch (error) { + // [#8895] Discriminate by error TYPE — this probe IS the referential + // guard, so `continue` is only truthful for the one failure that + // really means "no dependents". + // + // The bare `catch { continue }` this replaces reached `continue` on + // ANY probe failure, and every consequence of that is silent: the + // `restrict` branch below never fires, so a delete the integrity + // rules say must be REFUSED is allowed through; `set_null`/`cascade` + // never run, so child rows that should have been nulled or removed + // are left orphaned; nothing is logged and the caller is told the + // delete succeeded. Fail-OPEN on an integrity guard — the read did + // not happen and the answer "there are none" was invented for it + // (ADR-0110 D3: "the probe found nothing" and "the probe could not + // run" are different facts with opposite meanings here). + // + // Benign: the child object is registered but its TABLE was never + // provisioned (schema sync not run yet). It cannot hold a row that + // references anything, so zero dependents IS the truth and skipping + // the relation is correct. Asked through the shared + // `isMissingTableError` predicate (`@objectstack/metadata/errors`, + // #4825) — the same call `seedAutonumber` and `resolveFileReferences` + // make — never a hand-rolled code test. + // + // Everything else (connection drop, timeout, permission denial, a + // query error, a missing COLUMN on a provisioned table) means the + // dependents may well exist and simply were not seen. It propagates: + // the delete fails loudly and nothing is written, which is the same + // disposition the maintainer's 2026-08-15 ruling gives this family — + // unprovisioned is truthful emptiness, everything else must surface. + // A guard that could not be EVALUATED must not silently pass. + // + // No new response field and no new error code: the caller receives + // the probe's own failure, envelope intact. + if (isMissingTableError(error)) continue; + throw error; } if (!dependents || dependents.length === 0) continue; From cafeea779bb0c28af09a884817519f6f0f0e9740 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 05:07:24 +0000 Subject: [PATCH 2/2] test(objectql): pin the cascade-delete probe-failure discrimination; changeset (#8895) --- .../cascade-delete-probe-failure-surfaces.md | 41 +++++++++++++++++ ...ngine-cascade-delete-probe-failure.test.ts | 46 ++++++++++++++----- 2 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 .changeset/cascade-delete-probe-failure-surfaces.md diff --git a/.changeset/cascade-delete-probe-failure-surfaces.md b/.changeset/cascade-delete-probe-failure-surfaces.md new file mode 100644 index 0000000000..e9c24d7024 --- /dev/null +++ b/.changeset/cascade-delete-probe-failure-surfaces.md @@ -0,0 +1,41 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a cascade-delete dependents probe that FAILS no longer skips the referential guard — only an unprovisioned child table is read as "no dependents" (#8895) + +`ObjectQL.cascadeDeleteRelations()` probes each child relation +(`find(child, { where: { fk: id } })`) to decide what the parent's delete must +do. That probe **is** the referential-integrity guard, and it sat behind a bare +`catch { continue; }` — so **any** failure of it (a connection drop, a timeout, +a permission denial, a query error, a missing column) was indistinguishable +from "this child has no rows": + +- a `deleteBehavior: 'restrict'` relation never refused the delete, so a delete + the integrity rules say must be **refused was allowed through**; +- `set_null` / `cascade` never ran, so child rows that should have been nulled + or removed were **left orphaned**, pointing at a parent that no longer exists; +- nothing was logged and nothing was returned, so the caller was told the + delete **succeeded**. + +That is fail-OPEN on an integrity guard: the read never happened and the answer +"there are none" was invented for it (ADR-0110 D3 — "the probe found nothing" +and "the probe could not run" are different facts, and here they have opposite +meanings). + +The `catch` is not removed; it is **discriminated by error type**, through the +same shared `isMissingTableError` predicate (`@objectstack/metadata/errors`) +that `seedAutonumber` and `resolveFileReferences` already use: + +- **benign, unchanged** — the child object is registered but its **table** was + never provisioned (schema sync not run yet). It cannot hold a row referencing + anything, so zero dependents is the truth and the relation is skipped exactly + as before. +- **everything else now surfaces** — the delete fails with the probe's own + error, envelope intact, and nothing is written. A guard that could not be + **evaluated** must not silently pass. + +No new error code, no new response field: the caller receives the failure the +probe itself raised. The only behavioural change is that a delete which used to +report success over an unreadable child relation now reports the failure that +made the relation unreadable. diff --git a/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts b/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts index 3330a904e2..21c4f06d5d 100644 --- a/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts +++ b/packages/objectql/src/engine-cascade-delete-probe-failure.test.ts @@ -27,39 +27,59 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; import { ObjectQL } from './engine.js'; -const acct = { +/** The package id every fixture below is registered under. */ +const OWNER_PACKAGE = 'test-8895'; + +/* + * The fixtures are typed as `ServiceObject` (and registered WITH their + * `packageId`) rather than left to inference, so this file adds nothing to + * `@objectstack/objectql`'s TEST_DEBT ledger — a shrink-only ratchet (#5278). + * Typing them is also what shows that `primaryKey` is not a declared field + * property: the registry provisions the primary key itself, so carrying it + * would be a silent no-op — which is why the sibling cascade fixtures' `id` + * declaration is not copied here. + */ +const acct: ServiceObject = { name: 'acct', label: 'Account', fields: { - id: { name: 'id', type: 'text' as const, primaryKey: true }, - name: { name: 'name', type: 'text' as const }, + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, }, }; // required lookup → the defaulted set_null escalates to `restrict` -const oppRestrict = { +const oppRestrict: ServiceObject = { name: 'opp', label: 'Opportunity', fields: { - id: { name: 'id', type: 'text' as const, primaryKey: true }, - name: { name: 'name', type: 'text' as const }, - account: { name: 'account', type: 'lookup' as const, reference: 'acct', required: true }, + 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: 'acct', + required: true, + }, }, }; // explicit cascade → the probe decides whether children are REMOVED -const taskCascade = { +const taskCascade: ServiceObject = { name: 'task', label: 'Task', fields: { - id: { name: 'id', type: 'text' as const, primaryKey: true }, - title: { name: 'title', type: 'text' as const }, + id: { name: 'id', label: 'ID', type: 'text' as const }, + title: { name: 'title', label: 'Title', type: 'text' as const }, account: { name: 'account', + label: 'Account', type: 'lookup' as const, reference: 'acct', required: true, - deleteBehavior: 'cascade', + deleteBehavior: 'cascade' as const, }, }, }; @@ -155,7 +175,9 @@ describe('[#8895] cascadeDeleteRelations — a failed dependents probe must not readCalls = stub.readCalls; engine.registerDriver(stub.driver, true); await engine.init(); - for (const o of [acct, oppRestrict, taskCascade]) engine.registry.registerObject(o); + for (const o of [acct, oppRestrict, taskCascade]) { + engine.registry.registerObject(o, OWNER_PACKAGE); + } }); // ── POSITIVE CONTROLS — the probe RUNS, so the guard's two real answers