From db32a20decfac3521166e7a52977a78cf1119c12 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:46:25 +0000 Subject: [PATCH 1/2] fix(objectql): retire `sys_fetch_previous_delete` so the delete-path per-object gate is honest (#5929) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete()`'s prior-row read is gated per object on `hasHooksFor('beforeDelete', object) || hasHooksFor('afterDelete', object) || getSummaryDescriptors(object).length > 0`. On any kernel-hosted engine the first term was constant true, because `ObjectQLPlugin` registered its own `sys_fetch_previous_delete` builtin with `object: '*'` — so the per-object skip the gate exists to perform never happened outside the bare engines unit tests boot. The builtin could not use what it held open. Since #5272 (by-id) and #6697 (predicate path, per matched row) the engine reads the pre-image and binds `previous` before `beforeDelete` dispatches, so its `!ctx.previous` guard was permanently false and it issued no read. Its only remaining effect was holding open the gate that made it redundant. Retired under ADR-0049 enforce-or-remove; the measurement #5846 recorded in `plugin.ts` was re-verified on this branch rather than taken on trust. The gate's three terms are unchanged — no term was added or removed. What changed is that term 1 now reflects real hooks. `engine.ts` gains the enumeration of the delete-phase hooks that still register globally (plugin-auth, plugin-sharing, service-storage; plugin-audit narrows at the engine face with `excludeObjects`), so nobody reads a skip into a trace that will not show one. New `engine-delete-prior-read-scope.test.ts` pins the three terms per object, the `excludeObjects` subtraction on both phases, the predicate path's twin gate, and — on a real `ObjectKernel` + `ObjectQLPlugin`, the only configuration where the defect was observable — the zero-read skip and the still-bound `previous`. It replays the retired builtin's own shape and measures its guard short-circuiting, so "the guard can no longer be true" stays a measurement. The by-id `beforeDelete` REPOINT behaviour is deliberately untouched (#6752). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq --- .../retire-delete-fetch-previous-builtin.md | 49 ++ .../engine-delete-prior-read-scope.test.ts | 564 ++++++++++++++++++ packages/objectql/src/engine.ts | 42 ++ packages/objectql/src/plugin.ts | 93 +-- .../plugin-auth/src/last-admin-guard.ts | 8 +- 5 files changed, 709 insertions(+), 47 deletions(-) create mode 100644 .changeset/retire-delete-fetch-previous-builtin.md create mode 100644 packages/objectql/src/engine-delete-prior-read-scope.test.ts diff --git a/.changeset/retire-delete-fetch-previous-builtin.md b/.changeset/retire-delete-fetch-previous-builtin.md new file mode 100644 index 0000000000..e80d8a9475 --- /dev/null +++ b/.changeset/retire-delete-fetch-previous-builtin.md @@ -0,0 +1,49 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): retire `sys_fetch_previous_delete`, so `delete()`'s per-object prior-row gate is a real question (#5929) + +`delete()` reads the doomed row's pre-image only when something on **this +object** consumes it — a delete-side hook in either phase, or a roll-up summary +aggregating it. That gate has been per object since #5272, and on a +kernel-hosted engine it was **constant true for every object that has ever +existed**, so the skip it exists to perform never happened outside the unit +tests' bare engines. + +The reason was `ObjectQLPlugin`'s own builtin. `sys_fetch_previous_delete` +registered on `beforeDelete` with `object: '*'`, which made the gate's first +term true everywhere — and it could not use what it held open: since #5272 (and +#6697, which extended the same ordering to the predicate path, per matched row) +the engine reads the pre-image and binds `previous` **before** `beforeDelete` +dispatches, so the builtin's own `if (input.id && !ctx.previous)` guard was +permanently false and it issued no read. Its only remaining effect was holding +open the gate that made it redundant. Retired under ADR-0049 +enforce-or-remove. + +**What changes for you, in both directions:** + +- An object with **no** delete-side hook and no roll-up summary now performs + **no prior-row read** on a by-id `delete()`. Previously it always did, on any + kernel. +- Where a delete-side hook **does** exist, `previous` still arrives bound, + identically — from the engine's own read, which was already the only producer. + Hook handlers, declarative `condition`s reading `previous`, the record-change + trigger, the audit diff and the summary recompute are all unaffected. +- The residual shape the retired guard could still have been true for — the + engine's read found nothing because the row is already gone — is one where the + builtin's read found nothing either, so it binds nothing there too. `previous` + stays **unbound** rather than fabricated as `{}`, unchanged (#4649/#4775). + +Nothing about hook dispatch, the by-id repoint re-resolution, or the delete +dispatch ladder changes. The gate's three terms are unchanged — what changed is +that its first term now reflects **real** hooks. + +One caveat worth stating so nobody reads a skip into a trace that will not show +one: a kernel that also loads plugin-auth, plugin-sharing or service-storage +still has the gate held open on every object, because each registers a +delete-phase hook with no `object` and decides applicability inside its handler. +Those are real consumers, not circular ones. plugin-audit is the exception and +the worked example — it narrows at the engine face with `excludeObjects` +(#5860), so an excluded object really does skip the read. The full enumeration +is in `engine.ts` beside `wantsPreImage`. diff --git a/packages/objectql/src/engine-delete-prior-read-scope.test.ts b/packages/objectql/src/engine-delete-prior-read-scope.test.ts new file mode 100644 index 0000000000..c2e37ae319 --- /dev/null +++ b/packages/objectql/src/engine-delete-prior-read-scope.test.ts @@ -0,0 +1,564 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5929] `delete()`'s prior-row read is demanded PER OBJECT — and on a + * kernel-hosted engine that demand is finally answerable. + * + * ## The defect + * + * The gate has asked per object since #5272: + * + * `hasHooksFor('beforeDelete', object) || hasHooksFor('afterDelete', object) + * || getSummaryDescriptors(object).length > 0` + * + * and on any kernel it was CONSTANT TRUE, because `ObjectQLPlugin` registered a + * builtin of its own — `sys_fetch_previous_delete`, `object: '*'`, priority 5, + * `beforeDelete` — which made the first term true for every object that has + * ever existed. The per-object skip the gate exists to perform therefore never + * happened on a real deployment, only on the bare `new ObjectQL()` engines the + * unit tests boot. + * + * The builtin could not even use what it held open. #5272 made the engine read + * the pre-image and bind `previous` BEFORE dispatching `beforeDelete` (#6697 + * extended the same ordering to the predicate path, per matched row), so the + * builtin's own `if (input.id && !ctx.previous)` guard was permanently false and + * it issued no read. Circular: its only remaining effect was holding open the + * gate that made it redundant. Retired under ADR-0049 enforce-or-remove. + * + * ## What this file pins, and why it needed a KERNEL to pin it + * + * Sections 1–3 run on a bare engine and pin the gate's three terms, the + * `excludeObjects` subtraction, and the direction the gate is allowed to be + * wrong in (looser, never tighter). They would have passed before the + * retirement too — a bare engine never carried the builtin. + * + * Section 4 is the regression pin proper, and it boots a real `ObjectKernel` + * with `ObjectQLPlugin`, because that is the only configuration in which the + * defect was ever observable. `expect(reads).toBe(0)` there was `1` before this + * change. + * + * Section 5 replays the retired builtin's own shape as an authored hook and + * measures its guard short-circuiting, so "the guard can no longer be true" + * stays a measurement rather than a claim that rots. It is the delete-side twin + * of the case #5846 left in `engine-update-prior-read-scope.test.ts`. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQL } from './engine.js'; +import { ObjectQLPlugin } from './plugin.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import type { Hook, ObjectSchema } from '@objectstack/spec/data'; + +const TASK_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, +}; + +/** Two plain objects, neither declaring anything that needs a prior row. */ +const taskA = { name: 'del_scope_a', label: 'A', fields: TASK_FIELDS }; +const taskB = { name: 'del_scope_b', label: 'B', fields: TASK_FIELDS }; + +/** + * Declares a `readonlyWhen` field, so `needsPriorRecord(schema)` is true for it + * — the term this gate deliberately does NOT carry (section 1's last case). + */ +const lockedTask = { + name: 'del_scope_locked', + label: 'Locked', + fields: { + ...TASK_FIELDS, + title: { + name: 'title', label: 'Title', type: 'text' as const, + readonlyWhen: 'record.done == true', + }, + }, +}; + +/** Parent/child pair for the roll-up demand. */ +const invoice = { + name: 'del_scope_invoice', + label: 'Invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + line_total: { + name: 'line_total', label: 'Total', type: 'summary' as const, + summaryOperations: { object: 'del_scope_invoice_line', field: 'amount', function: 'sum' }, + }, + }, +}; +const invoiceLine = { + name: 'del_scope_invoice_line', + label: 'Line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + invoice: { name: 'invoice', label: 'Invoice', type: 'master_detail' as const, reference: 'del_scope_invoice' }, + }, +}; + +/** + * A driver that counts every read PER OBJECT, so "pays no prior read" is a + * measurement rather than an assertion about the code that was written. + * + * Per object because a delete legitimately reads OTHER objects — the roll-up + * recompute aggregates the child set, `cascadeDeleteRelations` looks for + * dependants. Counting only the total would conflate those with the pre-image + * read this file is about. + */ +function makeCountingDriver() { + const stores = new Map>>(); + const reads = { findOne: 0, find: 0, findOneOn: {} as Record, findOn: {} as Record }; + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((sub) => matchesWhere(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'del-counting', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(object: string, ast: any) { + reads.find += 1; + reads.findOn[object] = (reads.findOn[object] ?? 0) + 1; + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + reads.findOne += 1; + reads.findOneOn[object] = (reads.findOneOn[object] ?? 0) + 1; + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { + // Same reason as the bulk verbs below: a test's own `count()` assertion + // must not inflate the read counters it is asserting alongside. + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)).length; + }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + // ⚠️ The bulk verbs resolve their own row set WITHOUT going through + // `find()`. Routing them through it would charge the engine for a read the + // engine never issued — and this file's whole subject is read COUNTS, so a + // driver-internal read masquerading as an engine one would make every + // predicate-path number one too high. + async updateMany(object: string, ast: any, data: Record) { + const rows = Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + for (const r of rows) storeFor(object).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads, storeFor }; +} + +async function boot(hooks: Hook[] = [], objects: unknown[] = [taskA, taskB]) { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o as any); + if (hooks.length > 0) { + bindHooksToEngine(engine, hooks, { + packageId: 'app:del-scope', + logger: { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }, + }); + } + return { engine, reads: stub.reads, storeFor: stub.storeFor }; +} + +const observer = ( + name: string, object: string, event: string, sink: Array, extra: Record = {}, +): Hook => ({ + name, object, events: [event], priority: 90, + handler: (ctx: any) => { sink.push(ctx.previous); }, + ...extra, +} as unknown as Hook); + +/* ──────────────────────────────────────────────────────────────────────────── + * 1. The three terms, asked PER OBJECT + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5929] the delete-side prior-row demand is asked per object', () => { + it('object A pays NO prior read while only object B has an afterDelete hook', async () => { + const { engine, reads } = await boot([observer('audits_b', 'del_scope_b', 'afterDelete', [])]); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(0); + // The row really went — a skipped read must not have skipped the write. + expect(await engine.count('del_scope_a', {} as any)).toBe(0); + }); + + it('the object that DOES have the afterDelete hook pays it, and `previous` is the stored row', async () => { + const seen: Array = []; + const { engine, reads } = await boot([observer('audits_b', 'del_scope_b', 'afterDelete', seen)]); + + const row: any = await engine.insert('del_scope_b', { title: 'B', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_b'] ?? 0; + await engine.delete('del_scope_b', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_b'] ?? 0) - before).toBe(1); + expect(seen).toEqual([{ id: row.id, title: 'B', status: 'todo', done: false }]); + }); + + it('a `beforeDelete` hook holds the gate open ALONE, and reads the row it opened it for', async () => { + // Term 1 on its own, with nothing hooking the after phase. This is the term + // the retired builtin used to satisfy for every object at once; it has to + // still work when a REAL hook satisfies it, or the retirement would have + // taken a live demand with it. + const seen: Array = []; + const { engine, reads } = await boot([observer('pre_a', 'del_scope_a', 'beforeDelete', seen)]); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); + expect(seen).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + }); + + it('a hook registered for `*` still demands the read on every object', async () => { + // The direction that matters: `hasHooksFor` mirrors `triggerHooks`' own + // filter, so an entry targeting `'*'` DOES reach this object. Getting the + // gate looser than dispatch costs a query; getting it TIGHTER would drop + // hooks that were going to fire. The retirement removed a `'*'` entry from + // the KERNEL — it did not narrow what `'*'` means. + const seen: Array = []; + const { engine, reads } = await boot([{ + name: 'audits_everything', object: '*', events: ['beforeDelete'], priority: 90, + handler: (ctx: any) => { seen.push(ctx.previous); }, + } as unknown as Hook]); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); + expect(seen).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + }); + + it('a roll-up summary alone forces the read, with no hooks registered anywhere', async () => { + // Term 3. `recomputeSummaries` reads the doomed row's FK to find the parent + // to recompute, so a deployment with no delete hook at all still owes this + // read — and the parent must actually come back down. + const { engine, reads, storeFor } = await boot([], [invoice, invoiceLine]); + + const parent: any = await engine.insert('del_scope_invoice', { name: 'INV-1' }); + const line: any = await engine.insert('del_scope_invoice_line', { invoice: parent.id, amount: 40 }); + expect(storeFor('del_scope_invoice').get(parent.id)?.line_total).toBe(40); + + const before = reads.findOneOn['del_scope_invoice_line'] ?? 0; + await engine.delete('del_scope_invoice_line', { where: { id: line.id } } as any); + + expect((reads.findOneOn['del_scope_invoice_line'] ?? 0) - before).toBe(1); + expect(storeFor('del_scope_invoice').get(parent.id)?.line_total).toBe(0); + }); + + it('`needsPriorRecord` is NOT a term — a readonlyWhen object still skips the read', async () => { + // Stated as a case because it is the one asymmetry with `update()`'s twin + // gate, and an honest gate is exactly where someone would reflexively add + // it back. `delete()` evaluates no validation rules and no field + // predicates, so the term would buy a read with no reader. + const { engine, reads } = await boot([], [lockedTask]); + + const row: any = await engine.insert('del_scope_locked', { title: 'Ship it', status: 'done', done: true }); + const before = reads.findOneOn['del_scope_locked'] ?? 0; + await engine.delete('del_scope_locked', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_locked'] ?? 0) - before).toBe(0); + expect(await engine.count('del_scope_locked', {} as any)).toBe(0); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 2. `excludeObjects` — the registration face that actually narrows the gate + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5929 / #5860] the gate honours `excludeObjects` on both delete phases', () => { + /** + * plugin-audit's delivered registration face (#5860), replayed here rather + * than imported: `@objectstack/objectql` cannot depend on a plugin that + * depends on it. What is pinned is the ENGINE's half — `hookMatchesObject`'s + * subtract step reaching `hasHooksFor`, hence reaching this read. + * + * ⚠️ `registerHook` directly, NOT `bindHooksToEngine`: the binder refuses a + * hook whose `object` is absent or empty rather than widening it to `'*'` + * (#4001), so the global-minus-exclusions face is only expressible on the + * engine's own registration API — which is exactly the API plugin-audit uses. + */ + const registerGlobalDeleteHook = ( + engine: ObjectQL, event: string, sink: unknown[], excludeObjects: string[], + ): void => { + (engine as any).registerHook(event, (ctx: any) => { sink.push(ctx.object); }, { + excludeObjects, packageId: 'app:del-audit-face', priority: 90, + }); + }; + + it('an EXCLUDED object skips the prior read, and the hook does not fire on it', async () => { + const fired: unknown[] = []; + const { engine, reads } = await boot(); + registerGlobalDeleteHook(engine, 'beforeDelete', fired, ['del_scope_a']); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(0); + // The read count and the dispatch agree — which is the property, not a + // coincidence: both ask `hookMatchesObject`. + expect(fired).toEqual([]); + }); + + it('a NON-excluded object still pays it and still dispatches', async () => { + const fired: unknown[] = []; + const { engine, reads } = await boot(); + registerGlobalDeleteHook(engine, 'beforeDelete', fired, ['del_scope_a']); + + const row: any = await engine.insert('del_scope_b', { title: 'B', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_b'] ?? 0; + await engine.delete('del_scope_b', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_b'] ?? 0) - before).toBe(1); + expect(fired).toEqual(['del_scope_b']); + }); + + it('the after phase subtracts the same way', async () => { + const fired: unknown[] = []; + const { engine, reads } = await boot(); + registerGlobalDeleteHook(engine, 'afterDelete', fired, ['del_scope_a']); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(0); + expect(fired).toEqual([]); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 3. The bulk path is gated the same way (unchanged by this card, pinned so + * the two halves cannot drift apart unnoticed) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5929] the predicate delete path asks the same per-object question', () => { + it('a bulk delete on a hook-free object reads no matched row set', async () => { + const { engine, reads } = await boot([observer('elsewhere', 'del_scope_b', 'beforeDelete', [])]); + await engine.insert('del_scope_a', { title: 'A', status: 'stale', done: false }); + + const before = reads.findOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { multi: true, where: { status: 'stale' } } as any); + + expect((reads.findOn['del_scope_a'] ?? 0) - before).toBe(0); + expect(await engine.count('del_scope_a', {} as any)).toBe(0); + }); + + it('…and reads it exactly once when the object IS hooked', async () => { + const seen: Array = []; + const { engine, reads } = await boot([observer('pre_a', 'del_scope_a', 'beforeDelete', seen)]); + await engine.insert('del_scope_a', { title: 'A', status: 'stale', done: false }); + + const before = reads.findOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { multi: true, where: { status: 'stale' } } as any); + + expect((reads.findOn['del_scope_a'] ?? 0) - before).toBe(1); + expect(seen).toHaveLength(1); + expect((seen[0] as any).title).toBe('A'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 4. THE REGRESSION PIN — a real kernel, where the defect actually lived + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5929] on a KERNEL-hosted engine the per-object skip finally happens', () => { + /** + * `logger.level: 'silent'` and `gracefulShutdown: false` for the reasons + * `plugin.integration.test.ts` records: a test kernel must not log its whole + * bootstrap, and must not install process-wide signal handlers that vitest's + * SIGTERM worker recycling then races with. + */ + async function bootKernel(objects: ObjectSchema[]) { + const kernel = new ObjectKernel({ logger: { level: 'silent' }, gracefulShutdown: false } as any); + const stub = makeCountingDriver(); + await kernel.use({ + name: 'del-counting-plugin', type: 'driver', version: '1.0.0', + init: async (ctx: any) => { ctx.registerService('driver.del-counting', stub.driver); }, + } as any); + await kernel.use(new ObjectQLPlugin()); + await kernel.bootstrap(); + const engine = kernel.getService('objectql') as any; + for (const o of objects) engine.registry.registerObject(o, 'test', 'test'); + return { kernel, engine, reads: stub.reads }; + } + + const kernelTask: ObjectSchema = { + name: 'del_kernel_task', label: 'Kernel Task', datasource: 'del-counting', + fields: TASK_FIELDS, + } as unknown as ObjectSchema; + + it('registers NO `beforeDelete` hook of its own — the builtin is gone', async () => { + // The retirement stated as a fact about the booted engine rather than about + // `plugin.ts`'s source, so re-adding the hook under any name fails here. + const { kernel, engine } = await bootKernel([kernelTask]); + try { + expect((engine as any).hasHooksFor('beforeDelete', 'del_kernel_task')).toBe(false); + expect((engine as any).hasHooksFor('afterDelete', 'del_kernel_task')).toBe(false); + // The stamp builtins that DO survive are still bound — this asserts the + // absence of one hook, not of the builtin set. + expect((engine as any).hasHooksFor('beforeInsert', 'del_kernel_task')).toBe(true); + expect((engine as any).hasHooksFor('beforeUpdate', 'del_kernel_task')).toBe(true); + } finally { + if (kernel.getState() === 'running') await kernel.shutdown(); + } + }); + + it('a single-id delete on a hook-free object performs NO prior-row read', async () => { + // ⚠️ THE pin. This read count was 1 for every object on every kernel-hosted + // engine that has ever run, because `sys_fetch_previous_delete` held term 1 + // of the gate open — and then never used the row it forced the engine to + // fetch. + const { kernel, engine, reads } = await bootKernel([kernelTask]); + try { + const row: any = await engine.insert('del_kernel_task', { title: 'A', status: 'todo', done: false }); + const beforeFindOne = reads.findOneOn['del_kernel_task'] ?? 0; + const beforeFind = reads.findOn['del_kernel_task'] ?? 0; + + await engine.delete('del_kernel_task', { where: { id: row.id } }); + + expect((reads.findOneOn['del_kernel_task'] ?? 0) - beforeFindOne).toBe(0); + expect((reads.findOn['del_kernel_task'] ?? 0) - beforeFind).toBe(0); + expect(await engine.count('del_kernel_task', {})).toBe(0); + } finally { + if (kernel.getState() === 'running') await kernel.shutdown(); + } + }); + + it('an object WITH a real user `beforeDelete` still gets `previous` bound, from ONE read', async () => { + // The other half. The retirement must not cost a single binding: the row + // still arrives, and it arrives from the ENGINE's #5272/#6697 read — the + // count says there was only one, so nothing else supplied it. + const { kernel, engine, reads } = await bootKernel([kernelTask]); + try { + const seen: Array = []; + bindHooksToEngine(engine, [observer('user_pre', 'del_kernel_task', 'beforeDelete', seen)], { + packageId: 'app:del-kernel', + logger: { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }, + }); + + const row: any = await engine.insert('del_kernel_task', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_kernel_task'] ?? 0; + await engine.delete('del_kernel_task', { where: { id: row.id } }); + + expect((reads.findOneOn['del_kernel_task'] ?? 0) - before).toBe(1); + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: row.id, title: 'A', status: 'todo' }); + } finally { + if (kernel.getState() === 'running') await kernel.shutdown(); + } + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 5. The retired builtin's own shape, replayed — its guard cannot be true + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5929] a fetch-previous `beforeDelete` hook is now dead weight', () => { + it('its `!ctx.previous` guard short-circuits, so it issues no read of its own', async () => { + // Verbatim the retired builtin's shape — priority 5 (so it runs FIRST), + // `beforeDelete`, guard `if (input.id && !ctx.previous)`, fetching through + // `ctx.ql` exactly as `plugin.ts` reached for `this.ql`. Measured as a read + // count, because "the guard is false now" is the kind of claim that rots + // silently: if the engine ever stopped binding `previous` ahead of the + // before phase, this count would go to 2 and say so. + const supplied: Array = []; + const { engine, reads } = await boot([ + { + name: 'fetch_previous_delete_replay', object: '*', events: ['beforeDelete'], priority: 5, + handler: async (ctx: any) => { + if (ctx.input?.id && !ctx.previous) { + const existing = await ctx.ql.findOne(ctx.object, { + where: { id: ctx.input.id }, context: { isSystem: true }, + }); + if (existing) ctx.previous = existing; + } + }, + } as unknown as Hook, + observer('reads_previous', 'del_scope_a', 'beforeDelete', supplied), + ]); + + const row: any = await engine.insert('del_scope_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOneOn['del_scope_a'] ?? 0; + await engine.delete('del_scope_a', { where: { id: row.id } } as any); + + // The consumer saw the row… + expect(supplied).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + // …and exactly ONE read produced it: the engine's. The replayed builtin + // added none — which is the whole argument for deleting it rather than + // leaving it behind a guard that can no longer be true. + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); + }); + + it('the residual shape — engine read found nothing — leaves `previous` UNBOUND, not fabricated', async () => { + // The one shape in which the retired guard could still have been TRUE: the + // row is already gone, so the engine's read binds nothing. The builtin's + // read would have found nothing either (same row, same scope), so retiring + // it changes no binding here — and `bindPreImage` must still refuse to + // fabricate `{}`/`null` for a record nobody read (#4649/#4775). + const seen: Array = []; + const { engine } = await boot([observer('pre_a', 'del_scope_a', 'beforeDelete', seen)]); + + await engine.delete('del_scope_a', { where: { id: 'never_existed' } } as any); + + expect(seen).toEqual([undefined]); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 946bbeae3f..8428542beb 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -7273,6 +7273,48 @@ export class ObjectQL implements IObjectQLEngine { // the predicate path — see the pre-phase below. `delete()` was already // the right shape here; what changed is that the predicate branch grew // the same discipline the by-id branch has had since #5272. + // + // [#5929] The gate's THREE terms, unchanged by that card and enumerated + // here because it had to enumerate them to answer it: + // 1. `hasHooksFor('beforeDelete', object)` + // 2. `hasHooksFor('afterDelete', object)` + // 3. `getSummaryDescriptors(object).length > 0` + // No validation term, deliberately — see `needsPriorRecord` above. + // + // What #5929 changed is not this expression but what term 1 MEANS. Until + // then, objectql's own `ObjectQLPlugin` registered a builtin + // `sys_fetch_previous_delete` on `beforeDelete` with `object: '*'`, so on + // every kernel-hosted engine term 1 was true for every object and the + // per-object skip this gate exists to perform could never happen. The + // builtin's only remaining effect WAS holding this gate open: the read + // below binds `previous` before `beforeDelete` dispatches, so its own + // `!ctx.previous` guard was already unreachable. Retiring it (plugin.ts, + // ADR-0049 enforce-or-remove) makes term 1 an honest question. + // + // ⚠️ Honest is not the same as usually-false, and the difference is worth + // knowing before anyone reads a skip into a production trace. Every + // delete-phase hook below registers with NO `object` — global — so each + // holds term 1 or term 2 open for every object on a kernel that loads it: + // + // * `plugin-auth` identity-write-guard beforeDelete (filters by + // `isManaged(ctx.object)` inside the handler) + // * `plugin-sharing` record-share-cascade before+afterDelete (filters + // by `targets(objectName)` inside the handler) + // * `service-storage` file-reference-lifecycle before+afterDelete + // (filters by `activeFileFields(object)` inside) + // * `plugin-audit` captureBefore / writeAudit before+afterDelete, + // global MINUS `excludeObjects: AUDIT_EXCLUDED_OBJECTS` + // (#5860) — the one that narrows at the ENGINE face, + // so `hookMatchesObject` can subtract it and an + // excluded object really does skip this read. + // + // The first three are real handlers with real work, merely deciding their + // own applicability at dispatch time rather than at registration time; + // this gate answering "yes" for them is the gate WORKING, not a second + // instance of the #5929 defect. Narrowing any of them to the objects it + // actually serves — plugin-audit's `excludeObjects` face is the worked + // example — is what would convert them into skips, and that is each + // package's own card, not this one's. const deleteSchema = this._registry.getObject(object); const wantsPreImage = this.hasHooksFor('beforeDelete', object) || diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index 65f1287a58..fbceebc27d 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -917,51 +917,51 @@ export class ObjectQLPlugin implements Plugin { // This change removes one and makes the engine's the single producer; // `captureBefore`'s now-redundant read is the identity lane's follow-up. // - // ⚠️ `sys_fetch_previous_delete` below is in the SAME position now, and is - // deliberately left standing because retiring it is #5929's card, not a - // rider on this one. Recording the measurement so that card does not have - // to rediscover it: + // ⛔ RETIRED — `sys_fetch_previous_delete` (#5929, ADR-0049 + // enforce-or-remove). Do not reintroduce it. // - // `delete()` reads its pre-image when `wantsPreImage` is true, and that - // gate is `hasHooksFor('beforeDelete', object) || hasHooksFor( - // 'afterDelete', object) || summaries`. The builtin is itself a - // `beforeDelete` hook on `'*'`, so it makes the FIRST term true for every - // object — and then the engine's read binds `previous` before the builtin - // runs, so the builtin's own `!ctx.previous` guard is false and it issues - // no `findOne`. It is circular: the builtin's only remaining effect is to - // hold open the gate that makes it redundant. That circularity IS #5929 - // ("the delete-side per-object gate is always true"), and after this - // change its resolution is the same retirement performed above, with the - // same argument. + // #5846 left the measurement here rather than the hook, precisely so this + // retirement would not have to rediscover it. Quoted from the block above + // as it stood then, because it IS the argument: // - // The one shape where the guard can still be true — the engine read found - // nothing (the row is already gone) — is one where the builtin's read - // finds nothing either, so it changes no binding. - { - name: 'sys_fetch_previous_delete', - object: '*', - events: ['beforeDelete'], - priority: 5, - description: 'Auto-fetch the previous record for delete hooks', - handler: async (hookCtx: any) => { - if (hookCtx.input?.id && !hookCtx.previous) { - try { - const existing = await this.ql!.findOne(hookCtx.object, { - where: { id: hookCtx.input.id }, - context: { - positions: [], - permissions: [], - isSystem: true, - ...(hookCtx.transaction ? { transaction: hookCtx.transaction } : {}), - } as any, - }); - if (existing) hookCtx.previous = existing; - } catch (_e) { - // Non-fatal - } - } - }, - }, + // `delete()` reads its pre-image when `wantsPreImage` is true, and that + // gate is `hasHooksFor('beforeDelete', object) || hasHooksFor( + // 'afterDelete', object) || summaries`. The builtin is itself a + // `beforeDelete` hook on `'*'`, so it makes the FIRST term true for + // every object — and then the engine's read binds `previous` before the + // builtin runs, so the builtin's own `!ctx.previous` guard is false and + // it issues no `findOne`. It is circular: the builtin's only remaining + // effect is to hold open the gate that makes it redundant. + // + // Verified on this branch before removing anything, because a measurement + // recorded on one PR is a hypothesis on the next: the engine binds + // `previous` ahead of `beforeDelete` on BOTH delete shapes — by-id since + // #5272, per matched row since #6697 — so the guard is unreachable in + // production, and removing the hook changes no `previous` binding + // anywhere. The residual shape, `!hookCtx.previous` because the engine's + // own read found nothing, is one where this handler's read finds nothing + // either (same row, same transaction, same tenant scope): it binds + // nothing, and `bindPreImage` deliberately leaves `previous` UNBOUND + // rather than fabricating `{}` (#4649/#4775). + // + // What retiring it buys, and what it does NOT buy — both worth stating so + // the next reader does not over-read the result: + // * it makes `hasHooksFor('beforeDelete', object)` an honest question on + // an objectql-only kernel, so an object with no delete-side hook and + // no roll-up summary finally pays NO prior-row read on a by-id + // `delete()`. That skip could never happen while this hook stood. + // * it does not, on its own, make the gate false on a kernel that also + // loads plugin-auth / plugin-sharing / plugin-audit: each of those + // registers a delete-phase hook with no `object` (i.e. global), and + // they hold the same gate open for their own reasons. Those are real + // consumers with real handlers, not circular ones — the gate answering + // "yes" for them is the gate working. The enumeration lives in + // `engine.ts` beside `wantsPreImage`. + // + // The retired hook's own shape is replayed as an authored hook in + // `engine-delete-prior-read-scope.test.ts`, which measures that its guard + // short-circuits and it issues zero reads — so "the guard can no longer be + // true" stays a measurement instead of rotting into a claim. ]; if (typeof (this.ql as any).bindHooks === 'function') { @@ -979,7 +979,12 @@ export class ObjectQLPlugin implements Plugin { } } - ctx.logger.debug('Audit hooks registered via binder (created_by/updated_by, previousData)'); + // `previousData` used to be listed here as a third thing these builtins + // did. It is not one any more: both fetch-previous hooks are retired + // (#5846 update-side, #5929 delete-side) and `previous` is bound by the + // ENGINE on both write paths. A log line naming a producer that no longer + // exists is the cheapest way to send the next reader looking for it. + ctx.logger.debug('Audit hooks registered via binder (created_by/updated_by/created_at/updated_at/tenant_id stamping)'); } /** diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index c4e3ee1d11..9858e5d397 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -205,9 +205,11 @@ * the id alone — and reading it that way approved a sweep of every * administrator one legitimate-looking row at a time. `options.multi` is the * discriminator, and `resolveTargetIds` asks it FIRST. See that function. - * - `ctx.previous` (the engine's #5272 pre-image, and objectql's - * `sys_fetch_previous_delete` builtin — `object: '*'`, priority 5) is now - * bound on BOTH shapes: by-id since #5272, and per matched row since #5574. + * - `ctx.previous` (the engine's #5272 pre-image — its SOLE producer since + * #5929, which retired objectql's `sys_fetch_previous_delete` builtin + * (`object: '*'`, priority 5) once the engine's own earlier read made that + * hook's `!ctx.previous` guard unreachable) is now bound on BOTH shapes: + * by-id since #5272, and per matched row since #5574. * The guard still never consumes it, and the reason is sharper than before: * it needs the target IDS as a SET, and a `previous`-based implementation * would see exactly one row per dispatch — correct for a single write and From f26131eadebef63f903089e3be6b87c7e0e2fdf8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 18:23:18 +0000 Subject: [PATCH 2/2] test(objectql): keep the new delete-gate pins inside the erasure ratchets (#5929) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lint.yml gates caught the new test file on the first full run: * `check:slot-lookup` / `no-restricted-syntax` — `kernel.getService('objectql') as any` erased the slot's contract for every `engine.*` call in the kernel section, which is where the measurement lives. Typed as `getService('objectql')`. * `check:query-options-erasure` — three `count(obj, {} as any)` calls grew the test-surface count 263 → 266. The empty options bag is already on contract; the assertion was never needed. No assertion changed; the file still passes 16/16. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UNT8SWDEsDQp2TrmSBizKq --- .../src/engine-delete-prior-read-scope.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/objectql/src/engine-delete-prior-read-scope.test.ts b/packages/objectql/src/engine-delete-prior-read-scope.test.ts index c2e37ae319..139f5c9b8d 100644 --- a/packages/objectql/src/engine-delete-prior-read-scope.test.ts +++ b/packages/objectql/src/engine-delete-prior-read-scope.test.ts @@ -235,7 +235,7 @@ describe('[#5929] the delete-side prior-row demand is asked per object', () => { expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(0); // The row really went — a skipped read must not have skipped the write. - expect(await engine.count('del_scope_a', {} as any)).toBe(0); + expect(await engine.count('del_scope_a', {})).toBe(0); }); it('the object that DOES have the afterDelete hook pays it, and `previous` is the stored row', async () => { @@ -315,7 +315,7 @@ describe('[#5929] the delete-side prior-row demand is asked per object', () => { await engine.delete('del_scope_locked', { where: { id: row.id } } as any); expect((reads.findOneOn['del_scope_locked'] ?? 0) - before).toBe(0); - expect(await engine.count('del_scope_locked', {} as any)).toBe(0); + expect(await engine.count('del_scope_locked', {})).toBe(0); }); }); @@ -399,7 +399,7 @@ describe('[#5929] the predicate delete path asks the same per-object question', await engine.delete('del_scope_a', { multi: true, where: { status: 'stale' } } as any); expect((reads.findOn['del_scope_a'] ?? 0) - before).toBe(0); - expect(await engine.count('del_scope_a', {} as any)).toBe(0); + expect(await engine.count('del_scope_a', {})).toBe(0); }); it('…and reads it exactly once when the object IS hooked', async () => { @@ -436,7 +436,11 @@ describe('[#5929] on a KERNEL-hosted engine the per-object skip finally happens' } as any); await kernel.use(new ObjectQLPlugin()); await kernel.bootstrap(); - const engine = kernel.getService('objectql') as any; + // `getService` — the slot's real contract, not `as any`. The + // erasure would switch off checking for every `engine.*` call below while + // looking identical to code that keeps it (#4168/#4176/#4251), and the + // calls below are the measurement. + const engine = kernel.getService('objectql'); for (const o of objects) engine.registry.registerObject(o, 'test', 'test'); return { kernel, engine, reads: stub.reads }; }