diff --git a/.changeset/bulk-write-previous-hook-condition-diagnostic.md b/.changeset/bulk-write-previous-hook-condition-diagnostic.md new file mode 100644 index 0000000000..6fdd92fefd --- /dev/null +++ b/.changeset/bulk-write-previous-hook-condition-diagnostic.md @@ -0,0 +1,55 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a bulk write blocked by a `previous` hook condition is told it is a VERSION limit, not an authoring mistake (#5037) + +#4775 made an unevaluable hook `condition` abort the operation, and #4861 gave +the predicate-bulk-write case its own sentence instead of a raw +`Unknown variable: previous`. What that sentence still said was +*"rewrite the condition without `previous`, or target the write at one record"* — +written before the maintainer's 2026-08-04 ruling on #4800/#4862, and wrong in +its most important claim. The ruling settled the contract: **on a bulk write, +after-hooks and record-change flow triggers evaluate and fire per row** (recorded +as an ADR-0058 addendum, implemented by #5038). The author's transition condition +is legitimate; the engine is what is behind. Telling them to drop `previous` was +advising a silent semantic change — a transition ("just became done") becomes a +state test ("is done"), which fires on every row that was already done. + +The rejection now says what is actually true: + +- it names the batch and why there is no single prior record to bind, as before; +- it states this is a **current-version limitation**, cites the per-row contract + (ADR-0058 addendum, #4800/#4862) and the issue that retires the rejection + (#5038); +- it leads with the route that works today — target the write at one record, and + the same condition evaluates as authored — and prices the rewrite instead of + recommending it; +- it still refuses to point at a record-change flow trigger as a way out, which + remains verified rather than assumed: that trigger binds the same lifecycle + hooks and receives the same unbound `previous` on a bulk write (#4862). + +**Machine-readable, so a caller never parses the prose.** `HookConditionError` +gains `limitation?: 'bulk_write_previous_unbound' | +'bulk_write_stored_state_unavailable'` (exported as `HookConditionLimitation`) +alongside the existing `predicateBulkWrite` flag. It is deliberately *not* named +`code`: ADR-0112 makes `error.code` a closed wire vocabulary +(`StandardErrorCode` ∪ `ERROR_CODE_LEDGER`) and `rest-server.ts` promotes a +thrown error's `.code` onto the response envelope, so a `.code` here would mint +an unregistered wire code as a side effect. A code that needs to travel goes +through the ledger as a decision. + +**"Does this condition read `previous`" is now read off the parsed CEL AST** +(`collectCelRootIdentifiers`, the utility #4972's build gate already uses), +computed once at wrap time, with the old fault-text check kept as a fallback. +The diagnosis no longer depends on cel-js's wording, and it stays correct when +the evaluator faults on some other key the same condition reads. +`record.previous_status` is not a `previous` reference — the AST reports roots, +not member names — so it keeps the declared-field diagnosis, whose remedy is the +right one there. + +Unchanged, and pinned by tests: single-record writes (bound `previous`, condition +evaluates, handler runs), bulk writes whose conditions do not name `previous`, +the plain undeclared-key typo report on a bulk write, and fail-loud itself — the +write still fails. Nothing here is an exemption; it is the same rejection with a +diagnosis attached. diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index ca72bbe558..18e4379c64 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -56,6 +56,60 @@ --- +> **Addendum (2026-08, #4800 / #4862 / #5037) — BULK-WRITE SCOPE: on a predicate +> (`multi: true`) write, after-hooks and record-change flow triggers evaluate and +> fire PER ROW.** _Contract recorded here; implementation tracked by #5038; the +> rc window ships a named diagnostic in its place._ +> +> The addendum above settles what happens when a write-path predicate cannot be +> evaluated. It does not settle **what the evaluation is even over** when one +> write touches N rows — and that is a scope question this ADR owns, because +> "the same CEL means the same thing on every surface" is D6's premise. +> +> **The decision.** A bulk write is N record changes, so every record-scoped +> declaration on it is evaluated **per row**, with `record` = that row's state +> and `previous` = that row's pre-write state. This is not a new idea on the +> platform: validation predicates have worked this way on bulk writes since +> #3106 (`rulesNeedRows` fetches the matched rows and `evaluateValidationRules` +> runs once per row). Hook `condition`s — and the record-change flow triggers +> that ride the same lifecycle hooks — join them. An author writes one +> transition condition (`previous.done != true && record.done == true`) and it +> means the same thing whether the write carries an id or a predicate. +> +> **What the engine does today, measured (#4862).** A `multi: true` update +> reaches `driver.updateMany`, which resolves an affected COUNT; the lifecycle +> hook fires **once**, `hookContext.previous` is never assigned (only the +> single-id branch fetches a prior row), and `record` degrades to the write's +> bare payload. So a condition naming `previous` is unevaluable and — since the +> #4775 row above — **rejects the write**. +> +> **The rc-window stopgap (#5037).** The rejection stands: fail loud takes no +> exception here (the alternatives — logging an error and skipping the hook, or +> skipping it silently — were considered and refused on #4800, because a missing +> audit row is the one failure nobody goes looking for). What changed is that it +> must no longer read as an author's mistake. `HookConditionError` carries a +> machine-readable `limitation` (`bulk_write_previous_unbound`, +> `bulk_write_stored_state_unavailable`) and a message that names the batch, says +> the CURRENT VERSION is what cannot bind the row's prior state, points at the +> contract above, and gives the route that works today (target the write at one +> record). It is a stopgap with an expiry: when #5038 lands per-row evaluation +> the condition evaluates as authored and this rejection has nothing left to +> report. +> +> **Deliberately not written into that message:** "use a record-change flow +> trigger instead". Verified, not assumed — that trigger subscribes to these very +> lifecycle hooks, so on a bulk write it fires once with the same unbound +> `previous` (#4862). Naming it would have made the error that fixes a +> `declared ≠ delivered` into another one. +> +> **Consequences to price when #5038 implements this**: an after-hook that fires +> once per batch today fires N times (notification hooks send N messages, +> cache-invalidation hooks run N times), so the shape of `ctx.result` per row, +> the per-row meaning of `onError`, and a ceiling on very large matched sets are +> part of that implementation, not free riders on it. + +--- + ## TL;DR ObjectStack exposes **~50 authorable declarations** that hold an expression — formulas, visibility/required/readonly predicates, validation rules, hook conditions, flow/edge conditions, sharing-rule conditions, RLS `using`/`check`, action/view/app visibility, notification/ETL/export/sync/connector conditions — and they all funnel through **one authoring primitive** (`ExpressionInputSchema` → `{ dialect: 'cel', source }`, helpers `cel`/`F`/`P`). The authoring surface is already unified and clean. diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index a59b5c1c54..60f4a1403c 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -57,7 +57,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; -export type { WrapDeclarativeOptions } from './hook-wrappers.js'; +export type { WrapDeclarativeOptions, HookConditionLimitation } from './hook-wrappers.js'; // Validation export { ValidationError, validateRecord } from './validation/record-validator.js'; diff --git a/packages/objectql/src/hook-condition-bulk-previous.test.ts b/packages/objectql/src/hook-condition-bulk-previous.test.ts new file mode 100644 index 0000000000..87de7318fe --- /dev/null +++ b/packages/objectql/src/hook-condition-bulk-previous.test.ts @@ -0,0 +1,436 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5037] A hook condition that reads `previous` on a PREDICATE bulk write gets + * a diagnosis that names the LIMITATION — not a CEL riddle, and not permanent + * advice to abandon the shape. + * + * The rc-window stopgap half of the 2026-08-04 ruling on #4800 / #4862. + * The ruling settled the contract (ADR-0058, bulk-write addendum): on a bulk + * write, after-hooks and record-change flow triggers evaluate and fire **per + * row**, implemented by #5038. Until that lands, the engine's bulk branch binds + * no `previous` and the condition gate rejects the write (#4775 — fail loud, no + * exemptions). What this file pins is the shape of that rejection: + * + * a. bulk write + a condition reading `previous` → a rejection carrying the + * machine-readable `limitation`, saying it is the CURRENT VERSION that + * cannot do this, and naming the route that works today; + * b. the SAME hook on a single-record write → completely unchanged: bound + * `previous`, condition evaluates, handler runs; + * c. a bulk write whose condition does NOT read `previous` → completely + * unchanged, including the plain typo report; + * d. the generic `No such key` / `Unknown variable` riddle no longer reaches + * the caller as the WHOLE story for (a) — including when the fault names + * some other key the same condition reads, which is what the AST-based + * detection (`collectCelRootIdentifiers`) buys over reading cel-js's prose. + */ + +import { describe, it, expect } 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 }, + // Declared on purpose: a field whose NAME starts with the root's spelling, so + // a detector matching the source text rather than the parsed AST mistakes + // `record.previous_status` for a `previous` reference (see the pin below). + previous_status: { name: 'previous_status', label: 'Previous status', type: 'text' as const }, +}; +const taskObject = { name: 'hook_task', label: 'Task', fields: TASK_FIELDS }; +const qlStub = { getObject: (n: string) => (n === 'hook_task' ? taskObject : undefined) }; + +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +function makeHook(condition: string, extra: Partial = {}): Hook { + return { + name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 100, + condition, handler: () => {}, + ...extra, + } as unknown as Hook; +} + +/** An after-update context for a predicate (`multi: true`) bulk write: no id, + * no prior record — exactly what the engine's bulk branch builds. */ +function bulkCtx(data: Record): HookContext { + return { + object: 'hook_task', + event: 'afterUpdate', + input: { data, options: { multi: true } }, + previous: undefined, + ql: qlStub, + } as unknown as HookContext; +} + +/** The same write targeted at one record: prior row in hand, `previous` bound. */ +function singleCtx(data: Record): HookContext { + return { + object: 'hook_task', + event: 'afterUpdate', + input: { id: 't1', data }, + previous: { id: 't1', title: 'Ship it', status: 'todo', done: false }, + ql: qlStub, + } as unknown as HookContext; +} + +/* ──────────────────────────────────────────────────────────────────────────── + * a. The bulk write reading `previous` gets the named limitation + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5037] a bulk write whose hook condition reads `previous`', () => { + const TRANSITION = 'previous.done != true && record.done == true'; + + it('rejects with a machine-readable `limitation`, not just prose', async () => { + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, + ); + + const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + // The discriminator a caller branches on. Deliberately NOT `code`: ADR-0112 + // makes `error.code` a closed wire vocabulary and `rest-server.ts` promotes + // a thrown error's `.code` onto the envelope, so a `.code` here would mint + // an unregistered wire code by side effect. + expect(err.limitation).toBe('bulk_write_previous_unbound'); + expect((err as any).code).toBeUndefined(); + expect(err.predicateBulkWrite).toBe(true); + expect(err.reason).toBe('unevaluable'); + expect(err.hook).toBe('audit_task_completion'); + expect(err.condition).toBe(TRANSITION); + // …and the handler never ran (the gate is before it). + expect(ran).toEqual([]); + }); + + it('names the batch, the missing binding, and that the VERSION is what is behind', async () => { + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + + expect(err.message).toContain("Hook 'audit_task_completion'"); + expect(err.message).toContain('PREDICATE bulk write (multi: true)'); + expect(err.message).toContain('no single prior record to bind'); + // The ruling's substance: the author's metadata is fine, the platform is + // behind, and the gap has an owner. Without this the message reads as + // "never write transition conditions", which is the opposite of the + // contract recorded on ADR-0058. + expect(err.message).toContain('CURRENT-VERSION limitation'); + expect(err.message).toContain('PER ROW'); + expect(err.message).toContain('ADR-0058'); + expect(err.message).toContain('#5038'); + }); + + it('gives the route that works TODAY, and prices the one that changes meaning', async () => { + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + + expect(err.message).toContain('target the write at one record (update by id)'); + // Dropping `previous` is not free and the message must not present it as + // the fix: a transition silently becomes a state test. + expect(err.message).toContain('becomes a state test'); + // VERIFIED (#4862): the record-change trigger subscribes to these same + // lifecycle hooks, so it is not an escape hatch. Naming it would make this + // very message the next `declared ≠ delivered`. + expect(err.message).toContain('A record-change flow trigger is NOT a way around this'); + }); + + it('still ABORTS the write — the diagnosis is not an exemption', async () => { + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: TRANSITION, + 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 }); + + const err = await engine + .update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.limitation).toBe('bulk_write_previous_unbound'); + }); + + it('fires for a delete-shaped predicate bulk write too', async () => { + const wrapped = wrapDeclarativeHook( + makeHook('previous.done != true', { events: ['afterDelete'] } as any), + (async () => {}) as any, { logger: silentLogger }, + ); + const ctx = { + object: 'hook_task', event: 'afterDelete', + input: { data: {}, options: { multi: true } }, previous: undefined, ql: qlStub, + } as unknown as HookContext; + + const err = await wrapped(ctx).then(() => null, (e) => e); + expect(err.limitation).toBe('bulk_write_previous_unbound'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * b. The single-record write of the SAME hook is untouched + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5037] a single-record write is completely unchanged', () => { + const TRANSITION = 'previous.done != true && record.done == true'; + + it('binds `previous`, evaluates the transition, and runs the handler', async () => { + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, + ); + + await wrapped(singleCtx({ done: true })); + + expect(ran).toEqual(['audited']); + }); + + it('skips (without throwing) when the transition did not happen', async () => { + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, + ); + // Already done before the write → not a transition → plain skip, no error. + const ctx = singleCtx({ done: true }); + (ctx as any).previous = { id: 't1', title: 'Ship it', status: 'todo', done: true }; + + await wrapped(ctx); + + expect(ran).toEqual([]); + }); + + it('through the real engine: the update lands and the hook fires', async () => { + const ran: string[] = []; + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: TRANSITION, + 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']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * c. A bulk write whose condition does not read `previous` is untouched + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5037] a bulk write with no `previous` in the condition is unaffected', () => { + it('evaluates over the payload and runs the handler', async () => { + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true'), (async () => { ran.push('ran'); }) as any, { logger: silentLogger }, + ); + + await wrapped(bulkCtx({ done: true })); + + expect(ran).toEqual(['ran']); + }); + + it('still skips quietly when the condition is FALSE', async () => { + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true'), (async () => { ran.push('ran'); }) as any, { logger: silentLogger }, + ); + + await wrapped(bulkCtx({ done: false })); + + expect(ran).toEqual([]); + }); + + it('reports an UNDECLARED key as the typo it is, with no batch talk', async () => { + 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.limitation).toBeUndefined(); + expect(err.predicateBulkWrite).toBeUndefined(); + expect(err.message).toContain("reads 'stauts', which this object does not declare"); + expect(err.message).not.toContain('PREDICATE bulk write'); + }); + + it('keeps the DECLARED-but-unset field diagnosis on its own limitation name', async () => { + // Same root cause (no stored row in hand), different remedy, so it carries + // its own machine-readable name rather than being folded into `previous`. + 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.limitation).toBe('bulk_write_stored_state_unavailable'); + expect(err.predicateBulkWrite).toBe(true); + expect(err.message).toContain("'archived' IS declared on this object"); + expect(err.message).toContain('#5038'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * d. The riddle no longer reaches the caller for the `previous` case + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5037] the generic CEL fault is never the whole story on this path', () => { + it('does not leave the author with the bare `No such key` / unbound-root sentence', 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); + + // `describeCelFault`'s generic sentences — both of which read as "you wrote + // it wrong" — must not be what this author is left holding. + expect(err.message).not.toMatch(/which this object does not declare/); + expect(err.message).not.toMatch(/which is not bound for this operation/); + // The raw fault still travels as a FACT (`fault`, and the head's summary), + // it is simply no longer the diagnosis. + expect(err.fault).toMatch(/Unknown variable: previous|No such key: previous/); + }); + + it('names `previous` for a condition that also reads a declared-but-unset field', async () => { + // Both halves are unevaluable on a batch, for the same reason. The + // `previous` half is the one the author cannot work around by writing the + // condition differently, so it is the one the message leads with — reading + // the answer off the parsed AST rather than off whichever fault the + // evaluator happened to raise first is what keeps that true. + const wrapped = wrapDeclarativeHook( + makeHook('record.archived == true && previous.done != true'), + (async () => {}) as any, { logger: silentLogger }, + ); + + const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + + expect(err.limitation).toBe('bulk_write_previous_unbound'); + expect(err.message).toContain("The condition reads 'previous'"); + }); + + it('does NOT mistake a declared field spelled like the root for a `previous` reference', async () => { + // The AST reports ROOT identifiers; `previous_status` is a member name + // under `record`, so the root set is `{record}` and this write gets the + // declared-field diagnosis (whose remedy — reference only what this write + // sets — is the correct one here). A detector matching the source TEXT + // would answer `bulk_write_previous_unbound` and send the author looking + // for a `previous` reference that is not there. + const wrapped = wrapDeclarativeHook( + makeHook('record.previous_status == "todo"'), + (async () => {}) as any, { logger: silentLogger }, + ); + + const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + + expect(err.limitation).toBe('bulk_write_stored_state_unavailable'); + expect(err.message).toContain("'previous_status' IS declared on this object"); + expect(err.message).not.toContain("The condition reads 'previous'"); + }); + + it('keeps the typo sentence when the condition BOTH misspells a key and reads `previous`', async () => { + // The two halves have different owners: the typo is the author's, the batch + // limitation is the platform's. Reporting only one would send them back for + // a second round. + const wrapped = wrapDeclarativeHook( + makeHook('record.stauts == "x" && previous.done != true'), + (async () => {}) as any, { logger: silentLogger }, + ); + + const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + + expect(err.limitation).toBe('bulk_write_previous_unbound'); + if (err.missingKey === 'stauts') { + // The evaluator surfaced the typo → the author is told about BOTH. + expect(err.message).toContain("reads 'stauts', which this object does not declare"); + } + expect(err.message).toContain('PREDICATE bulk write (multi: true)'); + }); + + it('is inert for a comprehension variable that happens to be named `previous`', async () => { + // `collectCelRootIdentifiers` reports comprehension bind variables as roots + // (its documented caveat), so this condition "reads previous" by that + // measure. It is a non-event: the expression binds its own variable and + // evaluates, so the answer is never consulted. + const ran: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('[1, 2].exists(previous, previous > 1)'), + (async () => { ran.push('ran'); }) as any, { logger: silentLogger }, + ); + + await wrapped(bulkCtx({ done: true })); + + expect(ran).toEqual(['ran']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * Real-engine harness (mirrors hook-condition-fail-loud.test.ts) + * ──────────────────────────────────────────────────────────────────────────── */ + +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-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 83f9c5309e..d8b13c3161 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -16,7 +16,7 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; import type { Expression } from '@objectstack/spec'; import type { HookHandler } from './engine.js'; -import { ExpressionEngine } from '@objectstack/formula'; +import { ExpressionEngine, collectCelRootIdentifiers } 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'; @@ -75,6 +75,40 @@ const noopLogger = { * resurrect exactly the silent skip this error exists to abolish, and would * mint a third set of semantics for the same word. */ +/** + * Which CURRENT-VERSION platform limitation made the condition unevaluable, + * when a limitation — rather than the author — is what happened (#5037). + * + * The distinction this names is the whole point of the diagnostic: an + * undeclared key is the AUTHOR's to fix and stays fixed; these two are the + * PLATFORM's, they contradict the contract ADR-0058's bulk-write addendum + * records, and they retire when #5038 lands per-row semantics. A caller that + * wants to tell "your hook is wrong" from "this version cannot do that yet" + * — a REST layer choosing a status, a test, a Studio surface — reads this + * field instead of matching on the message text. + * + * - `bulk_write_previous_unbound` — the condition names `previous` on a + * predicate (`multi: true`) write, which matches N rows and fires the hook + * once, so there is no single prior record to bind; + * - `bulk_write_stored_state_unavailable` — the condition names a DECLARED + * field this write does not set, and `record` is the bare payload on a bulk + * write for the same reason (no single stored row to merge with). + * + * ## Why this is not `error.code` + * + * Deliberately NOT named `code`. ADR-0112 makes `error.code` a CLOSED wire + * vocabulary — `StandardErrorCode` ∪ `ERROR_CODE_LEDGER`, both declared in + * `packages/spec/src/api/` — and `rest-server.ts` promotes a thrown error's + * `.code` straight onto the response envelope. Putting a `.code` here would + * therefore mint an unregistered wire code by side effect, which is the exact + * `declared ≠ enforced` shape this family exists to remove. If this ever needs + * to travel on the wire it goes through the ledger, as a decision, not as a + * property that happens to be named `code`. + */ +export type HookConditionLimitation = + | 'bulk_write_previous_unbound' + | 'bulk_write_stored_state_unavailable'; + export class HookConditionError extends Error { override readonly name = 'HookConditionError'; /** The hook whose declared condition could not be evaluated. */ @@ -93,6 +127,10 @@ export class HookConditionError extends Error { /** 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; + /** The current-version limitation behind the fault, when one is the cause + * rather than the authored expression (#5037). See + * {@link HookConditionLimitation}. */ + readonly limitation?: HookConditionLimitation; constructor(message: string, info: { hook: string; @@ -103,6 +141,7 @@ export class HookConditionError extends Error { fault: string; missingKey?: string; predicateBulkWrite?: boolean; + limitation?: HookConditionLimitation; }) { super(message); this.hook = info.hook; @@ -113,6 +152,7 @@ export class HookConditionError extends Error { this.fault = info.fault; this.missingKey = info.missingKey; this.predicateBulkWrite = info.predicateBulkWrite; + this.limitation = info.limitation; } } @@ -180,6 +220,11 @@ export function wrapDeclarativeHook( if (expr.source && expr.source.trim()) { const source = expr.source; const check = ExpressionEngine.compile(expr); + // Read ONCE, off the parsed AST, whether this condition names `previous` + // at all (#5037) — see `conditionReadsPrevious`. Wrap time, not call + // time: the answer is a property of the source, and the call path is on + // every write of the object. + const readsPrevious = conditionReadsPrevious(source); if (check.ok) { conditionFn = (ctx: HookContext) => { // `previous` is passed through as-is: `undefined` means the binding @@ -193,7 +238,7 @@ export function wrapDeclarativeHook( const r = ExpressionEngine.evaluate(expr, { record: record ?? {}, previous }); if (!r.ok) { // [#4775] Fail LOUD. Not `false` — see `HookConditionError`. - throw unevaluableConditionError(meta, ctx, source, r.error, declaredFieldsFor(ctx)); + throw unevaluableConditionError(meta, ctx, source, r.error, declaredFieldsFor(ctx), readsPrevious); } return Boolean(r.value); }; @@ -462,6 +507,38 @@ function isPredicateBulkWrite(ctx: HookContext): boolean { return Boolean(options && typeof options === 'object' && options.multi); } +/** + * Does this condition NAME `previous` at all? Answered from the parsed CEL AST + * (#5037), not from the fault text a failed evaluation happened to produce. + * + * The diagnostic below has to tell "this hook reads the pre-write state, which + * a bulk write cannot supply in this version" from "this hook has a typo". + * Deriving that from cel-js's message (`Unknown variable: previous`) works + * today and is kept as the fallback, but it makes an author-facing diagnosis + * depend on an upstream library's prose: reword the fault and the batch silently + * goes back to reporting a riddle. The AST is the same fact stated by the + * expression itself, so the diagnosis holds whatever the evaluator says — and it + * is available even when the fault names something ELSE the same condition also + * reads. + * + * `collectCelRootIdentifiers` is the utility #4972's build gate already uses for + * "which roots does this expression reference". Its documented caveat: a + * comprehension bind variable (`[1,2].exists(previous, previous > 1)`) is + * reported as a root, so a hook that names its bind variable `previous` reads as + * a `previous` reference here. That false positive is inert by construction — + * this answer is consulted ONLY on the error path, and such an expression binds + * its own variable and evaluates fine, so it never reaches one. Paying for a + * comprehension-aware walk to remove an unreachable case would be the more + * expensive wrong call. + * + * A source that does not parse returns `false`: it never compiled, so it goes + * down the `uncompilableConditionError` path and never reaches this diagnosis. + */ +function conditionReadsPrevious(source: string): boolean { + const roots = collectCelRootIdentifiers(source); + return roots.ok ? roots.roots.includes('previous') : false; +} + /** * The rejection a condition that CANNOT BE EVALUATED produces (#4775). * @@ -489,16 +566,32 @@ function isPredicateBulkWrite(ctx: HookContext): boolean { * * 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 + * the author down the wrong path. When the condition reads `previous` AND + * misspells something, the author gets both sentences — the typo is theirs to + * fix, and the batch limitation is still waiting behind it. + * + * ## The rejection names a LIMITATION, not the contract (#5037, ADR-0058) + * + * The 2026-08-04 ruling on #4800/#4862 settled what a bulk write MEANS: after + * hooks and record-change flow triggers evaluate and fire PER ROW — recorded as + * an addendum on ADR-0058, implemented by #5038. This rejection is the rc-window + * stopgap for the gap between that contract and today's engine, so it says so: + * an author who reads it learns that the transition condition they wrote is + * legitimate and that the platform, not their metadata, is behind. The earlier + * wording ("rewrite the condition without `previous`") predates the ruling and + * read as permanent guidance to abandon a supported shape — worse, silently + * changing a transition into a state test, which fires on rows that were + * already done. The single-record route is now the recommended one, with the + * rewrite named for what it costs. + * + * ⚠️ The escape routes named below are deliberately the only ones. "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. + * undefined — measured, not assumed (#4862). Naming it here would have made + * this message the next `declared ≠ delivered`; it converges on the same per-row + * contract through #5038, not before. */ function unevaluableConditionError( meta: Hook, @@ -506,6 +599,7 @@ function unevaluableConditionError( source: string, error: CelFault, declaredFields: Record | undefined, + readsPrevious = false, ): HookConditionError { const { summary, missingKey, unknownVariable, detail } = describeCelFault(error, { what: 'condition', @@ -514,22 +608,46 @@ function unevaluableConditionError( 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):` + + const declaredMissingKey = missingKey && declaredFields + && Object.prototype.hasOwnProperty.call(declaredFields, missingKey) + ? missingKey + : undefined; + // The typo sentence, kept alongside the batch diagnosis when the fault + // named a key this object does not declare: that half IS the author's. + const typoDetail = missingKey && !declaredMissingKey ? detail : ''; + // Does the condition read `previous`? The AST says so directly; the fault + // text (`Unknown variable: previous` — the ROOT is absent) is the fallback + // for a caller that did not pass the AST answer through. + const namesPrevious = readsPrevious || unknownVariable === 'previous'; + + let limitation: HookConditionLimitation | undefined; + let bulkDetail: string | undefined; + if (namesPrevious) { + limitation = 'bulk_write_previous_unbound'; + bulkDetail = + ` 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).` + + ` This is a CURRENT-VERSION limitation, not the contract: a bulk write is declared to` + + ` evaluate and fire after-hooks PER ROW (ADR-0058, bulk-write addendum; ruling on #4800/#4862),` + + ` and this rejection retires when that lands (#5038).` + + ` Until then, target the write at one record (update by id) — a single-record write binds` + + ` 'previous', so this very condition evaluates as authored. Dropping 'previous' from the` + + ` condition also unblocks the batch, but it changes what the hook MEANS: a transition` + + ` ("just became done") becomes a state test ("is done"), which fires on rows that were` + + ` already done.` + ` 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; + ` and receives the same unbound 'previous' on a bulk write (#4862).`; + } else if (declaredMissingKey) { + limitation = 'bulk_write_stored_state_unavailable'; + bulkDetail = + ` '${declaredMissingKey}' 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).` + + ` Same current-version limitation as above: per-row evaluation (#5038) gives 'record' the row's` + + ` real state.`; + } if (bulkDetail !== undefined) { - return new HookConditionError(`${head}${bulkDetail}`, { + return new HookConditionError(`${head}${typoDetail}${bulkDetail}`, { hook: meta.name, object: ctx.object, event: ctx.event, @@ -538,6 +656,7 @@ function unevaluableConditionError( fault: summary, ...(missingKey ? { missingKey } : {}), predicateBulkWrite: true, + limitation, }); } } diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 3cc6148caa..c6b849c3d9 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -90,7 +90,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; -export type { WrapDeclarativeOptions } from './hook-wrappers.js'; +export type { WrapDeclarativeOptions, HookConditionLimitation } from './hook-wrappers.js'; // Export Validation export { ValidationError, validateRecord } from './validation/record-validator.js'; diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index 4beba4f963..64ad269944 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -6,6 +6,11 @@ "adrs": ["ADR-0057", "ADR-0058"], "invariant": "A declared field lock is the SERVER's to enforce (ADR-0057 D10 — the client grid is courtesy). A `readonlyWhen` whose predicate names a scope root the write path could not bind resolves to LOCKED, not to \"not locked\": \"could not check\" must never read as \"allowed\" on a field the author declared frozen. This narrows ADR-0058 D5's fail-soft tier deliberately and only for that case — a merely BROKEN predicate (undeclared key, null overload, parse fault, throw) still fails open, and requiredWhen / option visibleWhen are untouched." }, + { + "file": "packages/objectql/src/hook-wrappers.ts", + "adrs": ["ADR-0058", "ADR-0112"], + "invariant": "An unevaluable hook `condition` ABORTS the operation (ADR-0058's write-path addendum, #4775) — `onError` never sees it and cannot soften it back into a silent skip. On a predicate (`multi: true`) bulk write the rejection must name the LIMITATION, not the author: ADR-0058's bulk-write addendum declares per-row evaluation the contract (#5038), so this rejection is a dated stopgap and its message says so. The discriminator is `limitation`, deliberately NOT `code` — ADR-0112 makes `error.code` a closed wire vocabulary and the REST layer promotes a thrown error's `.code` onto the envelope, so naming it `code` would mint an unregistered wire code by side effect." + }, { "file": "packages/spec/src/identity/membership-role.ts", "adrs": ["ADR-0090", "ADR-0108"],