diff --git a/.changeset/action-body-write-not-found-gate.md b/.changeset/action-body-write-not-found-gate.md new file mode 100644 index 0000000000..c4c4f4fa84 --- /dev/null +++ b/.changeset/action-body-write-not-found-gate.md @@ -0,0 +1,93 @@ +--- +"@objectstack/objectql": patch +"@objectstack/core": patch +"@objectstack/metadata-protocol": patch +"@objectstack/runtime": patch +"@objectstack/plugin-audit": patch +"@objectstack/plugin-auth": patch +--- + +fix(objectql): a by-id `update()`/`delete()` against a nonexistent record answers 404 `RECORD_NOT_FOUND` instead of a 400 from further down the pipeline (#7867) + +Nothing on the action-body write path ever asked whether the target row existed. +`ctx.api.object(name).update({ id, … })` reached `ObjectQL.update()`'s by-id +branch through `buildSandboxApi` → `ObjectRepository`, and that branch had **no +existence gate at all**: `engine.update()` on a ghost id was a silent no-op that +resolved `null`, so the write ran on into validation, the driver and the hook +chain and died on whichever complained first. + +**Which one it died on varied with the object's declarations**, which is why the +defect read as several unrelated bugs: + +- a **hooked** object → `400` `HookConditionError`, from an `afterUpdate` + condition reading `previous` on a row nobody read; +- an **unhooked** object → `400` `VALIDATION_FAILED` "X is required", because + with no prior row a PATCH is validated as if it were a whole record. + +The 400 class varied; the missing 404 was the constant. Measured on one showcase +stack, same id, same object, same second: `POST /actions/showcase_task/ +showcase_mark_done/` answered 400 while `PATCH /data/showcase_task/ +` answered 404. Both answer **404 `RECORD_NOT_FOUND`** now. + +`delete()` had the same shape and was the worse of the two: with no gate it +reported success for a row that was never there, so a typo'd id, an +already-deleted row and a real deletion were indistinguishable. + +**This is not a `previous`-binding bug.** `if (priorRecord) hookContext.previous += …` is correct and is untouched — ADR-0058 Addendum II / #4649 require that an +absent row leave `previous` UNBOUND rather than fabricated. It was behaving +correctly on a path that should never have been entered, so the fix removes the +producer rather than specializing what it produced. + +**Where the gate went, and why there.** At the engine, in the by-id branches of +`update()` and `delete()` — the one point all three action-body write faces +funnel through (`ctx.api.object()`, its context-less repo-facade fallback, and +`ctx.engine.update()`). A repository-level gate would have closed one of the +three and made `ql.update(o, { id })` and `ctx.api.object(o).update({ id })` +answer one ghost id two different ways. Two sibling paths already gated +correctly — `protocol.updateData`/`deleteData` (#4435) and `callData`'s ObjectQL +fallback (#5138) — and all three now throw the **same** `recordNotFoundError`, +which moved to `@objectstack/core` so the engine can reach it without importing +`@objectstack/metadata-protocol` (forbidden in the `/core` closure by ADR-0076 +D2's boundary ratchet). `@objectstack/metadata-protocol` re-exports it unchanged. + +Existence is asked with a pre-write read, never off the write's own result: +`IDataDriver.update` declares no not-found signal, and the engine's post-write +readback is `null` for a second reason (a write that moves the row out of the +caller's row scope), so reading either would answer 404 to a write that landed. + +**Behaviour change worth knowing about — the by-id prior-row read is now +unconditional.** #5284 (update) and #5929 (delete) had narrowed it to "does +anything CONSUME the prior row?", skipping the read for objects with no hook, no +prior-reading validation rule and no roll-up. Existence is a consumer that +demand list never enumerated and the one consumer every by-id write has, and no +cheaper question answers it — so the skip and the gate are mutually exclusive. +The measured cost is small: #5929's own record enumerates the global hook +registrants (plugin-sharing, service-storage, plugin-auth, plugin-audit), so on +any kernel that loads them the demand was already true for every object and the +narrowing skipped nothing. The read is genuinely new only for a bare +`@objectstack/objectql/core` embedder — which is buying a 404 it did not have. + +Three read-count pins measured the old skip and now measure the read, each +recording what changed and why at its own site: #5284's and #5929's in +`packages/objectql`, and #5860's `sys_job_queue` case in `@objectstack/plugin-audit`. +The DISPATCH half all three are actually about — the per-object `hasHooksFor` +question, the `excludeObjects` subtraction, and the retired +`sys_fetch_previous_*` builtins — is untouched and still pinned. + +One further case encoded the old silent no-op as correct: `@objectstack/plugin-auth`'s +#5941 last-admin-guard test deleted a `sys_account` id that was never seeded and +asserted it RESOLVED, to show the guard does not write-guard that object. It now +deletes a REAL row — which states the same thing more strongly — and separately +pins that a ghost id there is refused by the ENGINE rather than by the guard. + +**Scope.** By-id only. A `multi: true` predicate write matching zero rows still +resolves "0 rows affected" — the same line both sibling paths draw. + +`@objectstack/runtime`: the sandbox error passthrough now also carries `status` +alongside `code` and `fields`, so an error that names its own HTTP status keeps +it across the QuickJS boundary. Without it the action surface answered the right +diagnosis at the wrong status (`{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }`); +`domains/actions.ts` already honoured `.status` first — the number simply never +arrived. A permission refusal thrown inside a body likewise keeps its 403 now +instead of flattening to 400. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c02358876d..eed5e45e71 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -44,6 +44,12 @@ export * from './utils/migration-journal.js'; // Export the runtime filter-placeholder resolver (framework#3582) export * from './utils/filter-tokens.js'; +// Export the shared single-record 404 (#4435/#5138, moved down here in #7867) — +// the one `RECORD_NOT_FOUND` envelope `protocol.updateData`/`deleteData`, +// `callData`'s ObjectQL fallback and the engine's own by-id write gate answer +// with. `@objectstack/metadata-protocol` re-exports it from its original home. +export * from './utils/record-not-found.js'; + // Export in-memory fallbacks for core-criticality services export * from './fallbacks/index.js'; diff --git a/packages/core/src/utils/record-not-found.ts b/packages/core/src/utils/record-not-found.ts new file mode 100644 index 0000000000..510ef19898 --- /dev/null +++ b/packages/core/src/utils/record-not-found.ts @@ -0,0 +1,64 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4435] The 404 a single-record operation answers when the id names no row. + * + * Extracted so the READ and the two WRITE paths cannot disagree about it. They + * did: `getData` answered `404 RECORD_NOT_FOUND` while `updateData` returned + * `200 { record: null }` and `deleteData` returned `200 { success: true }` for + * any string in the path — so a typo'd id, an already-deleted row and a real + * deletion were indistinguishable, and a client PATCHing a concurrently deleted + * record was told its write had landed. + * + * That is the same silent-no-op shape the v17 train removed everywhere else + * this window (#4240/#4303/#4315 refuse missing fields, #4169 refuses unknown + * params, #4190 stopped dropping filters) — a write that touched zero rows + * reporting 200 is that shape one level up, on the verb where it costs the + * most. + * + * [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer + * out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL + * FALLBACK, and the fallback had reinvented this fact three incompatible ways + * (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` → + * no check at all ⇒ `200 { deleted: true }` for a row that never existed). It + * now calls THIS function, so the two paths behind one `callData` answer a + * missing id identically — which is the only reason a caller may stop caring + * which of them served it. Re-spelling the envelope there would have been a + * second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo + * has. + * + * ── [#7867] Why it lives in `@objectstack/core` and not where it was written ── + * + * Because the THIRD path that needed it could not reach the second one. An + * action body's `ctx.api.object(name).update({ id, … })` traverses neither + * `protocol.updateData` nor `callData`: it reaches `ObjectQL.update()`'s by-id + * branch directly, which had no existence gate at all, so a ghost id was a + * silent no-op that then died on whatever the pipeline complained about first + * (a `HookConditionError` 400 on a hooked object, a required-field + * `VALIDATION_FAILED` 400 on an unhooked one — the 400 class varied with the + * object's declarations; the missing 404 was the constant). + * + * The gate for that path belongs in the engine, and `packages/objectql` cannot + * import `@objectstack/metadata-protocol` where this function was written: + * ADR-0076 D2's boundary ratchet (`core-boundary.ratchet.test.ts`) forbids the + * whole `@objectstack/objectql/core` closure — `engine.ts` included — from + * pulling that package in. So the choice was a FOURTH spelling of the envelope + * or one home both layers already depend on. #5138's own sentence rules the + * first out, so this is the second: the factory moved down to the lowest + * package the three producers share, and `@objectstack/metadata-protocol` + * re-exports it unchanged for every existing importer. + * + * This is the same move `engineCanRollBack` made for the same reason — a fact + * two layers must agree on lives in the layer beneath both, not in a copy each. + */ +export function recordNotFoundError(object: string, id: string | number): Error { + const err = new Error(`Record ${id} not found in ${object}`) as Error & { + code?: string; + status?: number; + object?: string; + }; + err.code = 'RECORD_NOT_FOUND'; + err.status = 404; + err.object = object; + return err; +} diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 77d9f58ccc..402e04dcb7 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3,7 +3,7 @@ import type { DataProtocol, MetadataProtocol, PackageProtocol, } from '@objectstack/spec/api'; -import { IDataEngine, engineCanRollBack } from '@objectstack/core'; +import { IDataEngine, engineCanRollBack, recordNotFoundError } from '@objectstack/core'; import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/types'; // [#6285] ADR-0105 D1's authority on "does this deployment wall organizations?". // `resolveMultiOrgEnabled()` is DEMOTED and its own doc comment says answering @@ -640,43 +640,22 @@ export function zodIssuesToMetadataIssues(issues: unknown): MetadataIssueEntry[] } /** - * [#4435] The 404 a single-record operation answers when the id names no row. - * - * Extracted so the READ and the two WRITE paths cannot disagree about it. They - * did: `getData` answered `404 RECORD_NOT_FOUND` while `updateData` returned - * `200 { record: null }` and `deleteData` returned `200 { success: true }` for - * any string in the path — so a typo'd id, an already-deleted row and a real - * deletion were indistinguishable, and a client PATCHing a concurrently deleted - * record was told its write had landed. - * - * That is the same silent-no-op shape the v17 train removed everywhere else - * this window (#4240/#4303/#4315 refuse missing fields, #4169 refuses unknown - * params, #4190 stopped dropping filters) — a write that touched zero rows - * reporting 200 is that shape one level up, on the verb where it costs the - * most. - * - * [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer - * out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL - * FALLBACK, and the fallback had reinvented this fact three incompatible ways - * (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` → - * no check at all ⇒ `200 { deleted: true }` for a row that never existed). It - * now calls THIS function, so the two paths behind one `callData` answer a - * missing id identically — which is the only reason a caller may stop caring - * which of them served it. Re-spelling the envelope there would have been a - * second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo - * has. + * [#4435/#5138] The 404 a single-record operation answers when the id names no + * row — the repo's ONE `RECORD_NOT_FOUND` envelope. + * + * [#7867] The body moved to `@objectstack/core` + * (`utils/record-not-found.ts` — full provenance lives there); this is a + * re-export, so every existing importer of + * `@objectstack/metadata-protocol`'s `recordNotFoundError` is unchanged and + * the three producers still share one function object. + * + * ⛔ Do not re-declare it here. It moved because a THIRD producer needed it and + * could not reach this package: `ObjectQL.update()`/`delete()`'s by-id gate + * lives in `packages/objectql`, whose `/core` entry closure is forbidden by + * ADR-0076 D2's boundary ratchet from importing `@objectstack/metadata-protocol` + * at all. A local copy here would be the second spelling #5138 ruled out. */ -export function recordNotFoundError(object: string, id: string | number): Error { - const err = new Error(`Record ${id} not found in ${object}`) as Error & { - code?: string; - status?: number; - object?: string; - }; - err.code = 'RECORD_NOT_FOUND'; - err.status = 404; - err.object = object; - return err; -} +export { recordNotFoundError }; /** * A 400 for a `$filter` ARRAY that looks like a filter AST but is not one. diff --git a/packages/objectql/src/engine-delete-dispatch.test.ts b/packages/objectql/src/engine-delete-dispatch.test.ts index 75b0621838..ee68f6d565 100644 --- a/packages/objectql/src/engine-delete-dispatch.test.ts +++ b/packages/objectql/src/engine-delete-dispatch.test.ts @@ -35,7 +35,18 @@ function makeRecordingDriver() { supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find() { return []; }, - async findOne() { return null; }, + // [#7867] Answers the row the by-id branch's not-found gate asks for. + // This used to be `return null`, which — now that a by-id update/delete + // refuses a ghost id with `RECORD_NOT_FOUND` — would make every by-id case + // in this file die at the gate and never reach the driver: a DOUBLE looser + // than the producer, hiding the very behaviour the file exists to observe + // (#4434/#4550's shape). It echoes back whatever id it was asked for, so it + // stays agnostic about the dispatch and can never make a `reject` case look + // like a `by-id` one. + async findOne(_o: string, ast: any) { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, title: 'stored' }; + }, async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, async update(_o: string, id: string, data: Record) { return { id, ...data }; }, async delete(_o: string, id: string) { calls.push({ fn: 'delete', arg: id }); return true; }, 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 139f5c9b8d..ebb3ac2fb7 100644 --- a/packages/objectql/src/engine-delete-prior-read-scope.test.ts +++ b/packages/objectql/src/engine-delete-prior-read-scope.test.ts @@ -41,6 +41,33 @@ * 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`. + * + * ## ⚠️ AMENDED BY #7867 — the by-id READ-SKIP half is retired + * + * This card's subject splits in two, and only one half survives: + * + * - **The DISPATCH half stands, untouched.** `hasHooksFor` still answers per + * object, `hookMatchesObject` still subtracts `excludeObjects`, an excluded + * object still dispatches nothing, and `sys_fetch_previous_delete` is still + * retired and must not come back. Those are what #5929 was actually about, + * and every case pinning them is unchanged below. + * + * - **The by-id READ-SKIP half is gone.** A by-id delete now reads its + * pre-image UNCONDITIONALLY, because #7867 added the not-found gate the + * path never had — a delete against an id naming no row must answer + * `RECORD_NOT_FOUND` instead of reporting success for a row that was never + * there (#5138's record names `delete` as the worst of the three when this + * gate was missing). Existence is a consumer the three-term demand never + * enumerated and the one consumer EVERY by-id delete has, and no cheaper + * question answers it — so the skip and the gate are mutually exclusive. + * + * The cases below that measured the SKIP now measure the READ, each saying so + * at its own site. What that costs is small for the reason section 4's own + * prose gives from the other side: on any kernel loading the global delete-side + * registrants (plugin-sharing, service-storage, plugin-auth), term 1 or 2 was + * true for every object anyway. The PREDICATE path (section 3) keeps its gate + * in full — a bulk delete matching zero rows is legitimately "0 rows affected", + * not a missing record. */ import { describe, it, expect } from 'vitest'; @@ -226,18 +253,48 @@ const observer = ( * ──────────────────────────────────────────────────────────────────────────── */ 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', [])]); + it('[#7867] object A pays the prior read too — the by-id read is no longer the gate\'s to skip', async () => { + // ⚠️ Asserted 0 until #7867. The DISPATCH question this file is about is + // unchanged (nothing fires on A), but the by-id READ is now unconditional: + // the not-found gate has to know whether the row is there, and that is the + // one demand no hook registration can express. The dispatch half is pinned + // by the `previous`-observing cases below, which are untouched. + const seen: Array = []; + const { engine, reads } = await boot([observer('audits_b', 'del_scope_b', 'afterDelete', 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(0); - // The row really went — a skipped read must not have skipped the write. + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); + // …and B's hook still did not fire for A's delete — the per-object dispatch + // question, which is what #5929 was actually about, is intact. + expect(seen).toEqual([]); + // The row really went — the added read must not have cost the write. expect(await engine.count('del_scope_a', {})).toBe(0); }); + it('[#7867] a by-id delete against an id that names no row is refused — RECORD_NOT_FOUND', async () => { + // What the unconditional read buys. #5138's own record: with no gate "the + // delete ran and the answer was 200 { deleted: true } for any string in the + // path, so a typo'd id, an already-deleted row and a real deletion were + // indistinguishable". That was still live on this path — the one an action + // body's `.delete()` takes — until this card. + const fired: Array = []; + const { engine } = await boot([observer('pre_a', 'del_scope_a', 'beforeDelete', fired)]); + + const err: any = await engine + .delete('del_scope_a', { where: { id: 'never_existed' } } as any) + .then(() => null, (e) => e); + + expect(err, 'a ghost id must be refused, not reported as a deletion').not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + // Refused before the before phase — no handler runs for a row that is not + // there, and no cascade touches a dependent of a parent that never existed. + expect(fired).toEqual([]); + }); + 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)]); @@ -303,18 +360,22 @@ describe('[#5929] the delete-side prior-row demand is asked per object', () => { 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. + it('`needsPriorRecord` is still NOT a term — a readonlyWhen object owes the read for EXISTENCE, not for rules', async () => { + // The asymmetry with `update()`'s twin gate survives #7867 and is worth + // keeping stated, because the read count no longer distinguishes it: this + // object now pays 1 like every other by-id delete. What is pinned here is + // the REASON — `delete()` evaluates no validation rules and no field + // predicates, so `needsPriorRecord` would still buy nothing if it were + // added; the read is owed to the not-found gate alone. Adding the term back + // would be adding a second, redundant justification for a read that already + // happens, which is how a gate acquires a term nobody can retire. 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((reads.findOneOn['del_scope_locked'] ?? 0) - before).toBe(1); expect(await engine.count('del_scope_locked', {})).toBe(0); }); }); @@ -343,7 +404,13 @@ describe('[#5929 / #5860] the gate honours `excludeObjects` on both delete phase }); }; - it('an EXCLUDED object skips the prior read, and the hook does not fire on it', async () => { + it('[#7867] an EXCLUDED object does not DISPATCH — it still pays the by-id existence read', async () => { + // ⚠️ The read half of this case asserted 0 until #7867; the DISPATCH half + // is the one #5860/#5929 are about and it is unchanged. They no longer move + // together on the by-id path, and that is the point rather than a + // regression: `hookMatchesObject` still decides who fires, but it no longer + // decides whether the engine looks — the not-found gate does, for every + // by-id delete, excluded or not. const fired: unknown[] = []; const { engine, reads } = await boot(); registerGlobalDeleteHook(engine, 'beforeDelete', fired, ['del_scope_a']); @@ -352,9 +419,7 @@ describe('[#5929 / #5860] the gate honours `excludeObjects` on both delete phase 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((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); expect(fired).toEqual([]); }); @@ -380,7 +445,9 @@ describe('[#5929 / #5860] the gate honours `excludeObjects` on both delete phase 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); + // [#7867] The dispatch subtraction is the assertion; the read is the + // unconditional by-id existence read, same as the before-phase case above. + expect((reads.findOneOn['del_scope_a'] ?? 0) - before).toBe(1); expect(fired).toEqual([]); }); }); @@ -466,11 +533,17 @@ describe('[#5929] on a KERNEL-hosted engine the per-object skip finally happens' } }); - 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. + it('[#7867] a single-id delete on a hook-free object performs EXACTLY ONE prior-row read', async () => { + // ⚠️ THE pin, amended. It asserted 0 between #5929 and #7867. The number it + // exists to hold down is "how many reads does ONE by-id delete cost" — + // #5929 drove it from 1 to 0 by retiring a builtin that forced a read it + // never used, and #7867 puts it back to 1 for a reader that does use it: + // the not-found gate, without which this delete would report success for a + // row that was never there. + // + // What must NOT come back is the builtin. The `hasHooksFor` case above + // still asserts its absence directly, and the number here is 1 — not 2, + // which is what a reintroduced `sys_fetch_previous_delete` would cost. const { kernel, engine, reads } = await bootKernel([kernelTask]); try { const row: any = await engine.insert('del_kernel_task', { title: 'A', status: 'todo', done: false }); @@ -479,7 +552,9 @@ describe('[#5929] on a KERNEL-hosted engine the per-object skip finally happens' await engine.delete('del_kernel_task', { where: { id: row.id } }); - expect((reads.findOneOn['del_kernel_task'] ?? 0) - beforeFindOne).toBe(0); + expect((reads.findOneOn['del_kernel_task'] ?? 0) - beforeFindOne).toBe(1); + // The PREDICATE path's gate is untouched by #7867 — no matched-row-set + // read on a hook-free object. expect((reads.findOn['del_kernel_task'] ?? 0) - beforeFind).toBe(0); expect(await engine.count('del_kernel_task', {})).toBe(0); } finally { @@ -552,17 +627,29 @@ describe('[#5929] a fetch-previous `beforeDelete` hook is now dead weight', () = 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). + it('[#7867] the residual shape is now UNREACHABLE — the delete is refused before any hook runs', async () => { + // ⚠️ This case used to assert `seen == [undefined]`: the row was already + // gone, the engine's read bound nothing, and `beforeDelete` dispatched + // anyway with `previous` UNBOUND (never fabricated — #4649/#4775). + // + // The never-fabricate rule is untouched and still governs `bindPreImage`. + // What #7867 changed is that this dispatch no longer happens at all: a + // by-id delete whose id names no row is refused with RECORD_NOT_FOUND + // BEFORE the before phase, so no handler is ever handed a context for a + // record nobody read. That is the same remedy #5574 chose one path over — + // kill the producer, do not specialize what it produced — and it is why + // #5571's six rounds of blaming the binding site were measuring the wrong + // thing: the binding was correct on a path that should never have been + // entered. 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); + const err: any = await engine + .delete('del_scope_a', { where: { id: 'never_existed' } } as any) + .then(() => null, (e) => e); - expect(seen).toEqual([undefined]); + expect(err).not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(seen, 'no handler may be dispatched for a row that is not there').toEqual([]); }); }); diff --git a/packages/objectql/src/engine-filter-tokens.test.ts b/packages/objectql/src/engine-filter-tokens.test.ts index e5d59f358d..c3e85c38e9 100644 --- a/packages/objectql/src/engine-filter-tokens.test.ts +++ b/packages/objectql/src/engine-filter-tokens.test.ts @@ -63,7 +63,16 @@ function makeDriver() { connect: vi.fn().mockResolvedValue(undefined), disconnect: vi.fn().mockResolvedValue(undefined), find: vi.fn(async (_o: string, ast: any) => { seen.findAst = ast; return []; }), - findOne: vi.fn(async (_o: string, ast: any) => { seen.findOneAst = ast; return null; }), + // [#7867] Echoes back the id it was asked for, so a by-id write reaches the + // driver instead of dying at the not-found gate. `return null` here would + // make this double looser than the producer on exactly the write path the + // by-id case below measures; `seen.findOneAst`, which the read cases assert + // on, is captured exactly as before. + findOne: vi.fn(async (_o: string, ast: any) => { + seen.findOneAst = ast; + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, title: 'stored' }; + }), count: vi.fn(async (_o: string, ast: any) => { seen.countAst = ast; return 0; }), aggregate: vi.fn(async (_o: string, ast: any) => { seen.aggregateAst = ast; return []; }), create: vi.fn(async (_o: string, d: any) => d), diff --git a/packages/objectql/src/engine-update-by-id-payload-id.test.ts b/packages/objectql/src/engine-update-by-id-payload-id.test.ts index 456b9627ca..3d4fe2ba29 100644 --- a/packages/objectql/src/engine-update-by-id-payload-id.test.ts +++ b/packages/objectql/src/engine-update-by-id-payload-id.test.ts @@ -86,7 +86,18 @@ function makeRecordingDriver() { supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find() { return []; }, - async findOne() { return null; }, + // [#7867] Answers the row the by-id branch's not-found gate asks for. + // This used to be `return null`, which — now that a by-id update/delete + // refuses a ghost id with `RECORD_NOT_FOUND` — would make every by-id case + // in this file die at the gate and never reach the driver: a DOUBLE looser + // than the producer, hiding the very behaviour the file exists to observe + // (#4434/#4550's shape). It echoes back whatever id it was asked for, so it + // stays agnostic about the dispatch and can never make a `reject` case look + // like a `by-id` one. + async findOne(_o: string, ast: any) { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, title: 'stored' }; + }, async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, async update(_o: string, id: string, data: Record) { calls.push({ fn: 'update', id, data: { ...data } }); diff --git a/packages/objectql/src/engine-update-dispatch.test.ts b/packages/objectql/src/engine-update-dispatch.test.ts index 2c58579e51..5b2a5fbec1 100644 --- a/packages/objectql/src/engine-update-dispatch.test.ts +++ b/packages/objectql/src/engine-update-dispatch.test.ts @@ -35,7 +35,18 @@ function makeRecordingDriver() { supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find() { return []; }, - async findOne() { return null; }, + // [#7867] Answers the row the by-id branch's not-found gate asks for. + // This used to be `return null`, which — now that a by-id update refuses a + // ghost id with `RECORD_NOT_FOUND` — would make every by-id case in this + // file die at the gate and never reach the driver, i.e. a DOUBLE looser + // than the producer hiding the very dispatch verdict the file exists to + // observe (#4434/#4550's shape, in the file that pins against it). It + // echoes back whatever id it was asked for, so it stays agnostic about the + // dispatch: it can never make a `reject` case look like a `by-id` one. + async findOne(_o: string, ast: any) { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, title: 'stored' }; + }, async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, async update(_o: string, id: string, data: Record) { calls.push({ fn: 'update', arg: id }); return { id, ...data }; }, async updateMany(_o: string, ast: unknown) { calls.push({ fn: 'updateMany', arg: ast }); return 0; }, diff --git a/packages/objectql/src/engine-update-multi-payload-id.test.ts b/packages/objectql/src/engine-update-multi-payload-id.test.ts index e4fe0b6812..326fd9ef1b 100644 --- a/packages/objectql/src/engine-update-multi-payload-id.test.ts +++ b/packages/objectql/src/engine-update-multi-payload-id.test.ts @@ -67,7 +67,18 @@ function makeRecordingDriver() { supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find() { return []; }, - async findOne() { return null; }, + // [#7867] Answers the row the by-id branch's not-found gate asks for. + // This used to be `return null`, which — now that a by-id update/delete + // refuses a ghost id with `RECORD_NOT_FOUND` — would make every by-id case + // in this file die at the gate and never reach the driver: a DOUBLE looser + // than the producer, hiding the very behaviour the file exists to observe + // (#4434/#4550's shape). It echoes back whatever id it was asked for, so it + // stays agnostic about the dispatch and can never make a `reject` case look + // like a `by-id` one. + async findOne(_o: string, ast: any) { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, title: 'stored' }; + }, async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, async update(_o: string, id: string, data: Record) { calls.push({ fn: 'update', id, data: { ...data } }); diff --git a/packages/objectql/src/engine-update-prior-read-scope.test.ts b/packages/objectql/src/engine-update-prior-read-scope.test.ts index 86dafbe467..547d1e7d9d 100644 --- a/packages/objectql/src/engine-update-prior-read-scope.test.ts +++ b/packages/objectql/src/engine-update-prior-read-scope.test.ts @@ -34,6 +34,24 @@ * The cases below that used to measure the ABSENCE now measure the presence, * and say so in place: the "count before-hooks too" reflex was answered with * evidence before, and is answered with evidence now that the evidence changed. + * + * ## ⚠️ AMENDED BY #7867 — the by-id SKIP is retired; the per-object question is not + * + * A by-id update now reads its prior row UNCONDITIONALLY, so the gate no longer + * decides whether the engine LOOKS. A fifth demand joined the list and it is + * not expressible as a registration: + * + * 5. EXISTENCE. A by-id update whose id names no row must answer + * `RECORD_NOT_FOUND` rather than run on into validation, the driver and + * the hook chain and die on whichever complains first. Every by-id update + * has this demand, and no cheaper question answers it — so demands 1–4 can + * no longer gate the read, only explain who else consumes it. + * + * What survives intact, and is still pinned below: the demand is asked PER + * OBJECT for DISPATCH (`hasHooksFor` mirroring `triggerHooks`' own filter), + * `previous` is bound from ONE read, the never-fabricate rule, and the + * PREDICATE path's gate — a `multi: true` update matching zero rows is + * legitimately "0 rows affected", not a missing record. */ import { describe, it, expect, vi } from 'vitest'; @@ -196,18 +214,51 @@ const observer = (name: string, object: string, event: string, sink: Array { - it('object A pays NO prior read while only object B has an afterUpdate hook', async () => { - // The issue itself: before the narrowing this delta was 1 — one plugin's - // registration on ONE object taxed every single-id update on every other. + it('[#7867] object A pays exactly ONE prior read — the by-id read is no longer the gate\'s to skip', async () => { + // ⚠️ Asserted 0 between #5284 and #7867. The delta this file exists to hold + // down was never "is it 0" but "does ONE object's registration tax the + // others": before #5284 it was 1 HERE and 1 on B, growing with every + // plugin's registration list. It is 1 here now for a demand that belongs to + // THIS call and nobody else's registration — the not-found gate — and it + // does not grow when another object gains a hook. The per-object property + // is intact; only the skip is gone. + const seen: Array = []; const { engine, reads } = await boot([ - observer('audits_b', 'scope_task_b', 'afterUpdate', []), + observer('audits_b', 'scope_task_b', 'afterUpdate', seen), ]); const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); const before = reads.findOne; await engine.update('scope_task_a', { status: 'in_progress' }, { where: { id: row.id } } as any); - expect(reads.findOne - before).toBe(0); + expect(reads.findOne - before).toBe(1); + // B's hook still did not fire for A's update — the DISPATCH half of the + // per-object question, which is what #5284 was about, is untouched. + expect(seen).toEqual([]); + }); + + it('[#7867] a by-id update against an id that names no row is refused — RECORD_NOT_FOUND', async () => { + // Demand 5, stated as behaviour. This is the defect #7867 fixed: nothing on + // the action-body write path ever asked whether the row existed, so a ghost + // id was a silent no-op that resolved `null` and the write then died on + // whatever the pipeline complained about first. The 400 class varied with + // the object's declarations; the missing 404 was the constant. + const seen: Array = []; + const { engine } = await boot([ + observer('pre_a', 'scope_task_a', 'beforeUpdate', seen), + ]); + + const err: any = await engine + .update('scope_task_a', { status: 'in_progress' }, { where: { id: 'never_existed' } } as any) + .then(() => null, (e) => e); + + expect(err, 'a ghost id must be refused, not silently resolved').not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + // Refused BEFORE the before phase, so no handler is handed a context for a + // record nobody read — the producer is removed, not the message + // specialized (#5574's ruled remedy for this family). + expect(seen, 'beforeUpdate must not dispatch for a row that is not there').toEqual([]); }); it('the object that DOES have the hook still pays it, and `previous` is the stored row', async () => { diff --git a/packages/objectql/src/engine-write-not-found-gate.test.ts b/packages/objectql/src/engine-write-not-found-gate.test.ts new file mode 100644 index 0000000000..9685bbe705 --- /dev/null +++ b/packages/objectql/src/engine-write-not-found-gate.test.ts @@ -0,0 +1,430 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7867] A by-id `update()` / `delete()` whose id names NO ROW is refused with + * `RECORD_NOT_FOUND`, before anything else on the write path runs. + * + * ## The defect, and why it is not the one it looked like + * + * Nothing on the action-body write path ever asked whether the target row + * existed. `ctx.api.object(name).update({ id, … })` → `buildSandboxApi` → + * `ObjectRepository.update` → `ObjectQL.update()`'s by-id branch, and no + * existence gate anywhere in it: `engine.update()` on a ghost id was a SILENT + * NO-OP that resolved `null`, and the write ran on into validation, the driver + * and the hook chain and died on whichever complained first. + * + * Which one that was varied with the object's DECLARATIONS, which is what made + * the defect read as several unrelated bugs: + * + * - a hooked object → `400` `HookConditionError`, from an `afterUpdate` + * condition reading `previous` on a row nobody read; + * - an UNHOOKED object → `400` `VALIDATION_FAILED` "X is required", because + * with no prior row a PATCH is validated as if it were a whole record. + * + * ⇒ **The 400 class varied; the missing 404 was the constant.** That is why + * this file's cases come in hooked/unhooked pairs: a suite that only covered + * the hooked path would be pinning the symptom instead of the defect. + * + * ⛔ It is NOT a `previous`-binding bug. `if (priorRecord) hookContext.previous + * = …` does exactly what ADR-0058 Addendum II / #4649 require — an absent row + * leaves `previous` UNBOUND rather than fabricated as `{}`/`null` — and that + * rule is untouched here. It was behaving correctly on a path that should never + * have been entered, which is the attribution #5571 carried for six triage + * rounds before its reproduction measured it wrong. The remedy is #5574's, one + * path over: remove the PRODUCER, do not specialize what it produced. Hence the + * "no handler ran" assertions below — they are the load-bearing half. + * + * ## Why the gate is at the engine and not at the repository + * + * The action body reaches the engine three ways, only ONE of which passes + * through `ObjectRepository`: + * + * 1. `ctx.api.object(n).update(…)` → `ScopedContext` → `ObjectRepository` + * 2. `ctx.api.object(n)` when the host engine has no `createContext` + * → `buildEngineRepoFacade` → `ql.update(…)` DIRECTLY + * 3. `ctx.engine.update(o, id, data)` → `buildActionEngineFacade` + * → `ql.update(…)` DIRECTLY + * + * A repository-level gate closes (1) and leaves (2) and (3) with the original + * defect, and makes `ql.update(o, { id })` and `ctx.api.object(o).update({ id })` + * answer one ghost id two different ways — the second de-facto contract PD #12 + * exists to keep out. The engine is where all three funnel through. + * + * ## Two sibling paths already had this gate; this is the third, not a fourth + * + * - `protocol.updateData`/`deleteData` probe existence and throw + * `recordNotFoundError` (#4435) + * - `callData`'s ObjectQL fallback does the same (#5138) + * + * All three now call the SAME `recordNotFoundError` — moved to + * `@objectstack/core` by this card so `engine.ts` can reach it without + * importing `@objectstack/metadata-protocol`, which ADR-0076 D2's boundary + * ratchet forbids in the `/core` closure. + * + * ## Scope line: BY-ID only + * + * A `multi: true` predicate write matching zero rows is legitimately "0 rows + * affected", not a missing record — the same line both siblings draw. Pinned + * below so the gate cannot creep onto the bulk path. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import type { Hook, ServiceObject } from '@objectstack/spec/data'; + +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +/** + * A HOOKED object carrying the exact declaration the reproduction tripped over: + * an `afterUpdate` condition that reads `previous`. + * + * Typed as `ServiceObject` rather than left to inference, and registered with + * its `packageId`, so this file adds nothing to `@objectstack/objectql`'s + * TEST_DEBT ledger — a shrink-only ratchet (#5278). Typing it is also what + * showed that `primaryKey` is not a declared field property: the registry + * provisions the primary key itself (`provisionPrimary`), so carrying the key + * here would have been a silent no-op the compiler could not see while the + * fixture stayed untyped. + */ +const hookedTask: ServiceObject = { + name: 'nf_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, + }, +}; + +/** + * An UNHOOKED object with a REQUIRED field — the #7867 P3 probe's shape. On + * this one the pre-fix answer was a required-field `ValidationError`, with no + * hook and no `previous` anywhere near it. + */ +const unhookedInvoice: ServiceObject = { + name: 'nf_invoice', + label: 'Invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + issued_on: { name: 'issued_on', label: 'Issued On', type: 'text' as const, required: true }, + }, +}; + +/** A minimal store-backed driver — the same shape `hook-condition-fail-loud` uses. */ +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 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 calls = { update: 0, delete: 0, updateMany: 0, deleteMany: 0 }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + 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) { + calls.update += 1; + const s = storeFor(o); const cur = s.get(id); if (!cur) return null; + const u = { ...cur, ...data, id }; s.set(id, u); return u; + }, + async upsert(o: string, data: any) { + const id = data.id; + return id && storeFor(o).has(id) ? this.update(o, id, data) : this.create(o, data); + }, + async delete(o: string, id: string) { calls.delete += 1; return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async updateMany(o: string, ast: any, data: Record) { + calls.updateMany += 1; + const rows = await this.find(o, ast); + for (const r of rows) storeFor(o).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany(o: string, ast: any) { + calls.deleteMany += 1; + const rows = await this.find(o, ast); + for (const r of rows) storeFor(o).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver: d, calls }; +} + +/** Owning package for the fixtures — `registerObject` requires one. */ +const OWNER_PACKAGE = 'app:nf'; + +async function bootEngine( + hooks: Hook[], + objects: ServiceObject[] = [hookedTask, unhookedInvoice], +) { + const engine = new ObjectQL(); + const stub = makeStubDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + // `packageId` is REQUIRED by `SchemaRegistry.registerObject` — passing it (as + // the kernel-booting sibling files do) rather than dropping an `as any` on + // the call keeps this file out of the package's TEST_DEBT ledger, which is a + // shrink-only ratchet (#5278). + for (const o of objects) engine.registry.registerObject(o, OWNER_PACKAGE); + if (hooks.length > 0) { + bindHooksToEngine(engine, hooks, { packageId: OWNER_PACKAGE, logger: silentLogger }); + } + return { engine, calls: stub.calls }; +} + +/** + * The reproduction's own hook: an `afterUpdate` whose declarative `condition` + * reads `previous`. Pre-fix, a ghost-id update reached this and produced the + * `HookConditionError` 400 that #5571 spent six rounds attributing to the + * binding site. + */ +const auditHook: Hook = { + name: 'nf_audit_task_completion', + object: 'nf_task', + events: ['afterUpdate'], + priority: 100, + condition: 'previous.done != true && record.done == true', + handler: () => {}, +} as unknown as Hook; + +/* ──────────────────────────────────────────────────────────────────────────── + * 1. update() — the 404, on BOTH declaration shapes + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#7867] a by-id update against a nonexistent id answers RECORD_NOT_FOUND', () => { + it('HOOKED object: 404 with code + status — not the HookConditionError 400 it used to be', async () => { + const { engine, calls } = await bootEngine([auditHook]); + + const err: any = await engine + .update('nf_task', { id: 'ghost', done: true }) + .then(() => null, (e) => e); + + expect(err, 'the write must be refused, not silently resolved').not.toBeNull(); + // Asserted on `code` + `status`, never on "it threw": the pre-fix behaviour + // ALSO threw here — with a 400 — so "it threw" cannot tell the two apart. + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + expect(err.message).toContain('ghost'); + expect(err.message).toContain('nf_task'); + // …and specifically NOT the shape the defect produced. + expect(err.name).not.toBe('HookConditionError'); + expect(String(err.message)).not.toContain('not bound for this operation'); + expect(calls.update, 'nothing may reach the driver').toBe(0); + }); + + it('UNHOOKED object with a required field: the same 404, not VALIDATION_FAILED', async () => { + // The measurement that widened the card: no hooks, no `previous`, same + // defect. Pre-fix this answered 400 "Issued On is required", because with + // no prior row a PATCH is validated as if it were a whole record. + const { engine, calls } = await bootEngine([]); + + const err: any = await engine + .update('nf_invoice', { id: 'ghost', status: 'sent' }) + .then(() => null, (e) => e); + + expect(err).not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + expect(String(err.message)).not.toMatch(/required/i); + expect(calls.update).toBe(0); + }); + + it('refuses BEFORE the before phase — no handler observes a row nobody read', async () => { + // The load-bearing assertion. Removing the PRODUCER is what makes the + // symptom impossible; leaving the dispatch in place and only changing the + // final status would leave every other handler on the path still running + // against a record that does not exist. + const ran: string[] = []; + const { engine } = await bootEngine([ + { name: 'nf_pre', object: 'nf_task', events: ['beforeUpdate'], priority: 10, + handler: () => { ran.push('before'); } } as unknown as Hook, + { name: 'nf_post', object: 'nf_task', events: ['afterUpdate'], priority: 10, + handler: () => { ran.push('after'); } } as unknown as Hook, + ]); + + await engine.update('nf_task', { id: 'ghost', done: true }).catch(() => undefined); + + expect(ran).toEqual([]); + }); + + it('an id that DOES name a row is untouched — the control', async () => { + const { engine, calls } = await bootEngine([auditHook]); + const row: any = await engine.insert('nf_task', { title: 'Ship it', done: false }); + + const res: any = await engine.update('nf_task', { id: row.id, done: true }); + + expect(res).toMatchObject({ id: row.id, done: true }); + expect(calls.update).toBe(1); + }); + + it('reaches the gate through `where.id` as well as through the payload', async () => { + // Both spellings dispatch by-id (`ENGINE_UPDATE_DISPATCH_CASES`), so both + // owe the same answer — a gate wired to one of them is the #3106 shape. + const { engine } = await bootEngine([]); + + const err: any = await engine + .update('nf_invoice', { status: 'sent' }, { where: { id: 'ghost' } } as any) + .then(() => null, (e) => e); + + expect(err?.code).toBe('RECORD_NOT_FOUND'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 2. delete() — the twin. #5138's record: the worst of the three. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#7867] a by-id delete against a nonexistent id answers RECORD_NOT_FOUND', () => { + it('404 instead of reporting a deletion that never happened', async () => { + // "The delete ran and the answer was 200 { deleted: true } for any string + // in the path, so a typo'd id, an already-deleted row and a real deletion + // were indistinguishable" — #5138 removed that from `callData`; it was + // still live here. + const { engine, calls } = await bootEngine([]); + + const err: any = await engine + .delete('nf_task', { where: { id: 'ghost' } } as any) + .then(() => null, (e) => e); + + expect(err).not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + expect(calls.delete, 'nothing may reach the driver').toBe(0); + }); + + it('refuses before `beforeDelete` dispatches — and before any cascade runs', async () => { + const ran: string[] = []; + const { engine } = await bootEngine([ + { name: 'nf_pre_del', object: 'nf_task', events: ['beforeDelete'], priority: 10, + handler: () => { ran.push('before'); } } as unknown as Hook, + { name: 'nf_post_del', object: 'nf_task', events: ['afterDelete'], priority: 10, + handler: () => { ran.push('after'); } } as unknown as Hook, + ]); + + await engine.delete('nf_task', { where: { id: 'ghost' } } as any).catch(() => undefined); + + expect(ran).toEqual([]); + }); + + it('a real id still deletes — the control', async () => { + const { engine, calls } = await bootEngine([]); + const row: any = await engine.insert('nf_task', { title: 'Ship it', done: false }); + + await engine.delete('nf_task', { where: { id: row.id } } as any); + + expect(calls.delete).toBe(1); + expect(await engine.count('nf_task', {})).toBe(0); + }); + + it('a SECOND delete of the same id is refused — the already-deleted case is now distinguishable', async () => { + const { engine } = await bootEngine([]); + const row: any = await engine.insert('nf_task', { title: 'Ship it', done: false }); + await engine.delete('nf_task', { where: { id: row.id } } as any); + + const err: any = await engine + .delete('nf_task', { where: { id: row.id } } as any) + .then(() => null, (e) => e); + + expect(err?.code).toBe('RECORD_NOT_FOUND'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 3. The scope line — the PREDICATE path keeps "0 rows affected" + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#7867] the gate is BY-ID only — a predicate write matching nothing is not a 404', () => { + it('a multi:true update matching zero rows resolves 0, it does not throw', async () => { + const { engine, calls } = await bootEngine([]); + + const res = await engine.update( + 'nf_task', { done: true }, { where: { title: 'nothing matches' }, multi: true } as any, + ); + + expect(res).toBe(0); + expect(calls.updateMany).toBe(1); + }); + + it('a multi:true delete matching zero rows resolves 0, it does not throw', async () => { + const { engine, calls } = await bootEngine([]); + + const res = await engine.delete( + 'nf_task', { where: { title: 'nothing matches' }, multi: true } as any, + ); + + expect(res).toBe(0); + expect(calls.deleteMany).toBe(1); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 4. The envelope is the repo's ONE not-found envelope + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#7867] the engine answers with the SAME error the protocol and callData answer with', () => { + it('is `recordNotFoundError` itself, not a look-alike', async () => { + // The whole point of moving the factory into `@objectstack/core`: a + // re-spelled envelope here would be the second not-found shape #5138 ruled + // out, and REST maps 404 by reading `error.code === 'RECORD_NOT_FOUND'` + // (`rest-server.ts`), so a look-alike with a different code would surface as + // a 500. + const { recordNotFoundError } = await import('@objectstack/core'); + const reference: any = recordNotFoundError('nf_task', 'ghost'); + + const { engine } = await bootEngine([]); + const actual: any = await engine + .update('nf_task', { id: 'ghost', done: true }) + .then(() => null, (e) => e); + + expect(actual.code).toBe(reference.code); + expect(actual.status).toBe(reference.status); + expect(actual.object).toBe(reference.object); + expect(actual.message).toBe(reference.message); + }); + + it('`@objectstack/metadata-protocol` still exports it, answering byte-identically', async () => { + // The move must be invisible to the two sibling paths that import it from + // there. Compared by ANSWER rather than by reference: this monorepo resolves + // `@objectstack/core` from source for objectql and from `dist` for + // metadata-protocol, so two module instances of one source file are the + // norm here and a `toBe` would be pinning the resolver, not the contract. + const { recordNotFoundError: fromProtocol } = await import('@objectstack/metadata-protocol'); + const { recordNotFoundError: fromCore } = await import('@objectstack/core'); + + expect(typeof fromProtocol).toBe('function'); + const a: any = fromProtocol('nf_task', 'ghost'); + const b: any = fromCore('nf_task', 'ghost'); + expect({ code: a.code, status: a.status, object: a.object, message: a.message }) + .toEqual({ code: b.code, status: b.status, object: b.object, message: b.message }); + }); +}); diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index d3adcbf6f2..f73c50cc63 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -961,10 +961,56 @@ describe('ObjectQL Engine', () => { expect(captured).toEqual({ id: 't1', status: 'in_review', assignee: 'sam@example.com' }); }); - it('does not fetch the prior record when no afterUpdate hook is registered and no rule needs it', async () => { + it('[#7867] DOES fetch the prior record even with no hook and no rule — existence is a consumer #5284 never counted', async () => { + // ⚠️ This case is the INVERSE of what it asserted until #7867, and + // the inversion IS the change, not a casualty of it. #5284 narrowed + // the by-id prior read to "does anything CONSUME the prior row?", so + // an object with no `beforeUpdate`/`afterUpdate` hook, no + // prior-reading validation rule and no roll-up paid no read. + // + // Existence is a consumer that demand list never enumerated, and it + // is the one consumer EVERY by-id write has: a write against an id + // that names no row must answer `RECORD_NOT_FOUND`, and no cheaper + // question answers that. The two are mutually exclusive, so the + // narrowing is retired rather than worked around. + // + // What that costs, measured rather than assumed: #5929's twin in + // `delete()` enumerates the global hook registrants (plugin-sharing, + // service-storage, plugin-auth, plugin-audit — all registering with + // no `object`, hence matching every object), so on any kernel that + // loads them the demand was ALREADY true for every object and the + // narrowing skipped nothing there. The read is genuinely new only + // for a bare `@objectstack/objectql/core` embedder — which is + // buying a 404 it did not have. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: 't1', status: 'todo' }); vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any); await engine.update('task', { id: 't1', status: 'done' }); - expect(mockDriver.findOne).not.toHaveBeenCalled(); + expect(mockDriver.findOne).toHaveBeenCalledTimes(1); + }); + + it('[#7867] refuses a by-id update whose id names no row — RECORD_NOT_FOUND, before any hook runs', async () => { + // The producer half of the defect the `previous` cases above + // describe from the other side. With no gate, a ghost id sailed on + // into validation, the driver and the hook chain and died on + // whichever complained first — a `HookConditionError` on a hooked + // object, a required-field failure on an unhooked one. The 400 + // class varied with the object's declarations; the missing 404 was + // the constant. + vi.mocked(mockDriver.findOne).mockResolvedValue(null as any); + let ran = false; + engine.registerHook('beforeUpdate', async () => { ran = true; }, { object: 'task' }); + + const err: any = await engine.update('task', { id: 'ghost', status: 'done' }) + .then(() => null, (e) => e); + + expect(err, 'a ghost id must be refused, not silently resolved').not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + // Refused BEFORE the before phase: no handler observes a row that is + // not there, which removes the symptom at its producer rather than + // specializing the message the symptom happened to produce. + expect(ran, 'beforeUpdate must not dispatch for a row that is not there').toBe(false); + expect(mockDriver.update).not.toHaveBeenCalled(); }); }); @@ -996,6 +1042,9 @@ describe('ObjectQL Engine', () => { }); it('still treats a scalar where.id as a single-row update', async () => { + // [#7867] The by-id branch reads the target row before it writes, + // so the double has to hold one — see the not-found gate case above. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: 't1', status: 'todo' }); vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any); await engine.update('task', { status: 'done' }, { where: { id: 't1' } } as any); expect(mockDriver.update).toHaveBeenCalledTimes(1); @@ -1061,6 +1110,8 @@ describe('ObjectQL Engine', () => { if (opCtx.operation === 'update') seenAst = opCtx.ast; await next(); }); + // [#7867] The by-id branch reads the target row before it writes. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: 't1', status: 'todo' }); vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any); await engine.update('task', { status: 'done' }, { where: { id: 't1' } } as any); @@ -1105,6 +1156,11 @@ describe('ObjectQL Engine', () => { // from the matched row set), so there is no branch left to // re-enter. The lever is refused by name rather than caught one // layer down by a security backstop that was never about it. + // [#7867] The target row must EXIST for the ladder to reach the + // before phase at all — a ghost id is now refused one step earlier, + // with RECORD_NOT_FOUND, so this case would otherwise never get to + // the lever it is about. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: 't1', status: 'todo' }); engine.registerHook('beforeUpdate', async (ctx: any) => { ctx.input.id = undefined; }); @@ -1358,6 +1414,12 @@ describe('ObjectQL Engine', () => { engine.registerDriver(mockDriver, true); await engine.init(); (mockDriver as any).updateMany = vi.fn().mockResolvedValue(1); + // [#7867] Every single-id case below writes to record '1'; the by-id + // branch now reads that row first and refuses with + // RECORD_NOT_FOUND when it is not there, so the double has to hold + // it. The cases that care about the prior row's CONTENT (the + // `readonlyWhen` ones) still override this with their own shape. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: '1', title: 'stored' } as any); }); const docSchema = { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 14990be244..b5856c4658 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -64,6 +64,11 @@ import { type RetryOptions, filterTokenContextFrom, resolveFilterTokens, + // [#7867] The repo's ONE single-record 404 (#4435/#5138). It lives in + // `@objectstack/core` precisely so this file can reach it: ADR-0076 D2's + // boundary ratchet forbids the `/core` entry closure — engine.ts included — + // from importing `@objectstack/metadata-protocol`, where it was written. + recordNotFoundError, } from '@objectstack/core'; import { SummaryRecomputeError, type SummaryRecomputeFailure } from './summary-errors.js'; import { CrossDatasourceTransactionWriteError, TransactionUnsupportedError } from './transaction-errors.js'; @@ -8160,9 +8165,9 @@ export class ObjectQL implements IObjectQLEngine { // Pre-update snapshot. Exposed to hooks via `hookContext.previous` in // BOTH phases now (the HookContext contract documents `previous` for // update/delete) and reused for object-level validation rules and the - // roll-up recompute. Fetched once, only for single-id updates, and only - // when something on THIS object actually consumes it — see - // `wantsPriorRecord` below. + // roll-up recompute. Fetched once, and only for single-id updates — + // [#7867] unconditionally there, since the not-found gate at the by-id + // branch below is a fourth consumer that every such write has. let priorRecord: Record | null = null; // [#5038] The matched rows a PREDICATE write fires its per-row // `afterUpdate` contexts over. `[]` is meaningful and distinct from @@ -8186,26 +8191,78 @@ export class ObjectQL implements IObjectQLEngine { // `ql.findOne` on every by-id update to bind exactly this value; // #5846 retires it, because the engine now binds `previous` before // any authored before-hook runs. - const wantsPriorRecord = - needsPriorRecord(updateSchema as any) || - this.hasHooksFor('beforeUpdate', object) || - this.hasHooksFor('afterUpdate', object) || - this.getSummaryDescriptors(object).length > 0; - if (wantsPriorRecord) { - // `buildDriverOptions` is what carries the open transaction and - // the tenant scope onto a raw driver read — the same bag the - // post-phase write uses, built here because the write's own - // merge has not happened yet. `delete()`'s pre-image read does - // the same for the same reason. - const priorAst: QueryAST = { object, where: { id }, limit: 1 }; - const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); - priorRecord = await driver.findOne(object, priorAst, preOpts); - // 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) — `delete()`'s `bindPreImage` rule, verbatim. - if (priorRecord) hookContext.previous = coerceBooleanFields(updateSchema as any, priorRecord as any) as any; - } + // [#7867] …and the read is now UNCONDITIONAL, because the gate below + // is a question only a read can answer. `wantsPriorRecord` — the + // #5284 narrowing this replaces — asked "does anything CONSUME the + // prior row?" and skipped the read when nothing did. Existence is a + // consumer it never counted, and it is the one consumer every by-id + // write has. + // + // What the narrowing actually bought, measured rather than assumed: + // its own #5929 twin in `delete()` enumerates the global registrants + // (plugin-sharing, service-storage, plugin-auth, plugin-audit — all + // registering with no `object`, hence matching every object), so on + // any kernel that loads them `wantsPriorRecord` was ALREADY true for + // everything and skipped nothing. The read becomes genuinely new + // only for a bare `@objectstack/objectql/core` embedder whose object + // has no hooks, no prior-reading validation rule and no roll-up — + // and that embedder is buying a 404 it did not have. + // + // ⚠️ It cannot be answered from the write's own return value + // instead. `IDataDriver.update` declares `Promise>` — no not-found signal in the contract at all — and the + // engine's post-write readback is `null` for a SECOND reason + // (`protocol.updateData`'s own note: a write that moves the row out + // of the caller's row scope, e.g. reassigning `owner_id` away from + // yourself under an owner-scoped policy, reads back null while + // having succeeded). Reading either as "not found" would answer 404 + // to a write that landed. Both siblings ask existence BEFORE the + // write for exactly this reason; so does this. + // + // `buildDriverOptions` is what carries the open transaction and + // the tenant scope onto a raw driver read — the same bag the + // post-phase write uses, built here because the write's own + // merge has not happened yet. `delete()`'s pre-image read does + // the same for the same reason. + const priorAst: QueryAST = { object, where: { id }, limit: 1 }; + const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); + priorRecord = await driver.findOne(object, priorAst, preOpts); + // ── [#7867] The not-found gate ────────────────────────────────── + // + // A by-id update whose id names no row was a SILENT NO-OP that + // resolved `null`: nothing on this path ever asked whether the row + // existed, so the write ran on into validation, the driver and the + // hook chain, and died on whichever of them complained first. The + // 400 class varied with the object's declarations — a + // `HookConditionError` on a hooked object, a required-field + // `VALIDATION_FAILED` on an unhooked one — while the missing 404 was + // the constant. Two sibling paths had this gate and an action body + // traverses neither: `protocol.updateData` (#4435) and `callData`'s + // ObjectQL fallback (#5138). This is the third, placed at the one + // point all of them funnel through, so it is not a fourth site. + // + // ⚠️ It throws BEFORE `triggerHooks('beforeUpdate')` deliberately, + // and that ordering is the fix rather than a detail of it. The + // reported symptom was an `afterUpdate` condition reading `previous` + // on a row that was never there; `if (priorRecord) …` below is + // CORRECT and stays untouched (ADR-0058 Addendum II / #4649 — + // never fabricate a prior state), it was simply running on a path + // that should never have been entered. Killing the producer is + // #5574's ruled remedy for this family, not specializing the + // message the symptom happened to produce. + // + // Scope: the BY-ID branch only. A `multi: true` predicate update + // that matches zero rows is legitimately "0 rows affected", not a + // missing record — same line both siblings draw. + if (!priorRecord) throw recordNotFoundError(object, id); + // 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) — `delete()`'s `bindPreImage` rule, verbatim. + // The guard is kept verbatim although the gate above now makes it + // permanently true here: it states the invariant, and the invariant + // outlives this call site. + if (priorRecord) hookContext.previous = coerceBooleanFields(updateSchema as any, priorRecord as any) as any; await this.triggerHooks('beforeUpdate', hookContext); // The retired lever, refused. Everything above — `previous`, and // below it the `readonlyWhen` strip and every validation rule — was @@ -9253,11 +9310,28 @@ export class ObjectQL implements IObjectQLEngine { // 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. + // [#7867] ⚠️ RETIRED — the three-term `wantsPreImage` gate the paragraphs + // above describe is GONE, and the paragraphs are kept because what they + // record (which registrants hold which term open, and why an honest gate + // is not the same as a usually-false one) is still the reason the removal + // costs nothing measurable. + // + // It asked "does anything CONSUME the pre-image?" and skipped the read + // when nothing did. Existence is a consumer it never counted — and it is + // the one consumer EVERY by-id delete has, because a delete against an id + // that names no row must answer 404 rather than run. So the by-id branch + // below reads the pre-image unconditionally and gates on it; the + // predicate branch reads its doomed rows under its own `perRowBefore/ + // AfterHooks` gate, which is untouched. On any kernel loading the global + // registrants enumerated above, term 1 or 2 was already true for every + // object, so this skipped nothing there anyway; the read becomes + // genuinely new only for a bare embedder whose object has no delete-side + // hook and no roll-up — and that embedder is buying a 404 it did not have. + // + // ⛔ Do not reintroduce it as a guard around the by-id read. A gate on + // whether to LOOK is not compatible with a rule about what to do when + // nothing is there. See `update()`'s twin. 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 — @@ -9300,10 +9374,29 @@ export class ObjectQL implements IObjectQLEngine { }; if (isByIdDelete) { - if (wantsPreImage) { - priorRecord = await readPreImage(id); - bindPreImage(priorRecord); - } + // [#7867] Read first, then GATE — the twin of `update()`'s, and #5138's + // own record names `delete` as the worst of the three when the gate was + // missing: "the delete ran and the answer was `200 { deleted: true }` + // for any string in the path, so a typo'd id, an already-deleted row + // and a real deletion were indistinguishable" — the shape #4435 removed + // from `protocol.deleteData` and #5138 removed from `callData`, still + // live here on the path an action body's `.delete()` actually takes. + // + // The pre-image is the only honest place to ask. `IDataDriver.delete` + // does declare `Promise` ("true if deleted, false if not + // found"), so the answer exists downstream — but downstream is AFTER + // `beforeDelete` has dispatched and after `cascadeDeleteRelations` has + // run, i.e. after handlers have fired and children have been touched + // for a parent that was never there. `IDataEngine.delete` also declares + // `Promise` and passes its driver's result through the hook chain, + // so testing it for `=== false` here would read a signal this layer's + // contract does not promise — #5138's argument, unchanged. + priorRecord = await readPreImage(id); + if (!priorRecord) throw recordNotFoundError(object, id); + // Bound unconditionally now that the row is proven present. + // `bindPreImage`'s never-fabricate rule (#4649/#4775) is unchanged and + // still the reason the binding goes through it rather than around it. + bindPreImage(priorRecord); await this.triggerHooks('beforeDelete', hookContext); // [#6752] The retired lever, refused — the `update()` twin's check, // verbatim, because the rule is now ONE rule: a by-id target is diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index a55fab76d7..330e3cb89b 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -706,9 +706,15 @@ describe('[#5272] a single-record delete binds `previous` through the real engin 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. + it('[#7867] still reads exactly ONCE when the object has no delete-side hook — for existence, not for `previous`', async () => { + // ⚠️ Asserted 0 until #7867. The pre-image read on the by-id path is now + // unconditional: `delete()` has to know whether the row is there before it + // runs, because a delete against an id naming no row must answer + // RECORD_NOT_FOUND rather than report a deletion that never happened. + // + // What the number still holds down is that there is ONE read — the + // engine's — and not a second one behind some hook's guard. That was + // #5929's subject and it is unchanged. const { engine, reads } = await bootDelete([{ name: 'update_only_guard', object: 'hook_task', @@ -722,17 +728,31 @@ describe('[#5272] a single-record delete binds `previous` through the real engin const baseline = reads.findOne; await engine.delete('hook_task', { where: { id: row.id } } as any); - expect(reads.findOne - baseline).toBe(0); + expect(reads.findOne - baseline).toBe(1); }); - it('leaves `previous` UNBOUND when the row is not there — nothing is fabricated', async () => { + it('[#7867] never REACHES the unbound-`previous` shape — the delete is refused first', async () => { + // ⚠️ This asserted `[undefined]` until #7867: the row was gone, the engine + // bound nothing, and `beforeDelete` dispatched anyway with `previous` + // absent. The never-fabricate rule that made it `undefined` rather than + // `{}`/`null` is UNCHANGED and still governs `bindPreImage` (#4649/#4775) — + // what changed is that no handler is dispatched at all for a row that is + // not there, so the shape has no way to arise on this path. + // + // This is the correction #5571 spent six triage rounds not finding: the + // binding was behaving correctly on a path that should never have been + // entered. Loosening it would have fixed the wrong thing; removing the + // entry is the fix. const seen: Array | undefined> = []; const { engine } = await bootDelete([observer('beforeDelete', seen)]); - await engine.delete('hook_task', { where: { id: 'never_existed' } } as any); + const err: any = await engine + .delete('hook_task', { where: { id: 'never_existed' } } as any) + .then(() => null, (e) => e); - // `{}` or `null` here would let `previous.status == "done"` answer for a - // record nobody read. Absent stays absent (#4649/#4775). - expect(seen).toEqual([undefined]); + expect(err).not.toBeNull(); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + expect(seen, 'no handler may be dispatched for a record nobody read').toEqual([]); }); }); diff --git a/packages/objectql/src/plugin.integration.test.ts b/packages/objectql/src/plugin.integration.test.ts index 4391003e36..a013de8e72 100644 --- a/packages/objectql/src/plugin.integration.test.ts +++ b/packages/objectql/src/plugin.integration.test.ts @@ -1374,7 +1374,15 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { const mockDriver = { name: 'ro-capture', version: '1.0.0', connect: async () => {}, disconnect: async () => {}, - find: async () => [], findOne: async () => null, + find: async () => [], + // [#7867] The by-id update path reads its target row before it writes; + // `findOne: async () => null` would make this double looser than the + // producer and every case below would die at the not-found gate instead + // of reaching the strip it measures. + findOne: async (_o: string, ast: any) => { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, name: 'stored' }; + }, create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }), update: async (_o: string, _i: any, d: any) => { updates.push({ ...d }); return { id: _i, ...d }; }, delete: async () => true, syncSchema: async () => {}, @@ -1438,7 +1446,15 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { const mockDriver = { name: 'hist-capture', version: '1.0.0', connect: async () => {}, disconnect: async () => {}, - find: async () => [], findOne: async () => null, + find: async () => [], + // [#7867] The by-id update path reads its target row before it writes; + // `findOne: async () => null` would make this double looser than the + // producer and every case below would die at the not-found gate instead + // of reaching the strip it measures. + findOne: async (_o: string, ast: any) => { + const id = ast?.where?.id; + return id === undefined || id === null ? null : { id, name: 'stored' }; + }, create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }), update: async (_o: string, _i: any, d: any) => { updates.push({ ...d }); return { id: _i, ...d }; }, delete: async () => true, syncSchema: async () => {}, diff --git a/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts b/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts index 37363e1290..cbc40440c3 100644 --- a/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts +++ b/packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts @@ -254,7 +254,25 @@ const gateOpen = (engine: unknown, event: string, object: string): boolean => // --------------------------------------------------------------------------- describe('[#5860] a SKIP_OBJECTS object no longer forces the prior-row read', () => { - it('single-id update() on `sys_job_queue` pays ZERO prior reads', async () => { + it('[#7867] single-id update() on `sys_job_queue` pays ONE prior read — for existence, not for audit', async () => { + // ⚠️ Asserted 0 between #5860 and #7867, and the flip is deliberate. + // + // #5860's acceptance criterion is that the per-object DEMAND GATE judges a + // SKIP_OBJECTS object unhooked — and that is unchanged, asserted directly + // by the very next case (`gateOpen(...)` is `false` for all five events) + // and by the "no audit row is written" case below. What changed is that the + // gate no longer decides whether the ENGINE LOOKS at the row. + // + // #7867 added the not-found gate the by-id write path never had: a write + // against an id that names no row must answer `RECORD_NOT_FOUND` rather + // than run on and die on whatever the pipeline complains about first. + // Existence is a consumer no hook registration can express and the one + // consumer every by-id write has, so the read is unconditional now — for + // `sys_job_queue` exactly as for everything else. + // + // The number still holds down what this file cares about: it is ONE — the + // engine's own — not the TWO an audit handler forcing its own read would + // cost, and the audit ledger below is still empty. const { engine, reads } = await boot(); installAuditWriters(engine); @@ -262,9 +280,7 @@ describe('[#5860] a SKIP_OBJECTS object no longer forces the prior-row read', () const before = reads.findOneOn['sys_job_queue'] ?? 0; await engine.update('sys_job_queue', { status: 'running' }, { where: { id: row.id } } as any); - // Before this change: 1 — #5284's gate saw five global audit registrations - // and could not know the handler returns on its first line. - expect((reads.findOneOn['sys_job_queue'] ?? 0) - before).toBe(0); + expect((reads.findOneOn['sys_job_queue'] ?? 0) - before).toBe(1); }); it('the gate itself answers `false` for every audit event on a skipped object', async () => { diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts index a3a151fd48..68454def79 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -690,15 +690,39 @@ describe('[#5941] break-glass: the last unbanned administrator cannot be DELETED // `usr_member`'s membership carries no administrative grade, so removing it // takes no standing away — the standing halves (#5978) below judge every // `sys_member` delete, and this is what "judged and allowed" looks like. - await seedUser(engine, 'usr_member', { role: 'member' }); + // The account row is what the second half of this case deletes. + await seedUser(engine, 'usr_member', { role: 'member', accountProvider: 'credential' }); await expect( engine.delete('sys_member', { where: { id: 'mem_usr_member' }, ...SYSTEM }), ).resolves.toBeDefined(); - // …and a table this guard reads but does not write-guard is untouched. + // …and a table this guard reads but does not write-guard is untouched: the + // delete goes through. + // + // [#7867] This half used to delete the id `'nope'` — a row that was never + // seeded — and assert it RESOLVED. That passed for a reason unrelated to + // this guard: `ObjectQL.delete()` had no existence gate on its by-id path, + // so a delete naming no row was a silent no-op that reported success. The + // engine now answers `RECORD_NOT_FOUND` there, which is the change #7867 + // landed, so the old line would have been asserting the absence of a guard + // by way of a defect. + // + // Deleting a REAL `sys_account` row states the same thing without borrowing + // that defect, and states it more strongly: the guard does not merely fail + // to fire on a write that touched nothing — it lets a write that really + // removes a row on this object through. await expect( - engine.delete('sys_account', { where: { id: 'nope' }, ...SYSTEM }), + engine.delete('sys_account', { where: { id: 'acc_usr_member' }, ...SYSTEM }), ).resolves.toBeDefined(); + // The other half of "not this guard's business", kept explicit: a ghost id + // on this object is refused by the ENGINE, not by the last-admin guard — + // so the refusal above staying absent is about the guard, and this refusal + // is about existence. Two different questions, two different answers. + const ghost = await engine + .delete('sys_account', { where: { id: 'nope' }, ...SYSTEM }) + .then(() => null, (e: any) => e); + expect(ghost?.code).toBe('RECORD_NOT_FOUND'); + expect(String(ghost?.message ?? '')).not.toMatch(/last administrator/i); }); // NOTE (#5978): the case that used to live here asserted the OPPOSITE — that diff --git a/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts b/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts index abd5942f76..fc3996e203 100644 --- a/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts @@ -202,6 +202,69 @@ describe('showcase: anonymous posture is uniform across surfaces (#2567)', () => expect(r.status, 'an authenticated caller must clear the auth gate').not.toBe(401); }); + // ── #7867 — what the member's answer actually IS, now that it is asserted ── + // + // ⚠️ These two cases exist because the one ABOVE cannot fail for them, by + // design: its `.not.toBe(401)` is deliberately about anonymity and tolerates + // any non-401. For as long as this fixture has existed that tolerance let a + // real defect ride through this required 3-shard gate, 23/23 green, while + // printing a stack trace on every run: + // + // ERROR Update operation failed … Hook 'showcase_audit_task_completion' + // could not evaluate its condition (type: Unknown variable: previous) … + // at unevaluableConditionError (packages/objectql/src/hook-wrappers.ts) + // at _ObjectQL.triggerHooks (packages/objectql/src/engine.ts) + // ERROR [BodyRunner] sandboxed action threw … + // + // `ACTION` targets a deliberately NON-EXISTENT record id — the anonymous + // cases need that, since the 401 floor must land before any lookup — so the + // member's request is a by-id write against a ghost id. `ObjectQL.update()` + // had no not-found gate on that path, so the write ran on and died on + // whichever stage complained first: a `HookConditionError` 400 here, a + // required-field 400 on an unhooked object. The 400 class varied with the + // object's declarations; the missing 404 was the constant (#7867, from + // #5571's reproduction). + // + // Asserted on the CODE and the STATUS, never on "it threw": the pre-fix + // behaviour also produced an error response, so "it failed" cannot tell the + // two apart — and 404-vs-400 is exactly what a client's retry and cache + // policy read. + // + // ⛔ The case above is left as it was. Narrowing IT to 404 would make this + // file's anonymity proof fail for reasons belonging to another proof, which + // is what its own comment warns against. + it('[#7867] a member hitting the action with a NONEXISTENT record id gets 404 RECORD_NOT_FOUND', async () => { + const r = await stack.apiAs(memberToken, 'POST', ACTION, { params: {} }); + + expect(r.status, 'a ghost record id must answer 404, not a 400 from further down the pipeline').toBe(404); + const body = (await r.json()) as Record; + const err = (isRecord(body.error) ? body.error : {}) as Record; + expect(err.code).toBe('RECORD_NOT_FOUND'); + // The dispatcher's wrapper echoes the status it served; both are asserted so + // a body that says 404 while the response says 400 cannot pass either. + expect(err.httpStatus).toBe(404); + // …and specifically NOT the shape this fixture used to tolerate in silence. + expect(String(err.message ?? '')).not.toMatch(/not bound for this operation/); + expect(String(err.message ?? '')).not.toMatch(/HookConditionError/); + }); + + it('[#7867] …the SAME answer the REST data surface gives for that same id', async () => { + // The comparison that made #7867 measurable in the first place: one id, one + // object, one process, one second — and two surfaces that disagreed + // (`/actions` → 400, `/data` → 404). Both are asserted here, so they cannot + // drift apart again without this gate saying so. + const ghostId = ACTION.slice(ACTION.lastIndexOf('/') + 1); + const rest = await stack.apiAs(memberToken, 'PATCH', `/data/showcase_task/${ghostId}`, { done: true }); + + expect(rest.status).toBe(404); + const body = (await rest.json()) as Record; + // `@objectstack/rest` answers the flat envelope; the dispatcher-mounted + // `/actions` answers its own wrapper. Different shells, one code — each read + // in its own shape, with no `??` chain across the two (#5632's rule, which + // this file already enforces for the 401 bodies). + expect(body.code).toBe('RECORD_NOT_FOUND'); + }); + // ── /automation (dispatcher-mounted; runtime domains/automation.ts) ───── // // The gate is DOMAIN-WIDE and sits ahead of the `isServiceServeable` probe on diff --git a/packages/runtime/src/sandbox/error-passthrough.test.ts b/packages/runtime/src/sandbox/error-passthrough.test.ts index 43720d40c8..c3ffea21d0 100644 --- a/packages/runtime/src/sandbox/error-passthrough.test.ts +++ b/packages/runtime/src/sandbox/error-passthrough.test.ts @@ -160,3 +160,80 @@ describe('#3918 follow-up — VM error → host', () => { expect(err.fields).toBeUndefined(); }); }); + +/* ──────────────────────────────────────────────────────────────────────────── + * [#7867] `status` — the third allowlisted property, and why it had to join + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#7867] an error that names its own HTTP status keeps it across the boundary', () => { + /** The exact shape `@objectstack/core`'s `recordNotFoundError` produces. */ + const recordNotFound = () => + Object.assign(new Error('Record ghost not found in invoice'), { + code: 'RECORD_NOT_FOUND', + status: 404, + object: 'invoice', + }); + + const UNCAUGHT = `await ctx.api.object('invoice').update({ id: 'ghost' }); return { never: true };`; + + it('carries `status` onto the thrown SandboxError, so /actions can serve 404', async () => { + // Without this the action surface answered the RIGHT diagnosis at the + // WRONG status — `{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }` — + // because `domains/actions.ts`'s "an error that NAMES its own HTTP + // status is asking to be served with it" branch reads `.status`, and + // the number never arrived. 404 and 400 mean different things to a + // client's retry and cache policy, so a half-carried error is not a fix. + const err = await run(UNCAUGHT, recordNotFound()).catch((e) => e); + + expect(err).toBeInstanceOf(SandboxError); + expect(err.code).toBe('RECORD_NOT_FOUND'); + expect(err.status).toBe(404); + }); + + it('a body can read `status` off a rejected write', async () => { + const r = await run( + `try { await ctx.api.object('invoice').update({ id: 'ghost' }); } + catch (e) { return { code: e.code, status: e.status }; } + return { never: true };`, + recordNotFound(), + ); + + expect(r.value).toEqual({ code: 'RECORD_NOT_FOUND', status: 404 }); + }); + + it('still carries NOTHING outside the allowlist — `object` does not cross', async () => { + // The allowlist widened by exactly one property, and the security + // assertion widens with it rather than being relaxed. `recordNotFound` + // also hangs an `object` off itself; it must not become readable. + const r = await run( + `try { await ctx.api.object('invoice').update({ id: 'ghost' }); } + catch (e) { return { object: e.object ?? null, keys: Object.keys(e) }; } + return { never: true };`, + recordNotFound(), + ); + + const v = r.value as any; + expect(v.object).toBeNull(); + expect(v.keys.sort()).toEqual(['code', 'message', 'name', 'status']); + }); + + it('leaves an error with no status alone — `status` stays undefined', async () => { + // The contrast pin. A record `ValidationError` deliberately carries no + // `.status` (`validation-failure.ts`), which is what keeps it out of + // the classifier's status branch and on the 400 rejection exit. + const err = await run(UNCAUGHT, new ValidationError(FIELDS)).catch((e) => e); + + expect(err.code).toBe('VALIDATION_FAILED'); + expect(err.status).toBeUndefined(); + }); + + it('ignores a non-finite status rather than serving a nonsense one', async () => { + const err = await run( + `var e = new Error('nope'); e.code = 'WEIRD'; e.status = NaN; throw e;`, + new Error('unused'), + ).catch((e) => e); + + expect(err.code).toBe('WEIRD'); + expect(err.status).toBeUndefined(); + }); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 6302862040..7edf5986f2 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -284,8 +284,8 @@ export class QuickJSScriptRunner implements ScriptRunner { `function(e){ globalThis.__error = (e && e.message) ? (e.name + ': ' + e.message) : String(e); try { - globalThis.__errorInfo = (e && (e.code || e.fields || e['${SANDBOX_FAULT_PROP}'])) - ? JSON.stringify({ code: e.code, fields: e.fields, sandboxFault: e['${SANDBOX_FAULT_PROP}'] === true }) + globalThis.__errorInfo = (e && (e.code || e.fields || e.status || e['${SANDBOX_FAULT_PROP}'])) + ? JSON.stringify({ code: e.code, fields: e.fields, status: e.status, sandboxFault: e['${SANDBOX_FAULT_PROP}'] === true }) : undefined; } catch (_) { globalThis.__errorInfo = undefined; } }`; @@ -949,16 +949,29 @@ function safeJsonStringify(v: unknown): string { * style one: everything placed on the handle below becomes readable by * untrusted sandboxed code, and host errors routinely hang driver state, * connection details, or whole record payloads off themselves. Copying the - * error's own enumerable keys would leak all of it. Only these two are safe and - * useful — they are already destined for the HTTP client. + * error's own enumerable keys would leak all of it. Only these three are safe + * and useful — they are already destined for the HTTP client. * * Why they need to cross at all: a record `ValidationError` reaching a body via * `ctx.api.object(x).update(...)` used to arrive as bare `name`/`message`, so * its `fields[]` was gone before any dispatcher exit could map it (#3918 * follow-up) — a form action could only ever show prose, never highlight the * offending input. + * + * [#7867] `status` joined for the same reason one card later, on the same call + * shape. `ctx.api.object(x).update({ id, … })` against an id that names no row + * now throws the repo's one `RECORD_NOT_FOUND` — `code` 'RECORD_NOT_FOUND', + * `status` 404 — and `code` alone crossed, so the action surface answered + * `{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }`: the right diagnosis served + * with the wrong status, which is the half-fix a client cannot act on (404 and + * 400 mean different things to a retry policy and to a cache). + * + * `domains/actions.ts`'s classifier already honours a `.status` FIRST — "an + * error that NAMES its own HTTP status is asking to be served with it" — so + * nothing downstream needed teaching; the number simply never arrived. A + * number, like `code`, carries no host state. */ -const SANDBOX_ERROR_PASSTHROUGH = ['code', 'fields'] as const; +const SANDBOX_ERROR_PASSTHROUGH = ['code', 'fields', 'status'] as const; /** * Marshal a HOST error into the VM as a rejectable QuickJS error handle, @@ -967,7 +980,7 @@ const SANDBOX_ERROR_PASSTHROUGH = ['code', 'fields'] as const; * The caller owns the returned handle and must dispose it. */ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle { - const e = err as { name?: string; message?: string; code?: unknown; fields?: unknown }; + const e = err as { name?: string; message?: string; code?: unknown; fields?: unknown; status?: unknown }; const errH = err instanceof Error ? vm.newError({ name: e.name || 'Error', message: e.message ?? '' }) : vm.newError({ name: 'Error', message: String(err) }); @@ -984,6 +997,14 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle { vm.setProp(errH, 'fields', h); h.dispose(); } + // [#7867] Finite numbers only — a status is a small integer or it is not a + // status, and `NaN`/`Infinity` would not survive the JSON side-channel that + // carries it back out. + if (typeof e?.status === 'number' && Number.isFinite(e.status)) { + const h = vm.newNumber(e.status); + vm.setProp(errH, 'status', h); + h.dispose(); + } // [#4431] Mark the sandbox's OWN faults so the pump loop can tell them // apart from a user throw after the VM has flattened both to a string. if (err instanceof SandboxError) { @@ -1208,12 +1229,21 @@ export class SandboxError extends Error { * input instead of showing the message alone. */ readonly fields?: unknown[]; + /** + * [#7867] The HTTP status the error that crossed OUT of the VM named for + * itself — 404 for the engine's `RECORD_NOT_FOUND`, 403 for a permission + * refusal. `domains/actions.ts` serves it directly ("an error that NAMES its + * own HTTP status is asking to be served with it"); without it a by-id write + * against a nonexistent record was answered `RECORD_NOT_FOUND` at status 400. + */ + readonly status?: number; constructor(message: string, innerMessage?: string, info?: SandboxErrorInfo) { super(message); this.name = 'SandboxError'; this.innerMessage = innerMessage; if (info?.code) this.code = info.code; if (info?.fields) this.fields = info.fields; + if (typeof info?.status === 'number') this.status = info.status; } } @@ -1221,6 +1251,8 @@ export class SandboxError extends Error { export interface SandboxErrorInfo { code?: string; fields?: unknown[]; + /** [#7867] See {@link SandboxError.status}. */ + status?: number; /** * [#4431] The error that crossed `__error` was the SANDBOX's own fault — a * denied capability, an unavailable `ctx.api`, a marshalling failure — not @@ -1252,12 +1284,16 @@ function readErrorInfo(vm: QuickJSContext): SandboxErrorInfo | undefined { } catch { return undefined; } - const p = parsed as { code?: unknown; fields?: unknown; sandboxFault?: unknown }; + const p = parsed as { code?: unknown; fields?: unknown; status?: unknown; sandboxFault?: unknown }; const info: SandboxErrorInfo = {}; if (typeof p?.code === 'string' && p.code) info.code = p.code; if (Array.isArray(p?.fields)) info.fields = p.fields; + // [#7867] `Number.isFinite` rather than a bare `typeof`: an out-of-range + // number JSON-round-trips to `null`, and `NaN` would satisfy `typeof` while + // making `errorFromThrown` emit a nonsense status line. + if (typeof p?.status === 'number' && Number.isFinite(p.status)) info.status = p.status; if (p?.sandboxFault === true) info.sandboxFault = true; - return info.code || info.fields || info.sandboxFault ? info : undefined; + return info.code || info.fields || info.status !== undefined || info.sandboxFault ? info : undefined; } /**