From 121c864aadc80968610de4198e756b9a8c5d1486 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 14:49:57 +0000 Subject: [PATCH 1/2] fix(objectql): the delete-cascade path's two registry reads propagate instead of inventing 'no relations' (#9002) --- packages/objectql/src/engine.ts | 56 ++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 694ef38b49..773e6119ba 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9924,20 +9924,26 @@ export class ObjectQL implements IObjectQLEngine { * recurses into each child's own cascade; it is bounded by * {@link ObjectQL.MAX_CASCADE_DEPTH}, the ceiling the recursion itself carries. * - * Fails toward `'split'` on anything it cannot resolve (an unregistered - * reference, a `getDriver` throw): the degrade is today's behaviour, while a - * wrong `'atomic'` would manufacture the refusal this verdict exists to - * avoid. + * Fails toward `'split'` on anything it cannot resolve WITHIN the walk (an + * unregistered reference, a `getDriver` throw): the degrade is today's + * behaviour, while a wrong `'atomic'` would manufacture the refusal this + * verdict exists to avoid. + * + * [#9002] That fail-toward rule is about resolving individual PARTICIPANTS. + * It never covered the registry read that produces the participant list in + * the first place, and the `catch → 'none'` that used to sit there was not an + * instance of it: `'none'` is the one verdict that asserts something positive + * about the schema — *nothing references this object* — and an unreadable + * registry cannot support that claim. The swallow's own comment argued the + * direction from `cascadeDeleteRelations` returning on the same failure, so + * that nothing was left un-cascaded to make atomic; once that seam propagates + * the premise is gone, and this one propagates with it. Note the ORDER makes + * this seam the first to speak: `delete()` calls this before it runs the + * cascade, so an unreadable registry now fails the delete before any row is + * touched. */ private planCascadeAtomicity(object: string): 'none' | 'split' | 'atomic' { - let objects: ServiceObject[]; - try { - objects = this._registry.getAllObjects(); - } catch { - // Same swallow as `cascadeDeleteRelations` — an unreadable registry - // cascades nothing, so there is nothing to make atomic. - return 'none'; - } + const objects: ServiceObject[] = this._registry.getAllObjects(); // Which objects reference `name` via a relation the cascade would follow. // The `master_detail`/`lookup` + `reference` test is `cascadeDeleteRelations`'s @@ -10037,12 +10043,26 @@ export class ObjectQL implements IObjectQLEngine { depth = 0, ): Promise { if (id == null || depth >= ObjectQL.MAX_CASCADE_DEPTH) return; - let objects: ServiceObject[]; - try { - objects = this._registry.getAllObjects(); - } catch { - return; - } + // [#9002] The registry read is UNGUARDED, deliberately — and this is the + // statement that decides whether the cascade runs AT ALL. + // + // It used to sit behind a bare `catch { return; }`, which is the #8895 + // shape one layer up: #8895's `catch` invented "no dependents" for ONE + // relation whose probe could not run; this one invented "no relations" for + // EVERY relation at once, before the per-relation probe was ever reached — + // no `restrict` refusal, no `set_null`, no `cascade`, nothing logged, and + // the caller told the delete succeeded. + // + // #8895 ruled the family *discriminate or propagate*. Discrimination needs + // a benign failure class — there it was the unprovisioned child TABLE, + // which genuinely cannot hold a referencing row. Here there is none: an + // unreadable registry is never truthfully "no relations", so `propagate` + // is the whole answer and the `catch` has nothing left to do. + // + // No new error code and no new response field: whatever the registry read + // raised reaches the caller with its envelope intact, exactly as #8895's + // probe failure does. + const objects: ServiceObject[] = this._registry.getAllObjects(); for (const child of objects) { const childName = (child as any)?.name as string | undefined; const fields = (child as any)?.fields as Record | undefined; From 587dc863a1b796e34a70c37dd7a346b41eb91153 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 16:33:31 +0000 Subject: [PATCH 2/2] test(objectql): pin both delete-cascade registry-read seams; repair the vocabulary suite's registry double; changeset (#9002) --- .../cascade-registry-read-propagates.md | 37 ++ ...gine-cascade-registry-read-failure.test.ts | 330 ++++++++++++++++++ ...ne-middleware-operation-vocabulary.test.ts | 12 + 3 files changed, 379 insertions(+) create mode 100644 .changeset/cascade-registry-read-propagates.md create mode 100644 packages/objectql/src/engine-cascade-registry-read-failure.test.ts diff --git a/.changeset/cascade-registry-read-propagates.md b/.changeset/cascade-registry-read-propagates.md new file mode 100644 index 0000000000..ddb4706d48 --- /dev/null +++ b/.changeset/cascade-registry-read-propagates.md @@ -0,0 +1,37 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): the delete-cascade path's two registry reads propagate instead of answering "no relations" (#9002) + +`ObjectQL.delete()`'s by-id branch reads `registry.getAllObjects()` twice, and +both reads sat behind a swallow that invented an answer for a read that never +happened: + +- `planCascadeAtomicity()` — `catch { return 'none' }`. `'none'` is the verdict + that asserts *nothing references this object*, so #7413's cascade atomicity + was silently switched off on a registry nobody could read. +- `cascadeDeleteRelations()`, its first statement — `catch { return }`. That + skips the cascade entirely: no `restrict` refusal, no `set_null`, no + `cascade`, nothing logged, and the caller told the delete succeeded. + +This is the #8895 shape one layer up, with a strictly larger blast radius: +#8895's `catch` invented "no dependents" for one relation whose probe could not +run; this one invented "no relations" for every relation at once, before the +per-relation probe was ever reached. + +#8895 ruled the family *discriminate or propagate*. Discrimination needs a +benign failure class — there, an unprovisioned child table, which genuinely +cannot hold a referencing row. Here there is none: an unreadable registry is +never truthfully "no relations". So both `catch`es are removed and the read's +own failure reaches the caller, envelope intact — no new error code, no new +response field, and the second seam is decided in the same direction as the +first because its own argument rested on the first one firing. + +**No shipped behaviour changes.** `SchemaRegistry.getAllObjects()` is a walk +over in-memory `Map`s (`resolveObject()` returns `undefined` on every failure +branch it models) with no I/O and no `throw` on the measured path, so nothing in +a running deployment can reach either seam today. This is a structural close of +a fail-open shape, pinned by tests, so that the day the registry read grows a +throwing path it fails loudly instead of disabling every referential guard at +once. diff --git a/packages/objectql/src/engine-cascade-registry-read-failure.test.ts b/packages/objectql/src/engine-cascade-registry-read-failure.test.ts new file mode 100644 index 0000000000..6a99bb2e1e --- /dev/null +++ b/packages/objectql/src/engine-cascade-registry-read-failure.test.ts @@ -0,0 +1,330 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#9002] The delete-cascade path's TWO registry reads must not answer a failed + * read with an invented "no relations". + * + * `ObjectQL.delete()`'s by-id branch reads `registry.getAllObjects()` twice, and + * both reads used to sit behind a swallow: + * + * 1. {@link ObjectQL.planCascadeAtomicity} — `catch { return 'none' }`, i.e. + * "nothing references this object, so there is no multi-write unit to make + * atomic" (#7413's atomicity silently switched off); + * 2. `cascadeDeleteRelations()`'s first statement — `catch { return }`, i.e. + * the cascade does not run at all: no `restrict` refusal, no `set_null`, no + * `cascade`, nothing logged, and the caller told the delete succeeded. + * + * That is the #8895 shape one layer up, with a strictly larger blast radius: + * #8895's swallow invented "no dependents" for ONE relation, seam 2 here + * invents "no relations" for EVERY relation at once, before the per-relation + * probe is ever reached. #8895 ruled the family *discriminate or propagate*; + * discrimination needs a benign failure class and there is none here — an + * unreadable registry is never truthfully "no relations" — so both `catch`es + * are gone and the read's own failure reaches the caller. + * + * ⚠️ This pins a STRUCTURAL close, not a live defect. `SchemaRegistry`'s + * `getAllObjects()` is a walk over in-memory `Map`s (`resolveObject` → a spread + * fold; every failure branch returns `undefined`, the orphan-overlay one after + * a `console.warn`) with no I/O and no `throw` on the measured path, so nothing + * shipped can reach these seams today. The tests therefore inject the failure at + * the registry method itself — the injection IS the statement that the seam is + * unreachable from real data, and the pin is what keeps the fail-open shape from + * coming back the day `getAllObjects()` grows a throwing path. + * + * The two seams are told apart by WHICH read fails, not by mocking one function: + * `delete()` calls `planCascadeAtomicity` first and `cascadeDeleteRelations` + * second, so failing read #1 exercises seam 1 and failing read #2 — a read that + * fails once after succeeding, the flaky shape the card names — exercises + * seam 2 with the atomicity plan already computed. Every expectation is written + * against literals (the injected error object's identity, its literal `code` / + * `status` / message, literal row counts), and each is paired with a positive + * control in the same describe, so a harness that had stopped cascading at all + * could not pass vacuously. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { ServiceObject } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; + +/** The package id every fixture below is registered under. */ +const OWNER_PACKAGE = 'test-9002'; + +/* + * 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). + */ +const acct: ServiceObject = { + name: 'acct', + label: 'Account', + fields: { + 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`, so this +// relation is the one whose refusal the swallow used to disable. +const oppRestrict: ServiceObject = { + name: '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: 'acct', + required: true, + }, + }, +}; +// explicit cascade → the seam also decides whether children are REMOVED. +const taskCascade: ServiceObject = { + name: 'task', + label: 'Task', + fields: { + 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' as const, + }, + }, +}; + +/** A minimal in-memory driver — no read-failure injection here, deliberately: + * the failure this file is about happens in the REGISTRY, before any driver + * read, so a driver that always succeeds is what makes that visible. */ +function makeStubDriver() { + const stores = new Map>>(); + 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 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) { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + 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) { + 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 }; +} + +/** 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; + +/** + * Make the engine's registry throw `error` on its Nth `getAllObjects()` call + * counted FROM THIS CALL — every earlier read (setup, warm-up) has already + * happened, so `nth: 1` is the delete's first read and `nth: 2` its second. + * + * Returns the live call counter so each test can assert HOW MANY reads the + * delete actually got to make: that count is what separates "seam 1 stopped it" + * from "seam 2 stopped it", and it is read from the wrapper, never re-derived + * from the code under test. + */ +function failRegistryReadOn( + engine: ObjectQL, + nth: number, + error: unknown, +): { calls: () => number } { + const registry = engine.registry as unknown as { + getAllObjects: (packageId?: string) => ServiceObject[]; + }; + const real = registry.getAllObjects.bind(registry); + let n = 0; + registry.getAllObjects = (packageId?: string): ServiceObject[] => { + n += 1; + if (n === nth) throw error; + return real(packageId); + }; + return { calls: () => n }; +} + +describe('[#9002] the delete-cascade path\'s registry reads must not invent "no relations"', () => { + let engine: ObjectQL; + let stores: Map>>; + + beforeEach(async () => { + engine = new ObjectQL(); + const stub = makeStubDriver(); + stores = stub.stores; + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of [acct, oppRestrict, taskCascade]) { + engine.registry.registerObject(o, OWNER_PACKAGE); + } + }); + + // ── POSITIVE CONTROLS — the registry read SUCCEEDS, so both of the + // cascade's real answers are observable in this harness. Without these, + // every refusal assertion below could pass on a harness that no longer + // cascades at all. + + it('control: a readable registry still refuses a restricted 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 readable registry still cascades the children away', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('task', { title: 'Follow up', account: a.id }); + + await engine.delete('acct', { where: { id: a.id } } as any); + expect(rows(stores, 'acct')).toBe(0); + expect(rows(stores, 'task')).toBe(0); + }); + + it('control: a readable registry still lets a dependent-free 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); + }); + + // ── SEAM 1 — `planCascadeAtomicity`, the FIRST of the delete's two reads. + // It used to answer `'none'`: no transaction opened, and the pre-#7413 + // non-atomic path taken over a schema nobody could read. + + it('seam 1 (planCascadeAtomicity): a failed registry read surfaces and refuses the delete', async () => { + const a = await engine.insert('acct', { name: 'Acme' }); + await engine.insert('opp', { name: 'Deal', account: a.id }); + + const injected = Object.assign(new Error('registry unreadable: contributor fold failed'), { + code: 'REGISTRY_READ_FAILED', + status: 500, + }); + const probe = failRegistryReadOn(engine, 1, injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + + // The caller receives the REGISTRY read's own failure — this fix mints + // no new code and no new response field, so the envelope it arrived + // with is the envelope it leaves with. + expect(err).toBe(injected); + expect(err.message).toBe('registry unreadable: contributor fold failed'); + expect(err.code).toBe('REGISTRY_READ_FAILED'); + expect(err.status).toBe(500); + // Seam 1 runs BEFORE the cascade, so the delete stops at the first read. + expect(probe.calls()).toBe(1); + // …and emphatically NOT the pre-fix outcome: a silent, non-atomic + // success that removed the parent and orphaned the child. + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'opp')).toBe(1); + }); + + it('seam 1: a failed read refuses even a delete with NO dependents — "unreadable" is not "none"', async () => { + // The pre-fix `'none'` verdict was indistinguishable from this object's + // genuine one, which is the whole complaint: `'none'` asserts something + // positive about the schema that an unreadable registry cannot support. + const a = await engine.insert('acct', { name: 'Lonely' }); + + const injected = Object.assign(new Error('registry unreadable'), { code: 'REGISTRY_READ_FAILED' }); + failRegistryReadOn(engine, 1, injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toBe(injected); + expect(rows(stores, 'acct')).toBe(1); + }); + + // ── SEAM 2 — `cascadeDeleteRelations`, the SECOND read. Reached only when + // the first read SUCCEEDED, i.e. a read that fails once after succeeding + // — the flaky shape the card names. Pre-fix this returned silently and + // the parent's own `driver.delete` then ran, unguarded. + + it('seam 2 (cascadeDeleteRelations): a read that fails on the SECOND call surfaces 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('registry unreadable mid-delete'), { + code: 'REGISTRY_READ_FAILED', + status: 500, + }); + const probe = failRegistryReadOn(engine, 2, 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('registry unreadable mid-delete'); + expect(err.code).toBe('REGISTRY_READ_FAILED'); + expect(err.status).toBe(500); + // Proof this is seam 2 and not seam 1: the first read SUCCEEDED (the + // atomicity plan was computed) and the delete died on the second. + expect(probe.calls()).toBe(2); + // Pre-fix: the `restrict` relation was never consulted, the parent row + // was deleted, and the caller was told it succeeded. + expect(rows(stores, 'acct')).toBe(1); + expect(rows(stores, 'opp')).toBe(1); + }); + + it('seam 2: a CASCADE relation is not skipped either — no parent removed 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('registry unreadable mid-delete'), { + code: 'REGISTRY_READ_FAILED', + }); + const probe = failRegistryReadOn(engine, 2, injected); + + const err: any = await engine.delete('acct', { where: { id: a.id } } as any).catch((e) => e); + expect(err).toBe(injected); + expect(probe.calls()).toBe(2); + // Pre-fix this removed 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); + }); +}); diff --git a/packages/objectql/src/engine-middleware-operation-vocabulary.test.ts b/packages/objectql/src/engine-middleware-operation-vocabulary.test.ts index a9a136ccb3..00852f9ec9 100644 --- a/packages/objectql/src/engine-middleware-operation-vocabulary.test.ts +++ b/packages/objectql/src/engine-middleware-operation-vocabulary.test.ts @@ -100,6 +100,18 @@ vi.mock('./registry', () => { const instance: any = { getObject: vi.fn(), resolveObject: vi.fn((n: string) => instance.getObject(n)), + // [#9002] This double used to omit `getAllObjects`, and the suite passed + // anyway: `delete()`'s by-id branch reads it twice (`planCascadeAtomicity`, + // then `cascadeDeleteRelations`) and BOTH reads sat behind a `catch` that + // answered "no relations". The swallow absorbed the `TypeError` this + // omission raises just as silently as it would absorb a real read failure, + // so an incomplete double read as a registry with nothing in it. With the + // swallows gone the omission is a hard failure, which is the point — the + // double now has to model the method the engine actually calls. Empty is + // the right body here: this suite pins the middleware operation VOCABULARY + // and registers no relations, so "no object references the deleted one" is + // the truthful answer rather than an invented one. + getAllObjects: vi.fn(() => []), registerObject: vi.fn(), getObjectOwner: vi.fn(), registerNamespace: vi.fn(),