diff --git a/.changeset/hook-condition-fail-loud.md b/.changeset/hook-condition-fail-loud.md new file mode 100644 index 0000000000..79a45b101b --- /dev/null +++ b/.changeset/hook-condition-fail-loud.md @@ -0,0 +1,86 @@ +--- +"@objectstack/objectql": major +--- + +feat(objectql)!: a hook `condition` the platform cannot evaluate now ABORTS the operation (#4775) + +**Breaking.** A declarative hook whose `condition` cannot be evaluated used to +emit a `logger.warn` and `return false` — the hook simply did not fire. Existing +hooks that have been getting by on that silent skip will now **fail the write**. +That is the point of the change, not a side effect: those conditions were never +enforcing anything, and the failure is how you find out. + +## What changed + +"The condition said no" and "the platform could not work out what the condition +says" used to collapse into one outcome, and that one outcome carries **opposite** +risks depending on the hook: + +- a `before*` guard ("hold this write when the condition is met") swallowed into + `false` **lets through** a write it was declared to stop; +- an `after*` audit ("leave a trace when the condition is met") swallowed into + `false` **drops** a row nobody will go looking for, because nobody knows it + should exist. + +So an unevaluable condition is `declared ≠ enforced`, and it is now resolved the +way #4649 already resolved it for validation predicates one module over: reject +loudly, naming the hook and the key that would not resolve. The rejection is a +`HookConditionError` (exported), carrying `hook` / `object` / `event` / +`condition` / `reason` / `fault` / `missingKey` machine-readably. + +`before*` and `after*` take the **same** direction, knowingly: a typo in an +`afterUpdate` audit condition fails the write it was only watching. One rule, one +answer — the platform does not grow a hidden second rule that makes the failure +direction depend on the event name. + +A condition that never **compiled** aborts too. Its old treatment +(`condition ignored`) was the worse half of the swallow: the gate disappeared +entirely, so a declared guard let every write through and an audit fired on all +of them. It is reported at invocation rather than at bind time, so one broken +hook cannot wedge boot for an app nobody is writing to. + +## What did NOT change + +- A condition that evaluates **FALSE** is still just a skip, and the write still + succeeds. Only *unevaluable* is new. +- `onError` (`abort` / `log`) is untouched and is deliberately **not** in this + path. It governs a handler that threw; the condition gate runs before the + handler is ever reached. Routing a condition fault through it would let + `onError: 'log'` resurrect the exact silent skip this change abolishes, and + would mint a third set of semantics for one word. `retryPolicy` and `async` + are outside it for the same reason. + +## Predicate (`multi: true`) bulk writes (#4800) + +A bulk write matches N rows and fires the hook **once**, so `previous` is unbound +and `record` is the bare payload — there is no single prior record, and +materialising declared fields to `null` would state something false about all N. +Fail loud takes **no exception** here, but the message is a diagnosis rather than +a riddle: it names the hook, says *this is a predicate bulk write and there is no +single prior record*, and gives the route that works (rewrite without `previous`, +or target the write at one record by id). + +It deliberately does **not** offer "use a record-change flow trigger instead": +that trigger subscribes to these same lifecycle hooks, so on a bulk write it +fires once with `previous` undefined too — verified against +`trigger-record-change` and the engine, not assumed. Pointing at it would have +made this very message the next `declared ≠ delivered`. + +An **undeclared** key on a bulk write still gets the ordinary typo message — that +one really is a misspelling, and calling it a batch problem would send the author +to fix a field that is spelled correctly. + +## Migrating + +Run your app and watch for `HookConditionError`. Each one names the hook and the +key. The usual causes, in order of frequency: + +- **a misspelled or retired field** — fix the condition, or declare the field; +- **an unguarded `null` comparison** (`record.spent > record.budget`) — guard + with `!= null`. Note `has(x)` does **not** do this: a declared field holding + `null` is still PRESENT, so `has(x)` is `true` and the ordering comparison + still faults; +- **`previous` on a bulk write** — rewrite without `previous`, or write by id; +- **a bare identifier** (`done == true`) — hook conditions are `record`-scoped, + so write `record.done == true`. Flow/automation conditions, which flatten + fields to top level, are a different surface and are unaffected. diff --git a/packages/objectql/src/cel-fault.ts b/packages/objectql/src/cel-fault.ts new file mode 100644 index 0000000000..477e366cf2 --- /dev/null +++ b/packages/objectql/src/cel-fault.ts @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Read a CEL fault the way the AUTHOR needs to read it. + * + * Shared by the two surfaces that evaluate a CEL expression "against the + * record" and reject the write when they cannot: object-level validation + * predicates (`validation/rule-validator.ts`, #4649) and declarative hook + * `condition`s (`hook-wrappers.ts`, #4775). Both were told to reuse ONE error + * shape, and the only durable way to keep two messages worded alike is to stop + * writing them twice — the same argument that put {@link + * ./declared-fields.js#materializeDeclaredFields} in front of both evaluators. + * + * This module has no opinion about what a caller does with a fault. It answers + * three questions and hands back a sentence: + * + * 1. What broke, in one line? (`summary`) + * 2. Did the expression read a key the record does not carry, and which? + * (`missingKey` — after materialisation this can only mean an UNDECLARED + * key, i.e. an author typo or a retired field) + * 3. Did it name a ROOT that is not in scope at all? (`unknownVariable` — + * the fault an unbound `previous` produces, which is a different + * diagnosis from a missing key on a bound root) + * 4. Did it compare/order a `null`? (`nullOverload` — the other way a + * predicate over a TOTAL record still faults) + */ + +/** The `{ kind, message }` failure `@objectstack/formula` resolves. */ +export interface CelFault { + kind: string; + message: string; +} + +/** `No such key: ` is cel-js's word for "the expression read something the + * record does not carry" — the single most useful fact to put in front of the + * author, since after materialisation it can only mean an UNDECLARED key. */ +const NO_SUCH_KEY_RE = /No such key:\s*([A-Za-z_$][\w$]*)/; + +/** + * The OTHER way an expression written against a total record still faults: an + * ordering comparison (`<`, `>`, `<=`, `>=`) or arithmetic over a value that is + * `null`. CEL has no overload for it, so the whole expression aborts. + * + * This one deserves its own sentence because the obvious guard does not work: + * `has(x)` is TRUE for a declared field holding `null` (CEL asks whether the key + * is PRESENT, not whether it has a usable value), so `has(a) && has(b) && a < b` + * still faults the moment either is null — on any driver that returns its NULL + * columns, which is most of them. Such a rule never enforced anything on those + * rows; #4649 is what makes that visible instead of silent. + */ +const NULL_OVERLOAD_RE = /no such overload/i; + +/** + * `Unknown variable: ` is what cel-js says when a ROOT identifier is not + * in the scope at all — as opposed to `No such key`, which means the root + * resolved and the key under it did not. + * + * Both evaluators bind exactly two roots, `record` and `previous`, and bind + * `previous` only when the record's prior state is actually in hand. So this + * fault has one meaning worth spelling out: the expression asked for a binding + * that this operation does not have. Kept apart from {@link missingKeyOf} + * because the two need OPPOSITE advice — a missing key says "you named a field + * that isn't declared", an unknown variable says "the field is fine; the thing + * you hung it off isn't available here". + */ +const UNKNOWN_VARIABLE_RE = /Unknown variable:\s*([A-Za-z_$][\w$]*)/; + +/** + * One-line summary of a CEL fault. The engine appends a source excerpt and a + * caret line to `message`, which is right for a log and wrong for an API error, + * so only the first line travels. + */ +export function faultSummary(error: CelFault): string { + const first = String(error.message ?? '').split('\n')[0]!.trim(); + return `${error.kind}: ${first || 'unknown error'}`; +} + +/** The key a `No such key: ` fault names, or `undefined`. */ +export function missingKeyOf(error: CelFault): string | undefined { + return NO_SUCH_KEY_RE.exec(String(error.message ?? ''))?.[1]; +} + +/** The root identifier an `Unknown variable: ` fault names, or `undefined`. */ +export function unknownVariableOf(error: CelFault): string | undefined { + return UNKNOWN_VARIABLE_RE.exec(String(error.message ?? ''))?.[1]; +} + +/** True when the fault is the null-comparison overload fault (and not a + * missing key, which is always the more specific diagnosis). */ +export function isNullOverloadFault(error: CelFault): boolean { + const raw = String(error.message ?? ''); + return !missingKeyOf(error) && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw); +} + +/** What the two surfaces call the thing that faulted, so one helper can write + * both sentences without either side inventing its own phrasing. */ +export interface CelFaultSubject { + /** How the expression is named in prose: `'predicate'`, `'condition'`, … */ + what: string; + /** How to fix an undeclared key, e.g. `"fix the rule's condition, or declare the field"`. */ + undeclaredKeyFix: string; +} + +export interface CelFaultDescription { + /** `kind: first line` — safe to put in an API error. */ + summary: string; + /** The undeclared key the expression read, when that is the fault. */ + missingKey?: string; + /** The unbound ROOT the expression named, when that is the fault. */ + unknownVariable?: string; + /** True when the fault is the `null` ordering/arithmetic overload. */ + nullOverload: boolean; + /** A trailing sentence explaining the fault, or `''` when we have nothing + * more specific to say than {@link CelFaultDescription.summary}. Starts with + * a leading space so it appends directly onto a message. */ + detail: string; +} + +/** + * Turn a raw CEL fault into the facts + the sentence an author can act on. + * The wording is deliberately identical across surfaces; only `what` and the + * fix clause differ. + */ +export function describeCelFault(error: CelFault, subject: CelFaultSubject): CelFaultDescription { + const summary = faultSummary(error); + const missingKey = missingKeyOf(error); + const unknownVariable = missingKey ? undefined : unknownVariableOf(error); + const nullOverload = isNullOverloadFault(error); + let detail = ''; + if (missingKey) { + detail = + ` The ${subject.what} reads '${missingKey}', which this object does not declare` + + ` — ${subject.undeclaredKeyFix}.`; + } else if (unknownVariable) { + detail = + ` The ${subject.what} reads '${unknownVariable}', which is not bound for this operation` + + ` — the scope holds 'record', plus 'previous' only when the record's prior state is in hand.`; + } else if (nullOverload) { + detail = + ` The ${subject.what} compares a value that is null. Guard it with '!= null'` + + ` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`; + } + return { + summary, + ...(missingKey ? { missingKey } : {}), + ...(unknownVariable ? { unknownVariable } : {}), + nullOverload, + detail, + }; +} diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index e188b8bfec..a59b5c1c54 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -56,7 +56,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati // Hook binder & wrappers (declarative-metadata → engine glue) export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; -export { wrapDeclarativeHook } from './hook-wrappers.js'; +export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; export type { WrapDeclarativeOptions } from './hook-wrappers.js'; // Validation diff --git a/packages/objectql/src/hook-binder.test.ts b/packages/objectql/src/hook-binder.test.ts index e54fae46f7..d4fd0b93ab 100644 --- a/packages/objectql/src/hook-binder.test.ts +++ b/packages/objectql/src/hook-binder.test.ts @@ -3,7 +3,7 @@ import { describe, it, expect, vi } from 'vitest'; import { ObjectQL } from './engine.js'; import { bindHooksToEngine } from './hook-binder.js'; -import { wrapDeclarativeHook } from './hook-wrappers.js'; +import { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; import type { Hook, HookContext } from '@objectstack/spec/data'; function makeEngine() { @@ -377,8 +377,15 @@ describe('wrapDeclarativeHook', () => { expect(calls).toEqual(['done']); // awaited despite async=true }); - it('logs and treats invalid condition formulas as skipping', async () => { - const warn = vi.fn(); + it('rejects the operation when the condition formula does not compile', async () => { + // [#4775] This test used to accept EITHER outcome ("ignored at compile time + // (handler runs) or evaluated false (skipped) … just assert we didn't + // crash"). That latitude was the defect: "condition ignored" DELETED the + // gate, so a hook declared to run conditionally ran on every write, and the + // only trace was a `warn`. A condition that cannot compile can never be + // evaluated, so the hook can neither run nor be skipped honestly — the + // operation is rejected instead, naming the hook. + const error = vi.fn(); const calls: string[] = []; const meta: Hook = { name: 'badcond', object: 'a', events: ['beforeInsert'], priority: 100, @@ -386,12 +393,17 @@ describe('wrapDeclarativeHook', () => { handler: () => { calls.push('ran'); }, }; const wrapped = wrapDeclarativeHook(meta, meta.handler as any, { - logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + logger: { debug: () => {}, info: () => {}, warn: () => {}, error }, }); - await wrapped(makeCtx()); - expect(warn).toHaveBeenCalled(); - // Either ignored at compile time (handler runs) or evaluated false - // (skipped). Both are valid; just assert we didn't crash. - expect(calls.length === 0 || calls[0] === 'ran').toBe(true); + + const err = await wrapped(makeCtx()).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.reason).toBe('uncompilable'); + expect(err.message).toContain("Hook 'badcond'"); + expect(calls).toEqual([]); + // Reported at bind time too, at error level — an operator sees the broken + // hook before the first write trips over it. + expect(error).toHaveBeenCalled(); }); }); diff --git a/packages/objectql/src/hook-condition-fail-loud.test.ts b/packages/objectql/src/hook-condition-fail-loud.test.ts new file mode 100644 index 0000000000..f7f600f58a --- /dev/null +++ b/packages/objectql/src/hook-condition-fail-loud.test.ts @@ -0,0 +1,504 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4775] A hook `condition` the platform cannot evaluate ABORTS the operation. + * + * Before this, "the condition said no" and "the platform could not work out + * what the condition says" produced the same outcome: a `logger.warn` at a + * level most deployments read as noise, and `return false`. One result, two + * opposite risks — + * + * - a `before*` guard swallowed into `false` LETS THROUGH a write it was + * declared to stop; + * - an `after*` audit swallowed into `false` DROPS a row nobody will go + * looking for, because nobody knows it should exist. + * + * Maintainer's ruling (option B on the issue, superseding the C recorded in an + * earlier comment): fail loud GLOBALLY. `before*` and `after*` take the same + * direction, knowingly — a typo in an `afterUpdate` audit condition fails the + * write it was only watching. One rule, one answer; no hidden second rule that + * makes the failure direction depend on the event name. + * + * The three things this file pins, in the order they are easiest to get wrong: + * + * 1. UNEVALUABLE ⇒ rejected, naming the hook and the key (#4649's shape, + * shared through `cel-fault.ts`); + * 2. FALSE ⇒ still just a skip, and the write still SUCCEEDS. This is the + * change's blast radius, and the single most likely thing to break by + * accident; + * 3. `onError` is untouched — a condition fault is raised BEFORE the handler + * exists to fail, so `onError: 'log'` cannot resurrect the silent skip. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; +import type { Hook, HookContext } from '@objectstack/spec/data'; + +const TASK_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, + archived: { name: 'archived', label: 'Archived', type: 'boolean' as const }, +}; +const taskObject = { name: 'hook_task', label: 'Task', fields: TASK_FIELDS }; +const qlStub = { getObject: (n: string) => (n === 'hook_task' ? taskObject : undefined) }; + +function makeCtx(overrides: Partial = {}): HookContext { + return { + object: 'hook_task', + event: 'afterUpdate', + input: { id: 't1', data: { done: true } }, + previous: { id: 't1', title: 'Ship it', status: 'todo', done: false }, + ql: qlStub, + ...overrides, + } as unknown as HookContext; +} + +function makeHook(condition: string, extra: Partial = {}): Hook { + return { + name: 'audit_hook', object: 'hook_task', events: ['afterUpdate'], priority: 100, + condition, handler: () => {}, + ...extra, + } as unknown as Hook; +} + +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +/* ──────────────────────────────────────────────────────────────────────────── + * 1. Unevaluable ⇒ the operation is rejected, and the message is usable + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775] an unevaluable condition aborts the operation', () => { + it('throws a HookConditionError naming the hook and the undeclared key', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "done"'), + (async () => { calls.push('ran'); }) as any, + { logger: silentLogger }, + ); + + const err = await wrapped(makeCtx()).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.hook).toBe('audit_hook'); + expect(err.object).toBe('hook_task'); + expect(err.event).toBe('afterUpdate'); + expect(err.reason).toBe('unevaluable'); + expect(err.missingKey).toBe('stauts'); + expect(err.condition).toBe('record.stauts == "done"'); + // Names the hook, names the key, and says what to do about it — the same + // three facts `unevaluableRuleError` puts in a validation rejection. + expect(err.message).toContain("Hook 'audit_hook'"); + expect(err.message).toContain("reads 'stauts'"); + expect(err.message).toContain('does not declare'); + expect(err.message).toContain("fix the hook's condition, or declare the field"); + // And the handler never ran. + expect(calls).toEqual([]); + }); + + it("words the rejection like #4649's, so an author who has met one can read the other", async () => { + // The two surfaces share `cel-fault.ts`; this pins the shared sentence + // rather than a copy of it. + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "done"'), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(makeCtx()).then(() => null, (e) => e); + const { evaluateValidationRules } = await import('./validation/rule-validator.js'); + + let ruleMessage = ''; + try { + evaluateValidationRules( + { + name: 'hook_task', fields: TASK_FIELDS, + validations: [{ + type: 'script' as const, + name: 'broken_rule', + condition: { dialect: 'cel', source: 'record.stauts == "done"' }, + message: 'nope', + }], + } as any, + { done: true }, 'update', + { previous: { id: 't1', title: 'x', status: 'todo', done: false } } as any, + ); + } catch (e: any) { + ruleMessage = e?.fields?.[0]?.message ?? String(e?.message ?? ''); + } + expect(ruleMessage).not.toBe(''); + + const shared = "reads 'stauts', which this object does not declare"; + expect(err.message).toContain(shared); + expect(ruleMessage).toContain(shared); + }); + + it('explains a null comparison the same way the validation side does', async () => { + // `archived` is declared and materialised to null; ordering over null has + // no CEL overload, and `has()` does NOT guard it. + const wrapped = wrapDeclarativeHook( + makeHook('record.archived > 1'), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(makeCtx()).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.message).toMatch(/Guard it with '!= null'/); + expect(err.message).toMatch(/has\(x\) is true/); + }); + + it('logs nothing that pretends the hook was merely "skipped"', async () => { + const warn = vi.fn(); + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "done"'), (async () => {}) as any, + { logger: { ...silentLogger, warn } }, + ); + await wrapped(makeCtx()).catch(() => {}); + expect(warn.mock.calls.filter(([m]) => /treating as false/.test(String(m)))).toEqual([]); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 2. before* and after* take the SAME direction (acceptance #2 — no C tiering) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775] `before*` and `after*` fail in the SAME direction', () => { + const BROKEN = 'record.stauts == "done"'; + + it('rejects on a before* guard hook', async () => { + const wrapped = wrapDeclarativeHook( + makeHook(BROKEN, { name: 'guard_hook', events: ['beforeUpdate'] }), + (async () => {}) as any, { logger: silentLogger }, + ); + await expect(wrapped(makeCtx({ event: 'beforeUpdate' }))).rejects.toBeInstanceOf(HookConditionError); + }); + + it('rejects on an after* audit hook — the accepted cost of one rule, one answer', async () => { + const wrapped = wrapDeclarativeHook( + makeHook(BROKEN, { name: 'audit_hook', events: ['afterUpdate'] }), + (async () => {}) as any, { logger: silentLogger }, + ); + await expect(wrapped(makeCtx({ event: 'afterUpdate' }))).rejects.toBeInstanceOf(HookConditionError); + }); + + it('rejects on an after* hook even when it is `async` (fire-and-forget)', async () => { + // The gate runs BEFORE the fire-and-forget branch, so the caller still + // sees the rejection instead of it vanishing into a detached promise. + // `showcase_audit_task_completion` is exactly this shape. + const wrapped = wrapDeclarativeHook( + makeHook(BROKEN, { async: true, onError: 'log' } as any), + (async () => {}) as any, { logger: silentLogger }, + ); + await expect(wrapped(makeCtx())).rejects.toBeInstanceOf(HookConditionError); + }); + + it('produces the same `reason` for both, with no event-derived tiering', async () => { + const read = async (events: string[], event: string) => { + const wrapped = wrapDeclarativeHook( + makeHook(BROKEN, { events } as any), (async () => {}) as any, { logger: silentLogger }, + ); + return wrapped(makeCtx({ event } as any)).then(() => null, (e) => e); + }; + const before = await read(['beforeUpdate'], 'beforeUpdate'); + const after = await read(['afterUpdate'], 'afterUpdate'); + expect(before.reason).toBe(after.reason); + expect(before.missingKey).toBe(after.missingKey); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 3. FALSE is still just FALSE (acceptance #3 — the easiest thing to break) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775] a condition that evaluates FALSE still just skips', () => { + it('does not run the handler and does not throw', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.done == false'), + (async () => { calls.push('ran'); }) as any, + { logger: silentLogger }, + ); + + await expect(wrapped(makeCtx())).resolves.toBeUndefined(); + expect(calls).toEqual([]); + }); + + it('still records the skip as a `condition` skip, not an error', async () => { + const recordSkip = vi.fn(); + const wrapped = wrapDeclarativeHook( + makeHook('record.done == false'), (async () => {}) as any, + { + logger: silentLogger, + metrics: { recordSkip, recordRetry: () => {}, recordExecution: () => {} } as any, + }, + ); + await wrapped(makeCtx()); + expect(recordSkip).toHaveBeenCalledWith(expect.objectContaining({ hook: 'audit_hook' }), 'condition'); + }); + + it('a falsy comparison against a materialised null is FALSE, not a fault', async () => { + // `archived` is declared and absent from the stored row → null. `== true` + // is a legitimate NO, and must not be confused with "unevaluable". + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.archived == true'), + (async () => { calls.push('ran'); }) as any, + { logger: silentLogger }, + ); + await expect(wrapped(makeCtx())).resolves.toBeUndefined(); + expect(calls).toEqual([]); + }); + + it('through a real engine: a FALSE condition leaves the write succeeding', async () => { + const ran: string[] = []; + const engine = await bootEngine([{ + name: 'never_fires', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: 'record.done == false', + handler: () => { ran.push('x'); }, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const updated: any = await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + + expect(updated.done).toBe(true); + expect(ran).toEqual([]); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 4. `onError` is untouched (acceptance #5) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775] a condition fault never enters `onError`', () => { + it("`onError: 'log'` does NOT swallow a condition fault", async () => { + const error = vi.fn(); + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "done"', { onError: 'log' } as any), + (async () => {}) as any, + { logger: { ...silentLogger, error } }, + ); + + await expect(wrapped(makeCtx())).rejects.toBeInstanceOf(HookConditionError); + // The `onError` branch logs '[hook] handler failed (onError=log; suppressing)'. + // The condition never reached it. + expect(error.mock.calls.filter(([m]) => /onError=log/.test(String(m)))).toEqual([]); + }); + + it("`onError: 'log'` still swallows a HANDLER throw, exactly as before", async () => { + const error = vi.fn(); + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true', { onError: 'log' } as any), + (async () => { throw new Error('handler boom'); }) as any, + { logger: { ...silentLogger, error } }, + ); + + await expect(wrapped(makeCtx())).resolves.toBeUndefined(); + expect(error.mock.calls.filter(([m]) => /onError=log/.test(String(m)))).toHaveLength(1); + }); + + it('a condition fault is not retried by `retryPolicy` either', async () => { + const recordRetry = vi.fn(); + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "done"', { retryPolicy: { maxRetries: 3, backoffMs: 0 } } as any), + (async () => {}) as any, + { + logger: silentLogger, + metrics: { recordSkip: () => {}, recordRetry, recordExecution: () => {} } as any, + }, + ); + await wrapped(makeCtx()).catch(() => {}); + expect(recordRetry).not.toHaveBeenCalled(); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 5. The predicate bulk write gets a DIAGNOSIS, not `No such key` (#4800 / B1) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775 / #4800 B1] a predicate bulk write gets its own diagnosis', () => { + const bulkCtx = (data: Record) => makeCtx({ + previous: undefined, + input: { data, options: { multi: true } }, + } as any); + + it('`previous` on a bulk update names the batch instead of saying "No such key"', async () => { + const wrapped = wrapDeclarativeHook( + makeHook('previous.done != true && record.done == true'), + (async () => {}) as any, { logger: silentLogger }, + ); + + const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.predicateBulkWrite).toBe(true); + expect(err.message).toContain("Hook 'audit_hook'"); + expect(err.message).toContain('PREDICATE bulk write (multi: true)'); + expect(err.message).toContain('no single prior record to bind'); + expect(err.message).toContain('target the write at one record (update by id)'); + // The default riddle must NOT be the whole story the author gets. + expect(err.message).not.toMatch(/which this object does not declare/); + }); + + it('does NOT point at a record-change flow trigger as the way out', async () => { + // VERIFIED (probe, 2026-08-03): the record-change trigger subscribes to + // these same lifecycle hooks, so on a `multi: true` update it fires ONCE + // with `ctx.previous` undefined — same limitation, not an escape hatch. + // Naming it here would make this very message the next + // `declared ≠ delivered`, which is the defect the change exists to remove. + const wrapped = wrapDeclarativeHook( + makeHook('previous.done != true'), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + expect(err.message).toContain('A record-change flow trigger is NOT a way around this'); + }); + + it('a DECLARED field the bulk payload does not set gets the batch diagnosis too', async () => { + // Same root cause: no prior row in hand, so `record` is the bare payload + // and cannot be made total. `No such key: budget` alone reads as a typo, + // which would send the author to fix a field that is spelled correctly. + const wrapped = wrapDeclarativeHook( + makeHook('record.archived == true'), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + + expect(err.predicateBulkWrite).toBe(true); + expect(err.message).toContain("'archived' IS declared on this object"); + expect(err.message).toContain('PREDICATE bulk write (multi: true)'); + }); + + it('an UNDECLARED key on a bulk write is still reported as a TYPO, not as the batch', async () => { + // The batch diagnosis would be actively wrong here — this one really is a + // misspelling, and it is misspelled on a single-record write too. + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "x"'), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + + expect(err.message).toContain("reads 'stauts', which this object does not declare"); + expect(err.message).not.toContain('PREDICATE bulk write'); + }); + + it('fail loud takes NO exception for the batch — the bulk write still fails', async () => { + const engine = await bootEngine([{ + name: 'bulk_breaker', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: 'previous.done != true && record.done == true', + handler: () => {}, + } as unknown as Hook]); + + await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + await engine.insert('hook_task', { title: 'B', status: 'todo', done: false }); + + await expect( + engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any), + ).rejects.toThrow(/PREDICATE bulk write/); + }); + + it('a single-record write of the SAME hook still succeeds', async () => { + const ran: string[] = []; + const engine = await bootEngine([{ + name: 'bulk_breaker', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: 'previous.done != true && record.done == true', + handler: () => { ran.push('audited'); }, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const updated: any = await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + + expect(updated.done).toBe(true); + expect(ran).toEqual(['audited']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 6. A condition that never compiled (the worse half of the old swallow) + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4775] a condition that does not compile aborts too', () => { + it('rejects rather than firing the hook unconditionally', async () => { + const calls: string[] = []; + // Old behaviour: "condition formula failed to compile; condition ignored" + // → the gate vanished and the hook ran on EVERY write, as though no + // condition had been declared. That is the same `declared ≠ enforced`, + // one step earlier and pointing the other way. + const wrapped = wrapDeclarativeHook( + makeHook('record.done == = true'), + (async () => { calls.push('ran'); }) as any, + { logger: silentLogger }, + ); + + const err = await wrapped(makeCtx()).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.reason).toBe('uncompilable'); + expect(err.message).toContain("Hook 'audit_hook'"); + expect(err.message).toContain('does not compile'); + expect(calls).toEqual([]); + }); + + it('is reported at invocation, not at bind time — a broken hook cannot wedge boot', async () => { + // Wrapping must not throw; only running the operation does. + expect(() => + wrapDeclarativeHook(makeHook('record.done == = true'), (async () => {}) as any, { logger: silentLogger }), + ).not.toThrow(); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * Real-engine harness + * ──────────────────────────────────────────────────────────────────────────── */ + +function makeMemoryDriver(): any { + 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 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) { + 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) { 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) { + 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 beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +async function bootEngine(hooks: Hook[]): Promise { + const engine = new ObjectQL(); + engine.registerDriver(makeMemoryDriver(), true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + bindHooksToEngine(engine, hooks, { packageId: 'app:test', logger: silentLogger }); + return engine; +} diff --git a/packages/objectql/src/hook-condition-merged-record.test.ts b/packages/objectql/src/hook-condition-merged-record.test.ts index 31a1dc8c01..2622cda987 100644 --- a/packages/objectql/src/hook-condition-merged-record.test.ts +++ b/packages/objectql/src/hook-condition-merged-record.test.ts @@ -126,21 +126,19 @@ describe('[#4770] hook condition evaluates against stored ⊕ payload', () => { it('an UNDECLARED key stays unevaluable — materialisation does not paper over typos', async () => { const calls: string[] = []; - const { logger, conditionWarnings } = captureLogger(); + const { logger } = captureLogger(); // `dnoe` is the classic transposition of `done`. Nothing declares it, so // it must NOT be materialised to null and quietly answered "false". + // Since #4775 the unevaluable condition ABORTS the operation instead of + // being swallowed into `false` — the typo is reported, not absorbed. const wrapped = wrapDeclarativeHook( makeHook('record.dnoe == true', calls), (async () => { calls.push('ran'); }) as any, { logger }, ); - await wrapped(makeCtx()); - + await expect(wrapped(makeCtx())).rejects.toThrow(/No such key: dnoe/); expect(calls).toEqual([]); - const warns = conditionWarnings(); - expect(warns).toHaveLength(1); - expect(String(warns[0]![1]?.error)).toMatch(/No such key: dnoe/); }); it('fabricates nothing when the prior row is not in hand (predicate bulk update)', async () => { @@ -148,14 +146,20 @@ describe('[#4770] hook condition evaluates against stored ⊕ payload', () => { const { logger } = captureLogger(); // A `multi: true` update fetches no single prior row, so the persisted // state is unknown. Defaulting `done` to null here would not materialise an - // absent value — it would contradict N stored rows. + // absent value — it would contradict N stored rows. #4775: the condition is + // therefore unevaluable, and unevaluable now aborts the write. const wrapped = wrapDeclarativeHook( makeHook('record.done == null', calls), (async () => { calls.push('ran'); }) as any, { logger }, ); - await wrapped(makeCtx({ previous: undefined, input: { data: { status: 'x' } } } as any)); + await expect( + wrapped(makeCtx({ + previous: undefined, + input: { data: { status: 'x' }, options: { multi: true } }, + } as any)), + ).rejects.toThrow(/PREDICATE bulk write/); expect(calls).toEqual([]); }); diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index d6adac38f1..cd079fa4e1 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -122,19 +122,16 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { it('an UNDECLARED key on `previous` stays unevaluable — typos stay reportable', async () => { const calls: string[] = []; - const { logger, conditionWarnings } = captureLogger(); + const { logger } = captureLogger(); const wrapped = wrapDeclarativeHook( makeHook('previous.dnoe != true'), (async () => { calls.push('ran'); }) as any, { logger }, ); - await wrapped(makeCtx()); - + // #4775: "reportable" is now "rejects the operation", not "logs a warn". + await expect(wrapped(makeCtx())).rejects.toThrow(/No such key: dnoe/); expect(calls).toEqual([]); - const warns = conditionWarnings(); - expect(warns).toHaveLength(1); - expect(String(warns[0]![1]?.error)).toMatch(/No such key: dnoe/); }); it('never leaks materialised nulls back into the engine\'s ctx.previous', async () => { @@ -155,7 +152,7 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { it('leaves `previous` UNBOUND on insert — verbatim the validation side', async () => { const calls: string[] = []; - const { logger, conditionWarnings } = captureLogger(); + const { logger } = captureLogger(); // `rule-validator.ts` binds `previous` only for `mode: 'update'` with a // prior record in hand; on insert it passes `undefined`, which omits the // identifier from the CEL scope. Referencing it on an insert event is @@ -166,14 +163,15 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { { logger }, ); - await wrapped(makeCtx({ + // #4775: an unbound `previous` is unevaluable, and unevaluable rejects the + // insert rather than being swallowed into `false`. + await expect(wrapped(makeCtx({ event: 'beforeInsert', previous: undefined, input: { data: { done: true } }, - } as any)); + } as any))).rejects.toThrow(/Unknown variable: previous/); expect(calls).toEqual([]); - expect(conditionWarnings()).toHaveLength(1); }); it('a `record`-only condition on insert is unaffected by the added binding', async () => { @@ -200,14 +198,18 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { const { logger } = captureLogger(); // A `multi: true` update matched N rows and fires the hook ONCE; there is // no single prior record. Binding `{}` or `null` would make - // `previous.done != true` answer for rows nobody read. + // `previous.done != true` answer for rows nobody read. #4775/B1: it stays + // unbound AND the write is rejected — with a diagnosis, not `No such key`. const wrapped = wrapDeclarativeHook( makeHook(TRANSITION), (async () => { calls.push('ran'); }) as any, { logger }, ); - await wrapped(makeCtx({ previous: undefined, input: { data: { done: true } } } as any)); + await expect(wrapped(makeCtx({ + previous: undefined, + input: { data: { done: true }, options: { multi: true } }, + } as any))).rejects.toThrow(/PREDICATE bulk write/); expect(calls).toEqual([]); }); diff --git a/packages/objectql/src/hook-metrics.test.ts b/packages/objectql/src/hook-metrics.test.ts index 928dca2ef3..5cf6e84613 100644 --- a/packages/objectql/src/hook-metrics.test.ts +++ b/packages/objectql/src/hook-metrics.test.ts @@ -82,7 +82,13 @@ describe('hook metrics', () => { object: 'account', events: ['beforeInsert'], priority: 100, - condition: 'name == "skipme"', + // [#4775] Was a bare `name == "skipme"`. Hook conditions are + // `record`-scoped, so the bare identifier resolved to nothing and + // the expression FAULTED on every call — the "skip" this test + // asserted came from the old swallow (fault → warn → false), not + // from a condition that answered NO. `record.name` is the same + // intent, actually evaluated: 'acme' != 'skipme' ⇒ FALSE ⇒ skip. + condition: 'record.name == "skipme"', handler: async () => { /* noop */ }, }; bindHooksToEngine(engine, [hook], { packageId: 'p', metrics }); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index aef2e1b46d..83f9c5309e 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -19,6 +19,7 @@ import type { HookHandler } from './engine.js'; import { ExpressionEngine } from '@objectstack/formula'; import { noopHookMetricsRecorder, type HookMetricsRecorder, type HookMetricOutcome } from './hook-metrics.js'; import { materializeDeclaredFields } from './declared-fields.js'; +import { describeCelFault, type CelFault } from './cel-fault.js'; export interface WrapDeclarativeOptions { /** Logger for declarative-layer diagnostics (timeouts, retries, swallowed errors). */ @@ -39,24 +40,110 @@ const noopLogger = { error: () => {}, }; +/** + * A hook declared a `condition` and the platform could not work out its value + * (#4775). Thrown from the condition gate, which aborts the operation. + * + * ## Why an unevaluable condition is not `false` + * + * "The expression said no" and "the platform could not work out what the + * expression says" used to collapse into the same outcome — `logger.warn` and + * `return false` — and that single result carries OPPOSITE risks depending on + * the hook: + * + * - a `before*` guard ("hold this write when the condition is met") swallowed + * into `false` LETS THROUGH a write it was declared to stop; + * - an `after*` audit ("leave a trace when the condition is met") swallowed + * into `false` DROPS a record that was supposed to exist — an invisible + * absence, since nobody goes looking for a row they don't know should be + * there. + * + * So a declared condition the platform cannot evaluate is `declared ≠ + * enforced`, and the resolution is the one #4649 already chose for validation + * predicates one module over: reject loudly, naming the hook and the key that + * would not resolve. `before*` and `after*` take the SAME direction — + * deliberately, and knowing the cost: a typo in an `afterUpdate` audit + * condition fails the write it was only watching. One rule, one answer; the + * platform does not grow a hidden second rule that says "which way this fails + * depends on the event name". + * + * ## It is NOT an `onError` case + * + * `onError` (`abort` | `log`) governs a HANDLER that threw. The condition gate + * runs BEFORE the handler is ever reached, so this error is raised outside its + * reach on purpose: routing it through `onError` would let `onError: 'log'` + * resurrect exactly the silent skip this error exists to abolish, and would + * mint a third set of semantics for the same word. + */ +export class HookConditionError extends Error { + override readonly name = 'HookConditionError'; + /** The hook whose declared condition could not be evaluated. */ + readonly hook: string; + readonly object?: string; + readonly event?: string; + /** The condition source, verbatim as authored. */ + readonly condition: string; + /** `unevaluable` — compiled, but faulted on this record; + * `uncompilable` — never compiled at all. */ + readonly reason: 'unevaluable' | 'uncompilable'; + /** One-line CEL fault summary (`kind: message`). */ + readonly fault: string; + /** The key the expression read that the record does not carry, when known. */ + readonly missingKey?: string; + /** True when the operation is a predicate (`multi: true`) bulk write, whose + * N matched rows have no single prior state to bind (#4800/B1). */ + readonly predicateBulkWrite?: boolean; + + constructor(message: string, info: { + hook: string; + object?: string; + event?: string; + condition: string; + reason: 'unevaluable' | 'uncompilable'; + fault: string; + missingKey?: string; + predicateBulkWrite?: boolean; + }) { + super(message); + this.hook = info.hook; + this.object = info.object; + this.event = info.event; + this.condition = info.condition; + this.reason = info.reason; + this.fault = info.fault; + this.missingKey = info.missingKey; + this.predicateBulkWrite = info.predicateBulkWrite; + } +} + /** * Wrap a hook handler so it honours the declarative fields defined on * `HookSchema`. The wrapping order, from outermost to innermost, is: * - * 1. condition → skip when formula evaluates falsy + * 1. condition → skip when the formula evaluates FALSE; abort the operation + * when it cannot be evaluated at all (#4775) * 2. async → fire-and-forget (after* events only) * 3. retry → repeat on throw with backoff * 4. timeout → abort if handler runs too long * 5. onError → swallow when set to 'log' * + * Step 1 sits OUTSIDE steps 2–5 on purpose. `async`, `retryPolicy`, `timeout` + * and `onError` are all about running the HANDLER; the condition decides + * whether there is a handler run at all. So a `HookConditionError` is not + * retried, is not fire-and-forgotten, and is never softened by + * `onError: 'log'` — see that class for why routing it through `onError` was + * refused. + * * The condition formula is evaluated against two bindings, the same two a * validation predicate reads (#4784): * * - `record` — the record-shaped view built by {@link pickRecordPayload}: * for a write, the stored record overlaid with this write's payload, made - * total over the object's declared fields (#4770). Read events typically - * have no record yet, so a condition on a `beforeFind` will simply skip - * when no data is present. + * total over the object's declared fields (#4770). Read events carry no + * record at all, so since #4775 a `record.*` condition on a `beforeFind` + * REJECTS the read rather than quietly skipping — the hook was declared to + * gate on a field the event does not have, and that is an authoring error + * the platform can no longer paper over. * - `previous` — the record's pre-write state, built by * {@link pickPreviousPayload}: `ctx.previous`, made total over the same * declared fields. Absent (an unbound CEL identifier) whenever the prior @@ -84,37 +171,47 @@ export function wrapDeclarativeHook( }); // Pre-compile condition once so each invocation is cheap. - let conditionFn: ((record: any, previous: Record | undefined) => boolean) | undefined; + let conditionFn: ((ctx: HookContext) => boolean) | undefined; if (meta.condition) { // Accept either string shorthand or full Expression envelope. const expr: Expression = typeof meta.condition === 'string' ? { dialect: 'cel', source: meta.condition } : (meta.condition as Expression); if (expr.source && expr.source.trim()) { + const source = expr.source; const check = ExpressionEngine.compile(expr); if (check.ok) { - conditionFn = (record: any, previous: Record | undefined) => { + conditionFn = (ctx: HookContext) => { // `previous` is passed through as-is: `undefined` means the binding // is OMITTED from the CEL scope (see `buildScope` in // @objectstack/formula), which is exactly what the validation side // does when no prior record is in hand. Binding it to an empty // object instead would answer `previous.x == null` with a // fabricated "yes" for a record whose prior state is unknown. + const record = pickRecordPayload(ctx); + const previous = pickPreviousPayload(ctx); const r = ExpressionEngine.evaluate(expr, { record: record ?? {}, previous }); if (!r.ok) { - logger.warn('[hook] condition evaluation failed; treating as false', { - hook: meta.name, - condition: expr.source, - error: r.error.message, - }); - return false; + // [#4775] Fail LOUD. Not `false` — see `HookConditionError`. + throw unevaluableConditionError(meta, ctx, source, r.error, declaredFieldsFor(ctx)); } return Boolean(r.value); }; } else { - logger.warn('[hook] condition formula failed to compile; condition ignored', { + // [#4775] A condition that never compiled is the same defect one step + // earlier, and its old treatment ("condition ignored") was the WORSE + // half of the swallow: the gate disappeared entirely, so the hook fired + // on every write as though no condition had been declared. Reported at + // invocation rather than at bind time — a throw here would wedge boot + // for an app whose object nobody is even writing, and #4775's remit is + // to abort THE OPERATION that runs a broken hook. + const fault = check.error; + conditionFn = (ctx: HookContext) => { + throw uncompilableConditionError(meta, ctx, source, fault); + }; + logger.error('[hook] condition formula failed to compile; every operation on this hook\'s object will be rejected until it is fixed', { hook: meta.name, - condition: expr.source, + condition: source, error: check.error.message, }); } @@ -194,10 +291,11 @@ export function wrapDeclarativeHook( }; return async (ctx: HookContext): Promise => { - // 1. Condition gate + // 1. Condition gate. Throws (#4775) when the condition cannot be + // evaluated — deliberately OUTSIDE `runWithErrorPolicy`, so `onError` + // never sees it and cannot soften it back into a silent skip. if (conditionFn) { - const record = pickRecordPayload(ctx); - if (!conditionFn(record, pickPreviousPayload(ctx))) { + if (!conditionFn(ctx)) { logger.debug('[hook] skipped by condition', { hook: meta.name, object: ctx.object, @@ -342,6 +440,153 @@ function isInsertEvent(event: unknown): boolean { return event === 'beforeInsert' || event === 'afterInsert'; } +/** + * Is this operation a PREDICATE (`multi: true`) bulk write? + * + * The engine routes an update/delete to `updateMany`/`deleteMany` when the call + * carries no `id` and `options.multi` is set, and fires the lifecycle hook + * ONCE for the whole batch — `hookContext.previous` is never assigned and the + * payload is never merged with any stored row, because there are N stored rows + * and no single one of them is "the" prior state. + * + * Read off the same two facts the engine branches on (`input.id` absent + + * `options.multi`), which survive into the after-event context: `input.options` + * is rebuilt by `buildDriverOptions` as a COPY of the caller's bag, so `multi` + * is still there. + */ +function isPredicateBulkWrite(ctx: HookContext): boolean { + const input: any = ctx.input ?? {}; + if (!input || typeof input !== 'object') return false; + if (input.id !== undefined && input.id !== null && input.id !== '') return false; + const options: any = input.options; + return Boolean(options && typeof options === 'object' && options.multi); +} + +/** + * The rejection a condition that CANNOT BE EVALUATED produces (#4775). + * + * Mirrors `unevaluableRuleError` in `validation/rule-validator.ts` — same + * facts, same two explanatory sentences (both come out of the shared + * `cel-fault.ts`), so an author who has met one message can read the other. + * + * ## The predicate-bulk-write branch (#4800 / B1) + * + * A `multi: true` write matches N rows and fires the hook once, so two things + * the condition may legitimately name are simply not in hand: + * + * - `previous` — unbound, because there is no single prior record; + * - a DECLARED field the payload does not set — `record` is the payload + * alone here, since merging it with "the" stored row would mean picking one + * of N, and materialising declared fields to `null` would state something + * false about all N. + * + * Both are the SAME situation and neither is an author typo, so the default + * `No such key: previous` — which reads as "you misspelled something" — is + * actively misleading. This branch names the batch, says why the binding is + * missing, and gives the one route that actually works. It does NOT create an + * exception: the write still fails (maintainer's ruling on #4800 — fail loud, + * no exemptions), it just fails with a diagnosis instead of a riddle. + * + * An UNDECLARED key still routes to the ordinary typo message even on a bulk + * write: that one IS a typo, and saying "this is a batch" about it would send + * the author down the wrong path. + * + * ⚠️ The escape route named below is deliberately the ONLY one. "Use a + * record-change flow trigger instead" was considered and REJECTED on evidence: + * that trigger subscribes to these very lifecycle hooks + * (`trigger-record-change/src/record-change-trigger.ts` → `engine.registerHook`), + * so on a `multi: true` update it also fires once with `ctx.previous` + * undefined — measured, not assumed. Naming it here would have made this + * message the next `declared ≠ delivered`. Its bulk semantics are filed + * separately. + */ +function unevaluableConditionError( + meta: Hook, + ctx: HookContext, + source: string, + error: CelFault, + declaredFields: Record | undefined, +): HookConditionError { + const { summary, missingKey, unknownVariable, detail } = describeCelFault(error, { + what: 'condition', + undeclaredKeyFix: "fix the hook's condition, or declare the field", + }); + const head = `Hook '${meta.name}' could not evaluate its condition (${summary}) — operation aborted.`; + + if (isPredicateBulkWrite(ctx)) { + // `previous` unbound → cel reports `Unknown variable: previous` (the ROOT + // is absent). A declared field the payload does not set → `No such key: + // ` (the root resolved; `record` is the bare payload here). + const bulkDetail = unknownVariable === 'previous' + ? ` The condition reads 'previous', but this is a PREDICATE bulk write (multi: true):` + + ` it matches many rows and fires the hook ONCE, so there is no single prior record to bind.` + + ` Rewrite the condition without 'previous', or target the write at one record (update by id).` + + ` A record-change flow trigger is NOT a way around this — it binds the same lifecycle hook` + + ` and receives the same unbound 'previous' on a bulk write.` + : missingKey && declaredFields && Object.prototype.hasOwnProperty.call(declaredFields, missingKey) + ? ` '${missingKey}' IS declared on this object, but this is a PREDICATE bulk write (multi: true):` + + ` the stored state of the matched rows is not in hand, so 'record' carries only this write's` + + ` payload. Reference only fields this write sets, or target the write at one record (update by id).` + : undefined; + if (bulkDetail !== undefined) { + return new HookConditionError(`${head}${bulkDetail}`, { + hook: meta.name, + object: ctx.object, + event: ctx.event, + condition: source, + reason: 'unevaluable', + fault: summary, + ...(missingKey ? { missingKey } : {}), + predicateBulkWrite: true, + }); + } + } + + return new HookConditionError(`${head}${detail}`, { + hook: meta.name, + object: ctx.object, + event: ctx.event, + condition: source, + reason: 'unevaluable', + fault: summary, + ...(missingKey ? { missingKey } : {}), + }); +} + +/** + * The rejection a condition that never COMPILED produces (#4775). + * + * Kept distinct from the unevaluable case because the fix is different: this + * one is broken for every record, on every object, forever — there is no input + * that makes it work. Its previous treatment (`condition ignored`) silently + * DELETED the gate, so a guard that was declared to hold writes back let all of + * them through and an audit fired on every write. + */ +function uncompilableConditionError( + meta: Hook, + ctx: HookContext, + source: string, + error: CelFault, +): HookConditionError { + const { summary } = describeCelFault(error, { + what: 'condition', + undeclaredKeyFix: "fix the hook's condition", + }); + return new HookConditionError( + `Hook '${meta.name}' declares a condition that does not compile (${summary}) — operation aborted.` + + ` The condition can never be evaluated, so the hook can neither run nor be skipped honestly;` + + ` fix the expression: ${source}`, + { + hook: meta.name, + object: ctx.object, + event: ctx.event, + condition: source, + reason: 'uncompilable', + fault: summary, + }, + ); +} + /** * The object's DECLARED fields, if this context can reach them. * diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 238873763f..59626d8e79 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -70,7 +70,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati // Export Hook Binder & Wrappers (declarative-metadata → engine glue) export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; -export { wrapDeclarativeHook } from './hook-wrappers.js'; +export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; export type { WrapDeclarativeOptions } from './hook-wrappers.js'; // Export Validation diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 83914e161f..37f3aa55eb 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -130,6 +130,7 @@ import { // that evaluate CEL against "the record" cannot drift apart on what that record // contains — see the module's own doc comment. import { materializeDeclaredFields } from '../declared-fields.js'; +import { describeCelFault } from '../cel-fault.js'; type Mode = 'insert' | 'update'; @@ -846,35 +847,6 @@ function checkStateMachine( return null; } -/** `No such key: ` is cel-js's word for "the predicate read something the - * record does not carry" — the single most useful fact to put in front of the - * author, since after materialisation it can only mean an UNDECLARED key. */ -const NO_SUCH_KEY_RE = /No such key:\s*([A-Za-z_$][\w$]*)/; - -/** - * The OTHER way a predicate written against a total record still faults: an - * ordering comparison (`<`, `>`, `<=`, `>=`) or arithmetic over a value that is - * `null`. CEL has no overload for it, so the whole predicate aborts. - * - * This one deserves its own sentence because the obvious guard does not work: - * `has(x)` is TRUE for a declared field holding `null` (CEL asks whether the key - * is PRESENT, not whether it has a usable value), so `has(a) && has(b) && a < b` - * still faults the moment either is null — on any driver that returns its NULL - * columns, which is most of them. Such a rule never enforced anything on those - * rows; #4649 is what makes that visible instead of silent. - */ -const NULL_OVERLOAD_RE = /no such overload/i; - -/** - * One-line summary of a CEL fault. The engine appends a source excerpt and a - * caret line to `message`, which is right for a log and wrong for an API error, - * so only the first line travels. - */ -function faultSummary(error: { kind: string; message: string }): string { - const first = String(error.message ?? '').split('\n')[0]!.trim(); - return `${error.kind}: ${first || 'unknown error'}`; -} - /** * The rejection a predicate that CANNOT BE EVALUATED produces (#4649). * @@ -888,6 +860,12 @@ function faultSummary(error: { kind: string; message: string }): string { * `packages/spec`) is closed and a broken rule is still "a declared rule * rejected this write" from every consumer's point of view. `constraint.reason` * is what distinguishes the two for anyone who cares. + * + * The fault is READ by the shared `cel-fault.ts` helper (#4775), which words + * the same two sentences for the hook-`condition` surface next door. Two + * evaluators that reject a write for the same reason must not describe it in + * two dialects — the same argument that made `materializeDeclaredFields` + * shared. */ function unevaluableRuleError( ruleName: string, @@ -895,18 +873,10 @@ function unevaluableRuleError( error: { kind: string; message: string }, what: 'predicate' | 'when-predicate', ): FieldValidationError { - const raw = String(error.message ?? ''); - const summary = faultSummary(error); - const missingKey = NO_SUCH_KEY_RE.exec(raw)?.[1]; - const nullOverload = !missingKey && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw); - let detail = ''; - if (missingKey) { - detail = ` The ${what} reads '${missingKey}', which this object does not declare — fix the rule's condition, or declare the field.`; - } else if (nullOverload) { - detail = - ` The ${what} compares a value that is null. Guard it with '!= null'` + - ` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`; - } + const { summary, missingKey, nullOverload, detail } = describeCelFault(error, { + what, + undeclaredKeyFix: "fix the rule's condition, or declare the field", + }); return { field, code: 'rule_violation',