diff --git a/.changeset/hook-condition-merged-record.md b/.changeset/hook-condition-merged-record.md new file mode 100644 index 0000000000..db1a2f2ef7 --- /dev/null +++ b/.changeset/hook-condition-merged-record.md @@ -0,0 +1,59 @@ +--- +'@objectstack/objectql': minor +--- + +**A declarative hook `condition` is now evaluated against the RECORD — the stored row overlaid with this write's payload — not against the update payload alone (#4770).** + +⚠️ **Behaviour change — read this before upgrading.** The condition gate used to evaluate +against `ctx.input.data`: only the fields the current write happened to carry. +`ctx.previous` sat behind it, unreachable, and the two were never merged. So a condition +could reference only a field the update *happened* to touch; referencing anything else +aborted the CEL expression with `No such key` — which the gate swallowed into `false`, +leaving one WARN line as the sole trace. + +For a guard-style hook that reads as "let it through"; for an audit-style hook it reads as +"do not record it". `condition: "record.done == true"` on an audit hook therefore did NOT +run on the most ordinary updates there are — change the status, change the assignee — +because `done` was not in the payload. + +The record a condition reads is now built the same way a validation predicate's is +(#1871 / #4649, via one shared helper so the two cannot drift): + +- **stored ⊕ payload** — the prior record overlaid with this write's data, so a condition + may reference any field of the record, not just the changed ones. The payload still + wins for the fields it carries. +- **total over the object's DECLARED fields** — `null` for a declared field present in + neither, so a driver that stores only the columns it wrote no longer decides whether an + expression is evaluable. +- **declared fields only** — an undeclared or typo'd key (`record.stauts`) stays + unevaluable and is still reported, exactly as before. + +Materialisation happens only when the persisted state is actually in hand — an insert, or +an update whose prior row was fetched. A predicate (`multi: true`) bulk update carries no +prior row, so its payload is left as it is rather than gaining `null`s that would +contradict the stored rows. No code path fetches a record it did not already load. + +**What you may see after upgrading** + +- **Conditions that never fired start firing.** A hook gated on a field the payload rarely + carried was silently skipped; it now evaluates. This is the declaration finally being + honoured, but expect hooks to run on writes where they previously did not. +- **A condition is now about the record's STATE, not about this write's diff.** + `record.done == true` fires on every update of a task that *is* done, not only on the + update that set it. A condition cannot express a transition today — the CEL scope binds + `record` only. +- **Conditions guarded with `has(...)` need `!= null`.** `has(x)` asks whether the key is + **present**, and a declared field holding `null` is present — so + `has(a) && has(b) && a > b` still faults on `null > null`. Same lesson as #4649: + + ```diff + - condition: 'has(record.spent) && has(record.budget) && record.spent > record.budget' + + condition: 'record.spent != null && record.budget != null && record.spent > record.budget' + ``` + + `has()` remains correct for asking whether an **undeclared** key exists. + +**Unchanged, deliberately:** what happens when a condition is *still* unevaluable after +merging — it is logged at WARN and treated as `false`, as before. Whether that fallback +should differ by hook category (a guard fails open, an audit fails silent) is a separate +decision, tracked on its own issue. diff --git a/examples/app-showcase/src/data/hooks/index.ts b/examples/app-showcase/src/data/hooks/index.ts index a423bb1b5d..fa289a1b96 100644 --- a/examples/app-showcase/src/data/hooks/index.ts +++ b/examples/app-showcase/src/data/hooks/index.ts @@ -63,11 +63,15 @@ export const WarnOverBudgetHook = { label: 'Warn On Over-Budget Project', object: 'showcase_project', events: ['afterUpdate'] as LifecycleEvent[], - // Guard with has(): an afterUpdate fired by a partial write (e.g. the - // task-rollup that only touches task_count) carries a record WITHOUT - // spent/budget, and CEL throws "No such key" on a bare `record.spent`. - // has() is the missing-key-safe macro — the hook simply skips those. - condition: "has(record.spent) && has(record.budget) && record.spent > record.budget", + // Guard with `!= null`, NOT with `has()` (#4770, same lesson as #4649). A + // condition is evaluated against the STORED record overlaid with this + // write's payload, made total over the object's declared fields — so a + // partial write (the task-rollup that only touches task_count) still sees + // spent/budget, and `has(record.spent)` is uniformly TRUE for a declared + // field, including one holding null. Only `!= null` actually keeps + // `null > null` — which CEL has no overload for — from aborting the + // expression. + condition: "record.spent != null && record.budget != null && record.spent > record.budget", body: { language: 'js' as const, source: "var r = ctx.result || ctx.input || {}; ctx.log.warn('project over budget: ' + (r.name || r.id || 'unknown') + ' (' + r.spent + ' / ' + r.budget + ')');", diff --git a/packages/objectql/src/declared-fields.ts b/packages/objectql/src/declared-fields.ts new file mode 100644 index 0000000000..7a97356357 --- /dev/null +++ b/packages/objectql/src/declared-fields.ts @@ -0,0 +1,58 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Make a record TOTAL over an object's DECLARED fields. + * + * Shared by the two places that evaluate a CEL expression against "the + * record": object-level validation predicates + * (`validation/rule-validator.ts`, #1871 / #4649) and declarative hook + * `condition`s (`hook-wrappers.ts`, #4770). They used to disagree — a + * predicate saw a total record while a hook condition saw only the fields the + * current write happened to carry — which is precisely the drift this module + * exists to prevent: an author cannot be expected to know that the same + * `record.done == true` means two different things depending on which surface + * reads it. + * + * CEL is strict about missing keys: `record.x` on a record that does not carry + * the key `x` aborts the whole expression with `No such key`, which is NOT the + * same as reading `null`. Whether a key is carried is a property of the DRIVER + * (a driver that stores only written columns returns a record missing every + * column the write never touched), not of the data — so without this an + * expression's evaluability depends on storage internals the author cannot + * see. + * + * Scope is deliberately the object's **declared fields only**. Materialising + * every key an expression happens to name would paper over author typos: a + * `record.stauts` must stay unevaluable so it is reported (fail-closed for + * validation, #4649) rather than silently read as `null` and quietly answered + * "no violation" / "condition false". + * + * `undefined` counts as absent (not just a missing key): CEL treats an own key + * holding `undefined` exactly as it treats no key at all. + * + * ## Only ever call this when the record's persisted state is IN HAND + * + * On insert there is nothing to know — absence genuinely means "no value". On + * update it is knowable only when the prior row was actually fetched. Without + * it, defaulting a declared field to `null` would not be materialising an + * absent value, it would be FABRICATING one that contradicts the stored row. + * Callers decide; this function only applies the rule. + * + * ## Consequence worth knowing before writing an expression + * + * Because a declared field is always present afterwards, `has(record.)` is uniformly TRUE (a materialised `null` is a present key holding + * null — CEL's own rule). `has()` therefore guards against an UNDECLARED key, + * not against an empty value; test emptiness with `record.x != null`. + */ +export function materializeDeclaredFields>( + record: T, + fields: Record | undefined | null, +): T { + if (!fields || typeof fields !== 'object') return record; + const target = record as Record; + for (const name of Object.keys(fields)) { + if (target[name] === undefined) target[name] = null; + } + return record; +} diff --git a/packages/objectql/src/hook-condition-merged-record.test.ts b/packages/objectql/src/hook-condition-merged-record.test.ts new file mode 100644 index 0000000000..31a1dc8c01 --- /dev/null +++ b/packages/objectql/src/hook-condition-merged-record.test.ts @@ -0,0 +1,360 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4770] A declarative hook `condition` is evaluated against the RECORD — + * the stored row overlaid with this write's payload, made total over the + * object's declared fields — not against the update payload alone. + * + * Before this fix `pickRecordPayload` returned `ctx.input.data` and never + * reached `ctx.previous`, so a condition could only reference a field the + * update happened to touch. `showcase_audit_task_completion` + * (`condition: "record.done == true"`) therefore did NOT run on the most + * ordinary updates of all — change the status, change the assignee — because + * `done` was not in the payload: CEL aborted with `No such key: done` and the + * gate swallowed that into `false` plus one WARN line. + * + * The semantics reused here are #4649's (`validation/rule-validator.ts`, via + * the shared `materializeDeclaredFields`): payload ⊕ prior record, `null` for a + * DECLARED key present in neither. Declared-only is the load-bearing half — a + * typo'd or undeclared key must stay unevaluable, so the existing + * warn-and-treat-as-false fallback (deliberately UNCHANGED here; the failure + * direction is tracked separately) still reports it. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import { wrapDeclarativeHook } from './hook-wrappers.js'; +import type { Hook, HookContext } from '@objectstack/spec/data'; + +/** Fields as an object declares them; `archived` is never written by any test, + * so it exists only as a DECLARATION — the case materialisation covers. */ +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 }; + +/** Minimal `ctx.ql` stand-in: the condition gate only ever asks the engine for + * the object's declared fields, which is an in-memory registry read. */ +const qlStub = { + getObject: (name: string) => (name === 'hook_task' ? taskObject : undefined), +}; + +function makeCtx(overrides: Partial = {}): HookContext { + return { + object: 'hook_task', + event: 'afterUpdate', + input: { id: 't1', data: { status: 'in_progress' } }, + previous: { id: 't1', title: 'Ship it', status: 'todo', done: true }, + ql: qlStub, + ...overrides, + } as unknown as HookContext; +} + +function makeHook(condition: string, calls: string[]): Hook { + return { + name: 'audit', object: 'hook_task', events: ['afterUpdate'], priority: 100, + condition, + handler: () => { calls.push('ran'); }, + } as Hook; +} + +function captureLogger() { + const warn = vi.fn(); + return { + warn, + logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + conditionWarnings: () => + warn.mock.calls.filter(([msg]) => String(msg).includes('condition evaluation failed')), + }; +} + +describe('[#4770] hook condition evaluates against stored ⊕ payload', () => { + it('fires on a declared field the update payload does NOT carry', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook(makeHook('record.done == true', calls), (async (ctx: any) => { + calls.push(ctx.event); + }) as any, { logger }); + + await wrapped(makeCtx()); + + // The payload is `{ status }` only; `done` comes from the stored record. + expect(calls).toEqual(['afterUpdate']); + expect(conditionWarnings()).toEqual([]); + }); + + it('the payload still WINS over the stored value for a field it does carry', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true', calls), + (async () => { calls.push('ran'); }) as any, + ); + + // stored done=true, this write flips it to false → must NOT fire. + await wrapped(makeCtx({ input: { id: 't1', data: { done: false } } } as any)); + expect(calls).toEqual([]); + + // stored done=false, this write flips it to true → must fire. + await wrapped(makeCtx({ + previous: { id: 't1', status: 'todo', done: false }, + input: { id: 't1', data: { done: true } }, + } as any)); + expect(calls).toEqual(['ran']); + }); + + it('materialises a DECLARED field absent from both payload and stored row', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + // `archived` is declared but the driver returned no such column — the + // shape #4649 describes. `record.archived == null` must be answerable. + const wrapped = wrapDeclarativeHook( + makeHook('record.archived == null', calls), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx()); + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('an UNDECLARED key stays unevaluable — materialisation does not paper over typos', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + // `dnoe` is the classic transposition of `done`. Nothing declares it, so + // it must NOT be materialised to null and quietly answered "false". + const wrapped = wrapDeclarativeHook( + makeHook('record.dnoe == true', calls), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx()); + + 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 () => { + const calls: string[] = []; + 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. + 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)); + expect(calls).toEqual([]); + }); + + it('materialises on INSERT too — same record shape a validation predicate reads', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const meta = { ...makeHook('record.done == null', calls), events: ['beforeInsert'] } as Hook; + const wrapped = wrapDeclarativeHook(meta, (async () => { calls.push('ran'); }) as any, { logger }); + + await wrapped(makeCtx({ + event: 'beforeInsert', + previous: undefined, + input: { data: { title: 'fresh' } }, + } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('evaluates a delete-shaped context against the (materialised) pre-image', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const meta = { ...makeHook('record.done == true', calls), events: ['beforeDelete'] } as Hook; + const wrapped = wrapDeclarativeHook(meta, (async () => { calls.push('ran'); }) as any, { logger }); + + await wrapped(makeCtx({ + event: 'beforeDelete', + input: { id: 't1', options: {} }, + } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('never mutates the engine\'s own ctx.previous / ctx.input.data', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.archived == null', calls), + (async () => { calls.push('ran'); }) as any, + ); + const ctx = makeCtx(); + await wrapped(ctx); + + expect(calls).toEqual(['ran']); + // The after-hooks that run next observe these objects; a materialised + // `archived: null` leaking into them would be a fabricated column. + expect(Object.keys(ctx.previous as object).sort()).toEqual(['done', 'id', 'status', 'title']); + expect(Object.keys((ctx.input as any).data)).toEqual(['status']); + }); + + it('a context with no engine still merges (no materialisation, no crash)', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true', calls), + (async () => { calls.push('ran'); }) as any, + ); + await wrapped(makeCtx({ ql: undefined } as any)); + expect(calls).toEqual(['ran']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * The showcase repro, at integration level. + * + * `rm -rf examples/app-showcase/.objectstack && pnpm dev` printed ten + * `condition evaluation failed` lines for `showcase_audit_task_completion`. + * This is the same object/hook shape driven through a REAL engine over an + * in-memory driver that — like a SQL driver — stores only the columns a write + * actually touched. + * ──────────────────────────────────────────────────────────────────────────── */ + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + // Only the written columns are stored — a declared-but-unwritten column + // is simply absent from every row this driver ever returns. + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).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 { driver, stores }; +} + +describe('[#4770] showcase repro — showcase_audit_task_completion over a real engine', () => { + let engine: ObjectQL; + let audited: string[]; + let warn: ReturnType; + + const conditionWarnings = () => + warn.mock.calls.filter(([msg]) => String(msg).includes('condition evaluation failed')); + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + + audited = []; + warn = vi.fn(); + bindHooksToEngine( + engine, + [{ + // The showcase hook, minus `async` (fire-and-forget would make the + // assertion a race) and minus the sandboxed body (the condition gate + // runs before either). + name: 'showcase_audit_task_completion', + object: 'hook_task', + events: ['afterUpdate'], + priority: 90, + condition: 'record.done == true', + handler: (ctx: any) => { audited.push(String(ctx.input?.id ?? '?')); }, + } as unknown as Hook], + { packageId: 'app:showcase', logger: { debug: () => {}, info: () => {}, warn, error: () => {} } }, + ); + }); + + it('audits a completed task on an update that never mentions `done`', async () => { + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'todo', done: true }); + + // The most ordinary update there is: move the status, nothing else. + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(audited).toEqual([row.id]); + // The ten-lines-per-boot symptom the issue opened on. + expect(conditionWarnings()).toEqual([]); + }); + + it('still does not audit a task that is not done', async () => { + const row: any = await engine.insert('hook_task', { title: 'Draft', status: 'todo', done: false }); + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(audited).toEqual([]); + expect(conditionWarnings()).toEqual([]); + }); + + it('a task whose `done` column was never written reads as null, not as a fault', async () => { + // The driver stores only written columns, so this row carries no `done` at + // all — the case that used to fault even WITH the prior record merged in. + const row: any = await engine.insert('hook_task', { title: 'Untouched', status: 'todo' }); + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(audited).toEqual([]); + expect(conditionWarnings()).toEqual([]); + }); +}); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 9b7790f4ec..4c067940f1 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -18,6 +18,7 @@ import type { Expression } from '@objectstack/spec'; 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'; export interface WrapDeclarativeOptions { /** Logger for declarative-layer diagnostics (timeouts, retries, swallowed errors). */ @@ -48,11 +49,12 @@ const noopLogger = { * 4. timeout → abort if handler runs too long * 5. onError → swallow when set to 'log' * - * The condition formula is evaluated against the most useful record-shaped - * payload available on the context (write payloads first, then `previous`, - * then a flat merge of input). Read events typically have no record yet, - * so a condition on a `beforeFind` will simply skip when no data is - * present. + * The condition formula is evaluated against the record-shaped view of the + * context 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 — the same shape a validation predicate reads). Read events + * typically have no record yet, so a condition on a `beforeFind` will simply + * skip when no data is present. */ export function wrapDeclarativeHook( meta: Hook, @@ -317,20 +319,88 @@ function installFlatInput(ctx: HookContext): () => void { }; } +/** Hook events whose write has no prior state at all — absence of a key on the + * payload genuinely means "no value", never "unchanged". */ +function isInsertEvent(event: unknown): boolean { + return event === 'beforeInsert' || event === 'afterInsert'; +} + /** - * Choose the record-shaped object the condition formula should evaluate - * against. Order: - * 1. ctx.input.data — write operations carry the new record here - * 2. ctx.previous — update/delete carry pre-image here - * 3. ctx.input — fall back to flat input bag (read ops, custom shapes) + * The object's DECLARED fields, if this context can reach them. + * + * `ctx.ql` is the engine and `getObject` is an in-memory registry read — no + * I/O, no extra row fetched, nothing loaded that the write path did not + * already have. When the context carries no engine (unit harnesses, embedders + * that hand-build a context) the record is simply merged without + * materialisation: the merge alone already fixes the common case, and a + * missing key then behaves exactly as it did before. + */ +function declaredFieldsFor(ctx: HookContext): Record | undefined { + const ql: any = (ctx as any).ql; + if (!ql || typeof ql.getObject !== 'function' || typeof ctx.object !== 'string') return undefined; + try { + const fields = ql.getObject(ctx.object)?.fields; + return fields && typeof fields === 'object' && !Array.isArray(fields) + ? (fields as Record) + : undefined; + } catch { + return undefined; + } +} + +/** + * Choose the record-shaped object the condition formula evaluates against. + * + * ## The record is the RECORD, not the patch (#4770) + * + * This used to return `ctx.input.data` — the fields the current write happens + * to carry — and `ctx.previous` was unreachable behind it, the two never + * merged. A condition could therefore only reference a field the update + * *happened* to touch; anything else aborted the CEL expression with + * `No such key` and was swallowed into `false` (see the condition gate above). + * For an audit-style hook (`record.done == true`) that meant the audit silently + * did not happen on exactly the ordinary updates — change the status, change + * the assignee — that leave `done` out of the payload. + * + * So the record is now **stored ⊕ payload**, made total over the object's + * declared fields — the same shape a validation predicate reads + * ({@link materializeDeclaredFields}, #1871/#4649). The two surfaces share one + * helper on purpose: `record.done == true` must not mean two different things + * depending on which of them evaluates it. + * + * Order, unchanged for shapes that are not write-like: + * 1. `ctx.input.data` (⊕ `ctx.previous`) — write operations carry the patch + * here and the pre-image there + * 2. `ctx.previous` — delete-shaped contexts carry only the pre-image + * 3. `ctx.input` — flat input bag (read ops, custom shapes) + * + * Materialisation is applied only when the record's persisted state is in hand + * — an insert (nothing to know) or an update whose prior row was fetched. + * A predicate bulk update carries no prior row, so its payload is left exactly + * as it is rather than gaining `null`s that contradict N stored rows. + * + * Copies, never mutates: `ctx.previous` and `ctx.input.data` are the engine's + * own objects, observed by the handlers that run after this gate. */ function pickRecordPayload(ctx: HookContext): any { const input: any = ctx.input ?? {}; - if (input && typeof input === 'object' && input.data && typeof input.data === 'object') { - return input.data; + const payload: Record | undefined = + input && typeof input === 'object' && input.data && typeof input.data === 'object' + ? (input.data as Record) + : undefined; + const prior: Record | undefined = + ctx.previous && typeof ctx.previous === 'object' + ? (ctx.previous as Record) + : undefined; + + if (payload) { + // No prior row and not an insert → the persisted state is unknown, so + // neither merging nor materialising is possible without inventing it. + if (!prior && !isInsertEvent(ctx.event)) return payload; + return materializeDeclaredFields({ ...prior, ...payload }, declaredFieldsFor(ctx)); } - if (ctx.previous && typeof ctx.previous === 'object') { - return ctx.previous; + if (prior) { + return materializeDeclaredFields({ ...prior }, declaredFieldsFor(ctx)); } return input; } diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index fc77b5e74e..83914e161f 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -126,6 +126,10 @@ import { type FieldValidationError, type ValidationMessageContext, } from './record-validator.js'; +// Shared with the declarative hook-condition gate (#4770) so the two surfaces +// 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'; type Mode = 'insert' | 'update'; @@ -477,35 +481,6 @@ function ruleNeedsPrior(r: unknown): boolean { return false; } -/** - * Materialise the object's DECLARED-but-absent fields as `null`, in place - * (#1871 for insert, #4649 for update and for the `previous` binding). - * - * CEL is strict about missing keys: `record.x` on a record that does not carry - * the key `x` aborts the whole predicate with `No such key`, which is NOT the - * same as reading `null`. Whether a key is carried is a property of the DRIVER, - * not of the data — a driver that stores only written columns returns a record - * missing every column the write never touched — so without this a predicate's - * evaluability depends on storage internals the author cannot see. - * - * Scope is deliberately the object's **declared fields only**. Materialising - * every key a predicate happens to name would defeat the fail-closed step: a - * typo'd `record.stauts` must stay unevaluable so it is reported, not silently - * read as `null` and quietly answered "no violation". - * - * `undefined` counts as absent (not just a missing key): CEL treats an own key - * holding `undefined` exactly as it treats no key at all. - */ -function materializeDeclaredFields( - record: Record, - fields: Record | undefined, -): Record { - if (!fields) return record; - for (const name of Object.keys(fields)) { - if (record[name] === undefined) record[name] = null; - } - return record; -} /** Field-level conditional rules (B2): a field is required / read-only when its * CEL predicate is TRUE over the record. */ diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index 7c0c99be80..217817594f 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -490,7 +490,8 @@ entered. Its CEL source sees exactly **three roots** — nothing else: **`record` and bare field names are NOT available and fail the node loudly.** Everywhere else on this platform `record` means "the record at event time" -(flow conditions: the trigger snapshot; hooks: the write payload) — at an +(flow conditions: the trigger snapshot; hook conditions: the stored record +overlaid with the write's payload, #4770) — at an approval node that phrase is ambiguous between two different times, so you must say which one: `current.x` or `trigger.x`. Do not carry the `record.x` habit over from conditions. diff --git a/skills/objectstack-data/references/data-hooks.md b/skills/objectstack-data/references/data-hooks.md index 07958757bb..689df53e38 100644 --- a/skills/objectstack-data/references/data-hooks.md +++ b/skills/objectstack-data/references/data-hooks.md @@ -231,6 +231,26 @@ condition: P`record.status in ['pending', 'in_review']` condition: P`record.type == 'enterprise' && record.region == 'APAC' && record.is_active == true` ``` +**`record` here is the RECORD, not this write's payload (#4770).** The condition +is evaluated against the stored row overlaid with the fields this write carries, +made total over the object's **declared** fields (`null` when a declared field is +in neither). So: + +- Reference **any** declared field, not just the ones this update touches — + `record.done == true` works on an update that only sets `status`. (This is + unlike `ctx.input` / `ctx.result` inside the handler, which stay partial — see + Gotcha 1.) +- A condition describes the record's **state**, not the diff. `record.done == + true` fires on every update of an already-done record, not only on the update + that set it. +- **Guard optional values with `!= null`, never with `has(...)`.** A declared + field holding `null` is *present*, so `has(record.spent)` is uniformly true and + `has(record.spent) && record.spent > record.budget` still faults on + `null > null`. `has()` answers "is this key declared at all", which is a + question about your spelling, not about your data. +- An **undeclared** key (a typo) stays unevaluable: the condition is logged at + WARN and treated as false. + #### `onError` — Error Handling The default is `'abort'` **unconditionally** — for `after*` hooks too, a sync @@ -418,8 +438,10 @@ const full = await ctx.api.object('candidate').findOne({ where: { id: ctx.result // full.position_id is present even though this PATCH only set `stage`. ``` -(A declarative `condition` on an un-written field hits the same wall — guard it -with the missing-key-safe `has(record.x)` macro.) +(A declarative `condition` does **not** hit this wall — since #4770 it is +evaluated against the stored record overlaid with the payload, so +`record.position_id` is readable there even when the PATCH never wrote it. Guard +optional values with `record.x != null`, not with `has(record.x)`.) ### ⚠️ Gotcha 2 — cross-object writes obey the *target's* sharing model