From 6c7b0d83c84541cb665e6475d40a3340dc145529 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 15:44:25 +0000 Subject: [PATCH] fix(objectql): bind `hookContext.previous` on a single-record delete (#5272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HookContext.previous` is documented "for update/delete", and `update()` has bound it all along — `delete()` never did. `previous` was `undefined` in both `beforeDelete` and `afterDelete` for every single-record delete, so a legal delete-side condition (`previous.status == 'done'`) was unevaluable and, since #4775, rejected the whole operation — through the generic branch, which reads as an author typo when the engine was simply not binding the key. #5038 inverted the asymmetry: a predicate bulk delete already binds each doomed row's pre-image on its per-row `afterDelete`, so the single-record path was strictly worse than the bulk one — the opposite of the #4800/#4862 ruling that single and bulk mean the same thing. `delete()` now reads the doomed row once, before `beforeDelete` fires, and binds it for both phases. The gate is demand-driven like `update()`'s: a delete-side hook in either phase, or a roll-up summary aggregating this object. The roll-up path's own later pre-image fetch is folded into that same read, so an object with both pays one read, not two — and it is now the raw driver read `update()` already hands `recomputeSummaries`. A missing row leaves `previous` unbound rather than fabricating `{}`/`null`, and a `beforeDelete` hook that repoints or clears the target id re-reads or drops the binding so a stale pre-image never rides into `afterDelete`. The pin that hid this — `hook-condition-previous-scope.test.ts`'s "a delete-shaped context evaluates `previous` against the pre-image" — built `previous` by hand and asserted the wrapper read it, greenlighting behaviour the engine never produced. Replaced with end-to-end cases that drive a real `engine.delete()`: both phases receive the stored pre-image, a `previous.*` condition evaluates instead of rejecting, declared-field materialisation and no-leak hold on the delete side, the read happens exactly once for the two phases, an object with no delete-side hook pays no read at all, and a missing row leaves `previous` unbound. Fixes #5272 --- .changeset/single-delete-binds-previous.md | 51 +++++ packages/objectql/src/engine.ts | 100 ++++++++- .../src/hook-condition-previous-scope.test.ts | 211 ++++++++++++++++-- 3 files changed, 336 insertions(+), 26 deletions(-) create mode 100644 .changeset/single-delete-binds-previous.md diff --git a/.changeset/single-delete-binds-previous.md b/.changeset/single-delete-binds-previous.md new file mode 100644 index 0000000000..80e5b5c740 --- /dev/null +++ b/.changeset/single-delete-binds-previous.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a single-record `delete()` binds `hookContext.previous` — the pre-image the contract has always promised (#5272) + +`HookContext.previous` is documented in the spec as *"the state of the record +BEFORE the operation (**for update/delete**)"*, and `update()` has bound it all +along. `delete()` never did. `previous` was `undefined` in **both** +`beforeDelete` and `afterDelete`, for every single-record delete, on every +object. + +That is not a cosmetic gap. Since #4775 a condition that cannot be evaluated +**fails the operation**, so a legal, contract-shaped delete-side hook: + +```ts +{ events: ['afterDelete'], condition: "previous.status == 'done'" } +``` + +rejected *every* single-record delete of that object — and reported it through +the generic branch (`Unknown variable: previous`), which reads like the author +misspelled a key. The key was fine; the engine never bound it. Same shape as +#5037: a platform gap surfacing as the author's mistake. + +**Why now.** #5038 made a predicate bulk delete dispatch `afterDelete` once per +matched row, each carrying that row's own pre-image. The single-record path +still bound nothing, so it became strictly *worse* than the bulk path — the +exact inversion the #4800/#4862 ruling ("an author writes the hook once; single +and bulk mean the same thing") exists to prevent. + +**What changed.** `delete()` now takes the doomed row's pre-image once, before +`beforeDelete` fires, and binds it to `hookContext.previous` for both phases — +so hook `condition`s, record-change flow triggers and delete-audit handlers all +see the deleted row. The read is demand-driven, exactly like `update()`'s: it +happens only when the object has a delete-side hook (either phase) or a roll-up +summary aggregating it. An object with neither pays nothing, and an object with +both phases pays **one** read, not two — the roll-up path's own separate +pre-image fetch has been folded into this one, and is now the same raw driver +read `update()` already feeds the summary recompute. + +Nothing is fabricated: if the row is not there, `previous` stays **unbound** +rather than becoming `{}`/`null`, so a condition reading it still faults loudly +instead of answering for a record nobody read (#4649/#4775). The batch dispatch +of a predicate delete still carries no `previous` — it stands for N rows — and +its per-row `afterDelete` contexts are unchanged. + +Upgrade impact: a delete-side hook whose `condition` reads `previous` starts +evaluating instead of rejecting the write, and delete-side handlers start +receiving `ctx.previous`. If you worked around the gap by testing +`ctx.previous == null` to detect "this is a delete", that test now answers +differently — read `ctx.event` instead. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index a842ad70f4..d6047fd66e 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5532,7 +5532,88 @@ export class ObjectQL implements IObjectQLEngine { transaction: opCtx.context?.transaction, ql: this }; + + // [#5272] The pre-image of the row this delete is about to remove. + // + // `HookContext.previous` is documented — in the spec, since it was + // written — as "the state of the record BEFORE the operation (for + // update/delete)", and `update()` has bound it all along. `delete()` + // never did: `previous` was `undefined` in `beforeDelete` AND in + // `afterDelete`, so a legal, contract-shaped delete-side transition + // condition (`previous.status == "done"`) was unevaluable — and since + // #4775 unevaluable REJECTS the operation. Worse, it was reported + // through the generic branch, which reads as "you misspelled a key" + // when the key was fine and the engine simply never bound it (#5037's + // shape, one path over). + // + // #5038 made the asymmetry visible from the other side: a predicate + // bulk delete already binds each doomed row's own pre-image on its + // per-row `afterDelete`, so the SINGLE-record path was strictly worse + // than the bulk one — the exact inversion #4800/#4862 ruled against. + // + // Demand-driven, like `update()`'s `priorRecord`: read only when + // something on this object actually consumes it — + // * a delete-side hook, EITHER phase (its `condition` may read + // `previous`; its handler — plugin-audit, the record-change + // trigger — reads `ctx.previous` directly); + // * a roll-up summary aggregating this object, which needs the + // doomed row's FK value to find the parent to recompute. + // Those two used to be separate reads at separate times (the summary + // one fetched only after `beforeDelete` had run); they are ONE read + // now — and a RAW driver read, which is exactly what `update()` + // already hands `recomputeSummaries` as its `previous` argument, so + // the two write paths now agree on what a pre-image is. + // + // `needsPriorRecord(schema)` is deliberately NOT part of this gate + // even though `update()`'s twin carries it: object validation rules + // are evaluated on insert/update only — `delete()` evaluates none — + // so including it would buy a read with no reader. + // + // Read BEFORE `beforeDelete` fires. A delete's `before` phase is the + // one that has nothing else to look at (its `input` carries an id and + // no data), and the pre-image has to be taken before the row is gone + // either way, so a single read serves both phases. + const deleteSchema = this._registry.getObject(object); + const wantsPreImage = + this.hasHooksFor('beforeDelete', object) || + this.hasHooksFor('afterDelete', object) || + this.getSummaryDescriptors(object).length > 0; + // `buildDriverOptions` is what carries the open transaction and the + // tenant scope onto a raw driver read. Skipping it here would read + // outside this write's transaction and across the tenant boundary — + // `update()`'s prior read passes the same bag for the same reason. + const readPreImage = async (targetId: unknown): Promise | null> => { + const preAst: QueryAST = { object, where: { id: targetId }, limit: 1 }; + const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); + return (await driver.findOne(object, preAst, preOpts)) as Record | null; + }; + const bindPreImage = (row: Record | null): void => { + // Never fabricate: a row that is not there leaves `previous` UNBOUND + // rather than `{}`/`null`, so a condition reading it faults loudly + // instead of answering for a record nobody read (#4649/#4775). + hookContext.previous = row ? (coerceBooleanFields(deleteSchema as any, row as any) as any) : undefined; + }; + let priorRecord: Record | null = null; + if (id && wantsPreImage) { + priorRecord = await readPreImage(id); + bindPreImage(priorRecord); + } + await this.triggerHooks('beforeDelete', hookContext); + + // A `beforeDelete` hook may repoint the target id, or clear it (which + // #4550's re-asked dispatch verdict below already accounts for). The + // pre-image bound above describes the OLD id, so it must not ride into + // `afterDelete` — or into the summary recompute — as though it + // described the new target. A cleared id falls through to the predicate + // branch, whose batch-scoped dispatch must carry no single row's + // pre-image at all (`hook-wrappers` diagnoses that dispatch by the + // absence of both). + if (wantsPreImage && hookContext.input.id !== id) { + priorRecord = hookContext.input.id ? await readPreImage(hookContext.input.id) : null; + bindPreImage(priorRecord); + } + hookContext.input.options = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); try { @@ -5545,15 +5626,6 @@ export class ObjectQL implements IObjectQLEngine { // `record-after-delete` flow must see each deleted row rather than // one context that names none of them. let bulkPerRowRows: Record[] | null = null; - // Capture the row's FK values BEFORE deletion so roll-up summaries can - // recompute the (now-orphaned) parent. Only when a summary aggregates - // this object — avoids an extra read on every delete. - let summaryPrev: any = null; - if (hookContext.input.id && this.getSummaryDescriptors(object).length > 0) { - try { - summaryPrev = await this.findOne(object, { where: { id: hookContext.input.id }, context: opCtx.context } as any); - } catch { /* best-effort */ } - } if (hookContext.input.id) { // Honor referential delete behavior (cascade/set_null/restrict) // for relations pointing at this record before removing it. @@ -5607,9 +5679,13 @@ export class ObjectQL implements IObjectQLEngine { await this.triggerHooks('afterDelete', hookContext); } - // Roll-up: recompute the parent summary now that the child is gone. - const summaryFailures = summaryPrev - ? await this.recomputeSummaries(object, null, summaryPrev, opCtx.context) + // Roll-up: recompute the parent summary now that the child is gone, + // from the row's FK values captured BEFORE deletion. [#5272] That + // capture is now the same single pre-image read `previous` rides on + // (it used to be its own later `findOne`), which is also what + // `update()` passes here. + const summaryFailures = priorRecord + ? await this.recomputeSummaries(object, null, priorRecord, opCtx.context) : []; // Same split as update(): per-record `data.record.deleted` (#4626), diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index 2295f1a595..0c39a684ec 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -240,20 +240,16 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { expect(conditionWarnings()).toEqual([]); }); - it('a delete-shaped context evaluates `previous` against the pre-image', async () => { - const calls: string[] = []; - const { logger, conditionWarnings } = captureLogger(); - const wrapped = wrapDeclarativeHook( - makeHook('previous.done != true', ['beforeDelete']), - (async () => { calls.push('ran'); }) as any, - { logger }, - ); - - await wrapped(makeCtx({ event: 'beforeDelete', input: { id: 't1', options: {} } } as any)); - - expect(calls).toEqual(['ran']); - expect(conditionWarnings()).toEqual([]); - }); + // [#5272] The delete-shaped case that used to live here hand-built + // `previous` on the context and asserted the wrapper read it. It passed + // against an engine that never produced one — `delete()` assigned + // `hookContext.previous` nowhere, so the pin was a green light for behaviour + // that did not exist, and the real failure (every single-record delete with + // a `previous.*` condition rejected under #4775) sat behind it. Its + // replacement drives a real `engine.delete()` end to end — see + // '[#5272] a single-record delete binds `previous` through the real engine' + // at the bottom of this file. Nothing about the wrapper needed fixing; the + // producer did, so that is where the test now looks. it('a context with no engine still binds `previous` (merge only, no materialisation)', async () => { const calls: string[] = []; @@ -503,3 +499,190 @@ describe('[#4784] a condition that never mentions `previous` costs zero extra fe expect(prevCost).toBe(plainCost); }); }); + +/* ──────────────────────────────────────────────────────────────────────────── + * [#5272] The DELETE side of the same contract, through a real engine. + * + * `HookContext.previous` is documented "for update/delete", and #5038 made a + * predicate bulk delete bind each doomed row's pre-image on its per-row + * `afterDelete`. The single-record path bound nothing at all, so it was + * strictly worse than the bulk one and every delete-side `previous.*` + * condition was rejected by #4775's fail-loud — through the generic branch, + * which reads like an author typo. + * + * Every case below goes insert → real `engine.delete()` → assert what the hook + * was handed. A hand-built context cannot answer any of these questions: it + * asserts what the test author already wrote down. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5272] a single-record delete binds `previous` through the real engine', () => { + async function bootDelete(hooks: Hook[]) { + const engine = new ObjectQL(); + const mem = makeMemoryDriver(); + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + const warn = vi.fn(); + bindHooksToEngine(engine, hooks, { + packageId: 'app:showcase', + logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + }); + return { + engine, + reads: mem.reads, + conditionWarnings: () => + warn.mock.calls.filter(([msg]) => String(msg).includes('condition evaluation failed')), + }; + } + + const observer = (event: string, sink: Array | undefined>): Hook => ({ + name: `observe_${event}`, + object: 'hook_task', + events: [event], + priority: 90, + handler: (ctx: any) => { sink.push(ctx.previous); }, + } as unknown as Hook); + + it('hands `beforeDelete` the stored pre-image', async () => { + const seen: Array | undefined> = []; + const { engine } = await bootDelete([observer('beforeDelete', seen)]); + + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'done', done: true }); + await engine.delete('hook_task', { where: { id: row.id } } as any); + + expect(seen).toHaveLength(1); + // The row as the database held it — not `undefined`, and not the bare + // `{ id }` the delete's `input` carries. + expect(seen[0]).toEqual({ id: row.id, title: 'Ship it', status: 'done', done: true }); + }); + + it('hands `afterDelete` the same pre-image — by then the row is gone', async () => { + const seen: Array | undefined> = []; + const { engine } = await bootDelete([observer('afterDelete', seen)]); + + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'done', done: true }); + await engine.delete('hook_task', { where: { id: row.id } } as any); + + // The pre-image is taken BEFORE the delete precisely because this is the + // only moment it exists: the row is unreadable now. + expect(await engine.findOne('hook_task', { where: { id: row.id } })).toBeFalsy(); + expect(seen).toHaveLength(1); + expect(seen[0]).toEqual({ id: row.id, title: 'Ship it', status: 'done', done: true }); + }); + + it('evaluates a `previous.*` delete-side condition instead of rejecting the delete (#4775)', async () => { + // The issue's own example: a legal, contract-shaped transition hook. Before + // the fix `previous` was unbound on every single-record delete, so this + // condition was unevaluable and #4775 failed the whole operation — with the + // generic `Unknown variable: previous`, which reads as a misspelling. + const audited: string[] = []; + const { engine, conditionWarnings } = await bootDelete([{ + name: 'audit_completed_task_deletion', + object: 'hook_task', + events: ['afterDelete'], + priority: 90, + condition: "previous.status == 'done'", + handler: (ctx: any) => { audited.push(String(ctx.input?.id ?? '?')); }, + } as unknown as Hook]); + + const done: any = await engine.insert('hook_task', { title: 'Shipped', status: 'done', done: true }); + const open: any = await engine.insert('hook_task', { title: 'Draft', status: 'todo', done: false }); + + // Neither delete is rejected, and the condition SELECTS: it fires for the + // completed task and not for the open one. + await expect(engine.delete('hook_task', { where: { id: done.id } } as any)).resolves.toBeDefined(); + await expect(engine.delete('hook_task', { where: { id: open.id } } as any)).resolves.toBeDefined(); + + expect(audited).toEqual([done.id]); + expect(conditionWarnings()).toEqual([]); + }); + + it('is TOTAL over declared fields on a delete too — an unwritten column reads as null', async () => { + // Same materialisation the update side gets: `done` was never written, so + // the driver's row has no such key. `previous.done` must read as the + // materialised null rather than aborting the condition. + const audited: string[] = []; + const { engine, conditionWarnings } = await bootDelete([{ + name: 'audit_incomplete_deletion', + object: 'hook_task', + events: ['beforeDelete'], + priority: 90, + condition: 'previous.done != true', + handler: (ctx: any) => { audited.push(String(ctx.input?.id ?? '?')); }, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'Untouched', status: 'todo' }); + await engine.delete('hook_task', { where: { id: row.id } } as any); + + expect(audited).toEqual([row.id]); + expect(conditionWarnings()).toEqual([]); + }); + + it('does not leak materialised nulls into what the delete hook observes', async () => { + const seen: Array | undefined> = []; + const { engine } = await bootDelete([{ + name: 'observe_previous_on_delete', + object: 'hook_task', + events: ['afterDelete'], + priority: 90, + condition: 'previous.archived == null', + handler: (ctx: any) => { seen.push(ctx.previous); }, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'todo' }); + await engine.delete('hook_task', { where: { id: row.id } } as any); + + expect(seen).toHaveLength(1); + // `archived` is declared and was materialised for the condition; the + // engine's own pre-image must not have gained a column the row never had. + expect(Object.keys(seen[0] as object).sort()).toEqual(['id', 'status', 'title']); + }); + + it('reads the pre-image ONCE for both phases', async () => { + // The cost guardrail: `beforeDelete` and `afterDelete` share one read, and + // it is the SAME read the roll-up summary path used to make separately. + const before: Array | undefined> = []; + const after: Array | undefined> = []; + const { engine, reads } = await bootDelete([ + observer('beforeDelete', before), + observer('afterDelete', after), + ]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const baseline = reads.findOne; + await engine.delete('hook_task', { where: { id: row.id } } as any); + + expect(reads.findOne - baseline).toBe(1); + expect(before[0]).toEqual(after[0]); + }); + + it('reads nothing at all when the object has no delete-side hook', async () => { + // Demand-driven, exactly like update()'s prior-row gate: an object nobody + // observes on delete pays for no pre-image. + const { engine, reads } = await bootDelete([{ + name: 'update_only_guard', + object: 'hook_task', + events: ['afterUpdate'], + priority: 100, + condition: 'record.status == "todo"', + handler: () => {}, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const baseline = reads.findOne; + await engine.delete('hook_task', { where: { id: row.id } } as any); + + expect(reads.findOne - baseline).toBe(0); + }); + + it('leaves `previous` UNBOUND when the row is not there — nothing is fabricated', async () => { + const seen: Array | undefined> = []; + const { engine } = await bootDelete([observer('beforeDelete', seen)]); + + await engine.delete('hook_task', { where: { id: 'never_existed' } } as any); + + // `{}` or `null` here would let `previous.status == "done"` answer for a + // record nobody read. Absent stays absent (#4649/#4775). + expect(seen).toEqual([undefined]); + }); +});