From 5306d5ba6c34f51866f05ec511eb7d8342d0fae8 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 4 Aug 2026 13:36:36 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(objectql)!:=20=E6=89=B9=E9=87=8F?= =?UTF-8?q?=E5=86=99=E6=8C=89=E8=A1=8C=E8=AF=AD=E4=B9=89=20=E2=80=94?= =?UTF-8?q?=E2=80=94=20after=20=E5=9E=8B=20hook=20=E6=8C=89=E8=A1=8C?= =?UTF-8?q?=E8=A7=A6=E5=8F=91,trigger=20=E6=8C=89=E8=A1=8C=E7=BB=91?= =?UTF-8?q?=E5=AE=9A=20previous/record=20(#5038)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2026-08-04 维护者在 #4800 / #4862 上的裁定,已记入 ADR-0058 的 bulk-write addendum:**一次 predicate(`multi: true`)写就是 N 次记录变更**,所以其上每一条 以记录为作用域的声明都按行求值 —— `record` 为该行的实际状态,`previous` 为该行的 前置状态。validation 侧自 #3106 起就是这个形状;hook `condition` 与骑在同一批 生命周期 hook 上的 record-change flow trigger 现在与之对齐。 原先的问题:`multi: true` 更新走到 `driver.updateMany`,它只解析出受影响行数。 生命周期 hook 只触发**一次**,`previous` 从不赋值(只有单 id 分支会取前置行), `record` 退化成本次写入的裸 payload。于是文档、formula skill 和 showcase 里十条 flow 共同教的过渡条件 `status == "done" && previous.status != "done"`,在批量写上 根本无法求值:hook 条件会拒绝该次写入(#4775/#5037),而 flow trigger **是静默的** —— 要么不触发,要么按一条并不存在的记录触发一次。审计行的缺失是最难被发现的那种 故障。 实现:engine 的批量 update / delete 分支现在把匹配行集**一次性**读出(复用 #3106 已有的那次 `driver.find`,只是把「本对象有 after 型 hook」并入它的取数条件), 然后按匹配行逐行派发 `afterUpdate` / `afterDelete`,每次的上下文都是**单记录形状**: `input.id` 为该行、`previous` 为其前置像、`result` 为其状态。这正是 #2922 为批量 INSERT 定下的规则的复述,也是本修复在消费端一行代码都不需要改的原因 —— hook-wrappers 的 `record`/`previous` 绑定、record-change trigger 的上下文构造、 plugin-audit 的 diff 读的都是这四个字段,在生产端修好即全部随之正确。 - 按行派发对 after 型 hook **统一生效**,刻意不以「条件文本里有没有 previous」为 开关 —— 裁定明确否决了那种让触发次数取决于条件字符串的隐性规则。 - `ctx.result` 按行为**该行**,由已在手的前置像 `row ⊕ payload` 组合而成,因此整批 仍只多一次查询,而非每行一次。批量 DELETE 没有后态,其按行上下文不设 `result`, 消费方回落到 `previous`。 - `onError` 无需新的按行含义 —— 它管的是跑在记录作用域上下文里的 handler,而按行 派发正是终于给了它这样一个上下文:`abort` 失败整个操作,`log` 吞掉该行、批次继续。 - 资源上限以**拒绝**落地:匹配行数超过 10 000 时,对有 after 型 hook 的对象的 predicate 写在调用 driver **之前**被拒(`ERR_BULK_PER_ROW_HOOK_LIMIT`),不写入 任何数据;绝不降级为整批一次派发 —— 那会静默漏掉 N-1 行的 hook。 对 hook 作者是破坏性变更,但方向正是契约声明的方向:接受 predicate 写的对象上的 after 型 hook,现在按匹配行触发而不是按批次触发(通知类 hook 发 N 条,缓存失效类 执行 N 次)。没有 after 型 hook 的对象完全不受影响,也不付任何额外读取成本。写入 自身的契约未变 —— predicate 写仍返回受影响行数,仍只发布一条聚合的 `data.records.updated`(#4639)。 `before*` hook 仍是批次作用域,且这不是待补的缺口:`beforeUpdate` / `beforeDelete` 之所以整批触发一次,是因为它们仍可能改写 payload,而一次 `updateMany` 只有一份 payload。因此 #5037 的 `HookConditionError` 与其 `limitation` 判别字段**保留并 收窄到该次派发**,文案不再承诺一个已经兑现的过期时间,而是点名 phase 就是原因, 并指向对应的 `after*` 事件 —— 在那里同一条件按行求值、与作者所写完全一致。文案现在 也把 record-change flow trigger 列为真实出路:#5037 依据实测拒绝这么写(当时该 trigger 共享同样未绑定的 `previous`),而这个事实已经改变。 文档(`data-modeling/formulas.mdx`)与 `skills/objectstack-formula` §5 同步收口: 两种写入形式共用一种过渡写法,并单独点出 `before*` 这个例外。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 --- .../bulk-write-per-row-hook-semantics.md | 71 +++ content/docs/data-modeling/formulas.mdx | 37 +- .../0058-expression-and-predicate-surface.md | 94 ++- examples/app-showcase/package.json | 1 + .../test/bulk-write-transition-flows.test.ts | 216 +++++++ .../src/bulk-write-per-row-hooks.test.ts | 540 ++++++++++++++++++ packages/objectql/src/engine.ts | 213 ++++++- .../src/hook-condition-bulk-previous.test.ts | 262 ++++++--- .../src/hook-condition-fail-loud.test.ts | 34 +- .../src/hook-condition-previous-scope.test.ts | 34 +- packages/objectql/src/hook-wrappers.ts | 203 ++++--- .../src/bulk-write-per-row-context.test.ts | 273 +++++++++ pnpm-lock.yaml | 3 + scripts/adr-anchors.json | 2 +- skills/objectstack-formula/SKILL.md | 57 +- 15 files changed, 1830 insertions(+), 210 deletions(-) create mode 100644 .changeset/bulk-write-per-row-hook-semantics.md create mode 100644 examples/app-showcase/test/bulk-write-transition-flows.test.ts create mode 100644 packages/objectql/src/bulk-write-per-row-hooks.test.ts create mode 100644 packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts diff --git a/.changeset/bulk-write-per-row-hook-semantics.md b/.changeset/bulk-write-per-row-hook-semantics.md new file mode 100644 index 0000000000..9f61299973 --- /dev/null +++ b/.changeset/bulk-write-per-row-hook-semantics.md @@ -0,0 +1,71 @@ +--- +"@objectstack/objectql": minor +"@objectstack/example-showcase": patch +--- + +feat(objectql)!: a predicate bulk write evaluates and fires after-hooks PER ROW (#5038) + +The 2026-08-04 maintainer ruling on #4800 / #4862, recorded as ADR-0058's +bulk-write addendum: **a bulk write is N record changes**, so every record-scoped +declaration on it is evaluated per row — `record` = that row's state, `previous` = +that row's pre-write state. Validation predicates have worked this way since +#3106; hook `condition`s and the record-change flow triggers riding the same +lifecycle hooks now join them. + +**What was broken.** A `multi: true` update reaches `driver.updateMany`, which +resolves an affected COUNT. The lifecycle hook fired **once**, `previous` was +never assigned (only the single-id branch fetched a prior row), and `record` +degraded to the write's bare payload. So the transition condition the docs, the +formula skill and ten showcase flows all teach — +`status == "done" && previous.status != "done"` — could not be evaluated on a +bulk write. Hook conditions rejected the write (#4775/#5037); record-change flow +triggers were **silent**, firing zero times or once for a record that did not +exist. A missing audit row is the one failure nobody goes looking for. + +**What changed.** The engine's bulk `update` / `delete` branches now read the +matched row set **once** — the same `driver.find` #3106 already issues, with +"this object has after-hooks" added to its demand test — and dispatch +`afterUpdate` / `afterDelete` once per matched row, each on a context with the +**single-record shape**: `input.id` = the row, `previous` = its pre-image, +`result` = its state. That is #2922's batch-INSERT ruling restated, and it is why +this fix has no code in the consumers: `hook-wrappers`' `record`/`previous` +bindings, the record-change trigger's context builder and plugin-audit's diff all +read those same fields and became correct at the producer. + +- **Per-row dispatch is uniform across after-hooks.** It is deliberately NOT + keyed on whether a condition mentions `previous` — the ruling rejected that as + a hidden rule that would make a hook's firing count depend on its condition + text. +- **`ctx.result` per row is the ROW**, composed as `row ⊕ payload` from the + pre-image already in hand, so the batch still costs one extra query, not one + per row. A bulk DELETE has no post-state: its per-row context sets no `result`, + and consumers fall back to `previous`. +- **`onError` needed no new meaning** — it governs a handler on a record-scoped + context, which is now what it always gets: `abort` fails the operation, `log` + swallows that row and the batch continues. +- **A ceiling, enforced as a refusal.** Past 10 000 matched rows a predicate + write against an object with after-hooks is rejected *before* the driver call + (`ERR_BULK_PER_ROW_HOOK_LIMIT`), so nothing is written. It is never downgraded + to one dispatch for the batch — that would skip the hook for N-1 rows silently. + +**Breaking for hook authors, in the direction the contract declares.** An +after-hook on an object that takes predicate writes now runs once per matched row +instead of once per batch: a notification hook sends N messages, a +cache-invalidation hook runs N times. Objects with no after-hooks are untouched +and pay for no extra read. The write's own contract is unchanged — a predicate +write still resolves the affected count and still publishes ONE aggregate +`data.records.updated` (#4639). + +**`before*` hooks stay batch-scoped, and that is not a gap.** `beforeUpdate` / +`beforeDelete` fire once for the whole batch because they may still rewrite the +payload, and one `updateMany` carries one payload. #5037's `HookConditionError` +and its `limitation` discriminator therefore **survive, rescoped to that +dispatch** — with a message that no longer promises an expiry that has already +happened, names the phase as the reason, and points at the matching `after*` +event where the same condition evaluates per row as authored. It also now names a +record-change flow trigger as a real route: #5037 refused to, on measured +evidence that the trigger shared the same unbound `previous`; that fact changed. + +Docs (`data-modeling/formulas.mdx`) and `skills/objectstack-formula` §5 are +updated to teach one transition shape for both write forms, with the `before*` +exception called out. diff --git a/content/docs/data-modeling/formulas.mdx b/content/docs/data-modeling/formulas.mdx index b0fb6f30da..1bde946605 100644 --- a/content/docs/data-modeling/formulas.mdx +++ b/content/docs/data-modeling/formulas.mdx @@ -200,7 +200,7 @@ Keep them pure, dependency-free, and AI-readable. | Binding | Source | Available in | |:---|:---|:---| | `record` | the row being evaluated | formulas, validation, sharing, visibility | -| `previous` | row before update | hooks, validation on update | +| `previous` | row before update — on a `multi: true` write, that row's own pre-write state in `after*` hooks / record-change triggers (per row); unbound in `before*` hooks, which fire once for the batch | hooks, validation on update | | `input` | hook payload | hooks | | `current_user` | the authenticated subject — the canonical binding (ADR-0068). `user`, `ctx.user` and `os.user` are aliases of the **same** object | predicates with identity | | `os.user` | alias of `current_user` | seed, predicates with identity | @@ -349,6 +349,41 @@ JS `body` is a different surface: it is wrapped as `new AsyncFunction('ctx', sou so inside it the record is `ctx.input` (there is no bare `record`), and every `ctx` API it touches must be covered by a declared capability. +#### Transitions on bulk writes + +The condition above is a **transition** — `record.status == 'escalated'` alone +would be true on every update of an already-escalated case, so "just became" is +only expressible by comparing against `previous`. + +Write it once. A predicate (`multi: true`) write is N record changes, so +`after*` hooks — and the record-change flow triggers that ride them — are +evaluated and fired **once per matched row**, with `previous` bound to that +row's own pre-write state and `record` holding that row's real state rather than +the write's payload. The same condition therefore means the same thing whether +the write targets one id or matches a thousand rows: + +```ts +await data.update('case', { status: 'escalated' }, { multi: true, where: { severity: 'high' } }); +// → `notify_on_escalation` fires once per case that ACTUALLY transitioned; +// cases already escalated do not fire. +``` + +The matched rows are read once for the whole batch and reused for every per-row +evaluation, so this costs one extra query per write, not one per row. Above +~10 000 matched rows a predicate write against an object with `after*` hooks is +**refused** rather than fanned out — paginate the write. The refusal is loud; +the platform never silently downgrades it to a single hook call. + + +`before*` hooks are the exception, by nature rather than by omission. +`beforeUpdate` / `beforeDelete` fire **once for the whole batch** — they may +still rewrite the payload, and a bulk write carries exactly one payload — so +`previous` is unbound there and a `before*` condition that reads it fails the +write with an error naming the batch and pointing at the matching `after*` +event. Keep transition conditions on `after*`; keep `before*` conditions to the +fields the incoming payload actually sets. + + --- ## Formula patterns diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index 18e4379c64..6e6e34e65d 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -56,10 +56,10 @@ --- -> **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._ +> **Addendum (2026-08, #4800 / #4862 / #5037 / #5038) — BULK-WRITE SCOPE: on a +> predicate (`multi: true`) write, after-hooks and record-change flow triggers +> evaluate and fire PER ROW.** _Contract recorded here; **implemented by #5038** +> (see "How it landed" below). `before*` hooks are outside it, by nature._ > > 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 @@ -76,37 +76,69 @@ > 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 +> **What the engine did before #5038, 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**. +> hook fired **once**, `hookContext.previous` was never assigned (only the +> single-id branch fetched a prior row), and `record` degraded to the write's +> bare payload. So a condition naming `previous` was unevaluable and — since the +> #4775 row above — **rejected the write**. The rc window (#5037) kept the +> rejection (fail loud takes no exception; logging-and-skipping was considered +> and refused on #4800, because a missing audit row is the one failure nobody +> goes looking for) but made it name the limitation instead of the author. > -> **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. +> **How it landed (#5038).** The engine's bulk branch reads the matched row set +> **once** — the same `driver.find` #3106 already issues for per-row validation, +> now also demanded when the object has after-hooks — and then dispatches +> `afterUpdate` / `afterDelete` **once per matched row**, on a context with the +> single-record shape: `input.id` = the row, `previous` = its pre-image, +> `result` = its state. That shape is #2922's ruling for batch INSERT restated +> (a single array-shaped context "broke every consumer built for the single +> shape"), and it is why the fix has no code in the consumers: `hook-wrappers`' +> `record`/`previous` bindings, the record-change trigger's `buildContext` and +> plugin-audit's diff all read those same fields and became correct at the +> producer. The write's own contract is untouched — a predicate write still +> resolves an affected count (#4639), and still publishes ONE aggregate +> `data.records.updated`, because per-row dispatch changed hook granularity, not +> what the write is. > -> **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. +> **The consequences, priced as this addendum required.** > -> **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. +> - **`ctx.result` per row is the ROW, not the batch** — composed as +> `row ⊕ payload` from the pre-image already in hand, so the guardrail above +> ("read the row set once") stays literal: no second full-set query after the +> write. A bulk DELETE has no post-state, so its per-row context sets no +> `result` and consumers fall back to `previous`, which is what `record` means +> for a delete. +> - **`onError` needed no per-row meaning.** It governs a HANDLER on a +> record-scoped context, and per-row dispatch is what finally gives it one: +> `abort` propagates and fails the operation (as on the single-record and +> batch-insert paths), `log` swallows that row and the batch continues. +> - **The ceiling is a refusal, not a downgrade.** Past +> `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000) a predicate write against an object +> with after-hooks is rejected BEFORE the driver call, so nothing is written. +> Falling back to one dispatch for the batch would skip the hook for N-1 rows +> silently — the failure shape this whole family exists to abolish. +> +> **`before*` hooks are NOT per row, and that is not a version gap.** A +> `beforeUpdate` / `beforeDelete` fires once for the whole batch because it may +> still rewrite the payload, and one `updateMany` carries one payload — there is +> nothing per-row to hand it. So #5037's `HookConditionError` and its +> `limitation` discriminator (`bulk_write_previous_unbound`, +> `bulk_write_stored_state_unavailable`) **survive, rescoped to that dispatch**, +> and their message no longer promises an expiry that has already happened: it +> names the phase as the reason and points at the matching `after*` event, where +> the same condition evaluates per row exactly as authored. Authors put +> transition conditions on `after*`; `before*` conditions stay over the incoming +> payload. +> +> **One refusal reversed on evidence.** #5037 deliberately did NOT offer "use a +> record-change flow trigger instead", because that trigger subscribes to these +> very lifecycle hooks and so fired once with the same unbound `previous` +> (#4862) — naming it would have made the error that fixes a +> `declared ≠ delivered` into another one. #5038 fixed it at the producer, so an +> after-type record-change trigger now rides the per-row dispatch and the route +> is real. The message names it because the fact changed, not because the +> constraint was relaxed. --- diff --git a/examples/app-showcase/package.json b/examples/app-showcase/package.json index 8c152dfa6e..851e7e639f 100644 --- a/examples/app-showcase/package.json +++ b/examples/app-showcase/package.json @@ -35,6 +35,7 @@ }, "devDependencies": { "@objectstack/cli": "workspace:*", + "@objectstack/formula": "workspace:*", "@objectstack/objectql": "workspace:*", "@playwright/test": "^1.62.1", "typescript": "^6.0.3", diff --git a/examples/app-showcase/test/bulk-write-transition-flows.test.ts b/examples/app-showcase/test/bulk-write-transition-flows.test.ts new file mode 100644 index 0000000000..d166f6d522 --- /dev/null +++ b/examples/app-showcase/test/bulk-write-transition-flows.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4862 / #5038] The showcase's transition flows, verified against the per-row + * bulk-write contract. + * + * #4862 named this app as the evidence that the gap was not theoretical: about + * ten flows shipped here gate on `status == "done" && previous.status != "done"` + * — the exact shape `content/docs/data-modeling/formulas.mdx` and + * `skills/objectstack-formula` §5 teach. On a predicate (`multi: true`) write + * the engine fired the lifecycle hook ONCE, never bound `previous`, and let + * `record` degrade to the write's bare payload, so every one of these start + * conditions was unevaluable on a bulk "mark these done" — the flows either did + * not fire or fired once for a record that did not exist, silently. + * + * ADR-0058's bulk-write addendum settled the contract and #5038 implemented it: + * a bulk write evaluates and fires after-hooks (and the record-change triggers + * riding them) PER ROW, with `previous` = that row's pre-write state. + * + * What this file checks is the app's own metadata, not a copy of it: it reads + * the REAL flows out of `allFlows` and evaluates their REAL start conditions + * through the same evaluator and the same variable shape the automation engine + * builds (`AutomationEngine.evaluateCondition` spreads the record's fields to + * top level and binds `record` + `previous`). Two rows per flow, matched by ONE + * predicate write: + * + * - a row that genuinely transitioned → the condition must be TRUE; + * - a row already in the target state → the condition must be FALSE. + * + * That second row is the whole point of `previous`, and it is the assertion the + * old behaviour could not satisfy on a batch at all: with `previous` unbound, + * the condition faulted for both rows; with `previous` fabricated as `{}` or + * `null`, it answered the same for both. + */ + +import { describe, it, expect } from 'vitest'; +import { ExpressionEngine } from '@objectstack/formula'; + +import { allFlows } from '../src/automation/flows/index.js'; + +type StartNode = { type?: string; config?: Record }; +type FlowLike = { name?: string; nodes?: StartNode[] }; + +/** The record-change start node of a flow, if it has one. */ +function startNodeOf(flow: FlowLike): Record | undefined { + const start = (flow.nodes ?? []).find((n) => n?.type === 'start'); + return start?.config as Record | undefined; +} + +function conditionSourceOf(config: Record | undefined): string | undefined { + const c = config?.condition; + if (typeof c === 'string') return c; + if (c && typeof c === 'object' && typeof (c as { source?: unknown }).source === 'string') { + return (c as { source: string }).source; + } + return undefined; +} + +/** + * Evaluate a start condition exactly as `AutomationEngine.evaluateCondition` + * does: every record field spread to top level (so bare `status` resolves), + * plus `record` and `previous`. + */ +function evaluateStartCondition( + source: string, + record: Record, + previous: Record | null, +): boolean { + const vars: Record = { ...record, record, previous }; + const result = ExpressionEngine.evaluate( + { dialect: 'cel', source }, + { extra: { ...vars, vars }, record: vars }, + ); + if (!result.ok) { + throw new Error(`condition did not evaluate: ${result.error?.message ?? 'unknown'} — ${source}`); + } + return Boolean(result.value); +} + +/** + * A `field == "value" && previous.field != "value"` transition, parsed out of + * the condition so the two rows can be GENERATED from the flow's own text + * rather than hand-copied into this file (a copy would drift the moment a flow + * changed its target state, and pass while doing so). + */ +const TRANSITION_RE = /^\s*(\w+)\s*==\s*(['"])(.+?)\2\s*&&\s*previous\.(\w+)\s*!=\s*(['"])(.+?)\5(.*)$/; + +interface Transition { + flow: string; + source: string; + field: string; + target: string; + /** Any further clauses (`&& total_amount >= 5000`), kept so they are satisfied. */ + tail: string; +} + +/** Fields the tail clauses of showcase transitions reference, set generously so + * an extra clause is satisfied rather than accidentally deciding the test. */ +const TAIL_SUPPORT = { + total_amount: 10_000, + budget: 500_000, + amount: 10_000, +}; + +const transitions: Transition[] = []; +const previousReaders: Array<{ flow: string; source: string }> = []; + +for (const flow of allFlows as unknown as FlowLike[]) { + const source = conditionSourceOf(startNodeOf(flow)); + if (!source || !/\bprevious\b/.test(source)) continue; + previousReaders.push({ flow: String(flow.name), source }); + const m = TRANSITION_RE.exec(source); + if (m && m[1] === m[4] && m[3] === m[6]) { + transitions.push({ flow: String(flow.name), source, field: m[1], target: m[3], tail: m[7] }); + } +} + +describe('[#4862/#5038] showcase transition flows on a predicate bulk write', () => { + it('the showcase really does ship ~10 of them — this is not a vacuous suite', () => { + // #4862 counted "at least 10 flows" on this shape. If a refactor drops them + // all, the per-flow cases below would pass by iterating nothing. + expect(transitions.length).toBeGreaterThanOrEqual(10); + // And every `previous`-reading start condition is accounted for: either it + // is one of the canonical transitions below, or it is covered by the + // explicitly enumerated shapes at the end of this file. + expect(previousReaders.length).toBeGreaterThanOrEqual(transitions.length); + }); + + describe.each(transitions)('$flow — `$source`', ({ field, target, tail, source }) => { + const base = { id: 'row_1', title: 'A showcase row', ...TAIL_SUPPORT }; + + it('FIRES for the row that transitioned in this batch', () => { + // Per-row bindings, as the engine now supplies them: `previous` is THIS + // row's pre-write state, `record` is its real state (stored ⊕ payload), + // not the write's bare payload. + const previous = { ...base, [field]: `not_${target}` }; + const record = { ...base, [field]: target }; + + expect(evaluateStartCondition(source, record, previous)).toBe(true); + }); + + it('does NOT fire for a row already in the target state', () => { + // Same batch, same payload, different prior state. Only a per-row + // `previous` can tell these two rows apart — the discrimination the flow + // was authored to make and could not make on a bulk write before #5038. + const previous = { ...base, id: 'row_2', [field]: target }; + const record = { ...base, id: 'row_2', [field]: target }; + + expect(evaluateStartCondition(source, record, previous)).toBe(false); + }); + + it('reads a field the bulk payload does not set, so `record` must be the row', () => { + // `record` degrading to the bare payload was #4862's fact 4. A predicate + // write that sets ONLY the transition field still has to answer for the + // rest of the row: `title` is never in such a payload, and every one of + // these conditions is evaluated against a record that has it. + const previous = { ...base, [field]: `not_${target}` }; + const record = { ...base, [field]: target }; + expect(record.title).toBe('A showcase row'); + expect(evaluateStartCondition(source, record, previous)).toBe(true); + // …and the tail clause, when there is one, was genuinely satisfied rather + // than skipped. + if (tail.trim()) expect(tail).toMatch(/&&|\|\|/); + }); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * The non-canonical `previous` shapes the showcase also ships + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#4862/#5038] the showcase\'s other `previous` start conditions', () => { + const find = (name: string) => { + const flow = (allFlows as unknown as FlowLike[]).find((f) => f.name === name); + const source = conditionSourceOf(startNodeOf(flow ?? {})); + if (!source) throw new Error(`flow '${name}' has no start condition — update this test`); + return source; + }; + + it('a field-CHANGED transition (`assignee != previous.assignee`) discriminates per row', () => { + const source = find('showcase_task_assigned_notify'); + expect(source).toContain('previous.assignee'); + + // One batch reassigning a set of tasks to `dana`: the row that already + // belonged to dana did not change hands, and must not notify her again. + expect(evaluateStartCondition(source, { assignee: 'dana' }, { assignee: 'ann' })).toBe(true); + expect(evaluateStartCondition(source, { assignee: 'dana' }, { assignee: 'dana' })).toBe(false); + }); + + it('a threshold-crossing transition (`budget > N && budget != previous.budget`)', () => { + const source = find('showcase_budget_approval'); + expect(source).toContain('previous.budget'); + + expect(evaluateStartCondition(source, { budget: 200_000 }, { budget: 50_000 })).toBe(true); + // Already over the threshold and unchanged by this write — not a crossing. + expect(evaluateStartCondition(source, { budget: 200_000 }, { budget: 200_000 })).toBe(false); + }); + + it('the create-OR-escalate shape (`previous == null || previous.priority != …`)', () => { + // #3427: on a `record-after-write` flow, `previous == null` is the CREATE + // leg. That discrimination is only honest when `previous` is genuinely + // absent for an insert and genuinely PRESENT for a bulk update — before + // #5038 a bulk update also arrived with no `previous`, so every batched + // escalation looked like a brand-new urgent record. + const source = find('showcase_urgent_task_alert'); + expect(source).toContain('previous == null'); + + // The create leg. + expect(evaluateStartCondition(source, { priority: 'urgent' }, null)).toBe(true); + // A real escalation inside a batch. + expect(evaluateStartCondition(source, { priority: 'urgent' }, { priority: 'normal' })).toBe(true); + // Already urgent — must NOT re-alert, and only a bound per-row `previous` + // can say so. + expect(evaluateStartCondition(source, { priority: 'urgent' }, { priority: 'urgent' })).toBe(false); + }); +}); diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts new file mode 100644 index 0000000000..778eb0db47 --- /dev/null +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -0,0 +1,540 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5038] A predicate (`multi: true`) write evaluates and fires its after-hooks + * PER ROW. + * + * The contract recorded as ADR-0058's bulk-write addendum (the 2026-08-04 + * maintainer ruling on #4800 / #4862): 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. Validation + * predicates have worked this way since #3106; hook `condition`s — and the + * record-change flow triggers riding the same lifecycle hooks — now join them. + * + * What used to happen instead, measured on #4862: `driver.updateMany` resolves + * an affected COUNT, the lifecycle hook fired ONCE, `hookContext.previous` was + * never assigned, and `record` degraded to the write's bare payload. So the + * transition condition the docs and ten showcase flows teach + * (`status == "done" && previous.status != "done"`) could not be evaluated on a + * bulk write, and the audit / notification automations behind it silently did + * not happen — the one failure shape nobody goes looking for. + * + * Pinned here: + * 1. firing GRANULARITY — N matched rows ⇒ N dispatches, uniformly for every + * after-hook, never keyed on whether the condition text says `previous`; + * 2. the per-row BINDINGS — `previous` is that row's pre-image, `record` is + * that row's real state (not the bare payload), `input.id` names the row; + * 3. the performance GUARDRAIL — the matched row set is read exactly ONCE and + * reused across every per-row evaluation, and is not read at all when the + * object has no after-hooks; + * 4. the resource CEILING — an oversized batch is refused before anything is + * written, never silently downgraded to one call; + * 5. `onError` and the write's own return contract, both unchanged; + * 6. the same contract on a bulk DELETE. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.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 }, + owner: { name: 'owner', label: 'Owner', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, +}; +const taskObject = { name: 'task', label: 'Task', fields: TASK_FIELDS }; +const otherObject = { name: 'other', label: 'Other', fields: TASK_FIELDS }; + +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +/** The transition shape the docs, the formula skill and the showcase all teach. */ +const TRANSITION = 'record.status == "done" && previous.status != "done"'; + +/* ──────────────────────────────────────────────────────────────────────────── + * 1. Firing granularity + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] a bulk write fires after-hooks once per matched row', () => { + it('N matched rows ⇒ N dispatches', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row', 'afterUpdate', (ctx) => { + seen.push(String((ctx.input as any).id)); + })]); + + await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + { title: 'c', status: 'todo' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toHaveLength(3); + // Each dispatch names a DIFFERENT row — this is N record changes, not one + // hook call repeated. + expect(new Set(seen).size).toBe(3); + }); + + it('is UNIFORM — a condition that never mentions `previous` fires per row too', async () => { + // The ruling rejected keying per-row dispatch on the condition text + // explicitly: a hook's firing COUNT must not depend on what its condition + // happens to say, because no author can infer that from any declaration. + // Two hooks, one reading `previous` and one not, must fire the same number + // of times on the same write. + const withPrevious: string[] = []; + const withoutPrevious: string[] = []; + const { engine } = await boot([ + hook('reads_previous', 'afterUpdate', (ctx) => { withPrevious.push(String((ctx.input as any).id)); }, TRANSITION), + hook('reads_record', 'afterUpdate', (ctx) => { withoutPrevious.push(String((ctx.input as any).id)); }, 'record.status == "done"'), + ]); + + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(withPrevious).toHaveLength(2); + expect(withoutPrevious).toHaveLength(2); + }); + + it('a hook with NO condition fires per row as well', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('uncondtional', 'afterUpdate', (ctx) => { + seen.push(String((ctx.input as any).id)); + })]); + + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toHaveLength(2); + }); + + it('zero matched rows ⇒ ZERO dispatches (a batch that changed nothing is not a record change)', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row', 'afterUpdate', () => { seen.push('x'); })]); + + await seedTasks(engine, [{ title: 'a', status: 'done' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'nothing_matches' } } as any); + + expect(seen).toEqual([]); + }); + + it('a single-record write still fires exactly once', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row', 'afterUpdate', () => { seen.push('x'); })]); + const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); + + await engine.update('task', { status: 'done' }, { where: { id: row.id } } as any); + + expect(seen).toEqual(['x']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 2. Per-row bindings + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] each dispatch carries THAT row\'s previous / record', () => { + it('`previous` is the row\'s own pre-write state, not a shared one', async () => { + const priors: Array | undefined> = []; + const { engine } = await boot([hook('capture', 'afterUpdate', (ctx) => { + priors.push(ctx.previous as Record); + })]); + + await seedTasks(engine, [ + { title: 'a', status: 'todo', owner: 'ann' }, + { title: 'b', status: 'blocked', owner: 'bob' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: {} } as any); + + expect(priors).toHaveLength(2); + // One payload, N DIFFERENT prior states — the #3106 shape, now on the hook + // side. The pre-images differ in exactly the fields the rows differed in. + expect(priors.map((p) => p?.status).sort()).toEqual(['blocked', 'todo']); + expect(priors.map((p) => p?.owner).sort()).toEqual(['ann', 'bob']); + }); + + it('the condition discriminates per row — only real transitions fire', async () => { + // The whole reason `previous` exists. `record.status == "done"` alone is + // true for the already-done row too; the transition is not. + const fired: string[] = []; + const { engine } = await boot([hook('transition', 'afterUpdate', (ctx) => { + fired.push(String((ctx.previous as any).title)); + }, TRANSITION)]); + + await seedTasks(engine, [ + { title: 'just_finished', status: 'todo' }, + { title: 'already_done', status: 'done' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: {} } as any); + + expect(fired).toEqual(['just_finished']); + }); + + it('`record` is the row\'s REAL state, not the bare payload (#4862 fact 4)', async () => { + // The payload sets only `status`. Before per-row dispatch, `record` WAS the + // payload, so `record.title` / `record.owner` — fields this write does not + // touch — were simply absent and any condition naming one was unevaluable. + const records: Array> = []; + const { engine } = await boot([hook('capture', 'afterUpdate', (ctx) => { + records.push(ctx.result as Record); + }, 'record.status == "done" && record.owner == "ann"')]); + + await seedTasks(engine, [ + { title: 'hers', status: 'todo', owner: 'ann' }, + { title: 'his', status: 'todo', owner: 'bob' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: {} } as any); + + // The condition could only select Ann's row by reading a field the write + // never carried — which is what "the row's real state" means. + expect(records).toHaveLength(1); + expect(records[0].title).toBe('hers'); + expect(records[0].owner).toBe('ann'); + // …overlaid with what this write applied. + expect(records[0].status).toBe('done'); + }); + + it('`input.id` names the row, giving the single-record context shape (#2922)', async () => { + const ids: unknown[] = []; + const { engine } = await boot([hook('capture', 'afterUpdate', (ctx) => { + ids.push((ctx.input as any).id); + // …and the payload is still reachable, as on any single-record write. + expect((ctx.input as any).data.status).toBe('done'); + })]); + + const rows = await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: {} } as any); + + expect(ids.sort()).toEqual(rows.map((r: any) => r.id).sort()); + }); + + it('a per-row handler mutating `input` does not leak into the next row', async () => { + // The batch has one payload, but each dispatch gets its own shallow copy so + // an after-handler writing through the flat-input proxy cannot rewrite what + // the next row's handler sees. + const observed: unknown[] = []; + const { engine } = await boot([hook('mutate', 'afterUpdate', (ctx) => { + observed.push((ctx.input as any).scribble); + (ctx.input as any).scribble = 'was here'; + })]); + + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + await engine.update('task', { status: 'done' }, { multi: true, where: {} } as any); + + expect(observed).toEqual([undefined, undefined]); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 3. The performance guardrail + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] the matched row set is read ONCE', () => { + it('one `find` serves every per-row evaluation, however many rows matched', async () => { + const { engine, driver } = await boot([hook('per_row', 'afterUpdate', () => {}, TRANSITION)]); + await seedTasks(engine, [ + { title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }, + { title: 'c', status: 'todo' }, { title: 'd', status: 'todo' }, + ]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + // Four rows, four hook dispatches, ONE query. Re-reading per row would make + // a bulk write cost N round trips — the guardrail the issue set. + expect(driver.findCalls).toHaveLength(1); + }); + + it('does NOT read the row set when the object has no after-hooks', async () => { + // The read is DEMAND-driven: an object nobody hooks pays nothing for a + // contract it cannot observe. + const { engine, driver } = await boot([hook('elsewhere', 'afterUpdate', () => {}, undefined, 'other')]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(driver.findCalls).toHaveLength(0); + }); + + it('reads it once for an object-filtered hook that DOES match', async () => { + const seen: string[] = []; + const { engine, driver } = await boot([hook('here', 'afterUpdate', () => { seen.push('x'); }, undefined, 'task')]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(driver.findCalls).toHaveLength(1); + expect(seen).toEqual(['x']); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 4. The resource ceiling + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] an oversized matched set is refused, never silently downgraded', () => { + it('rejects past the ceiling, naming the count, the limit and the routes out', async () => { + const fired: string[] = []; + const { engine } = await boot([hook('per_row', 'afterUpdate', () => { fired.push('x'); })]); + + const over = ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS + 1; + await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'todo' }))); + + const err = await engine + .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('ERR_BULK_PER_ROW_HOOK_LIMIT'); + expect(err.matched).toBe(over); + expect(err.limit).toBe(ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS); + expect(err.message).toContain('PER ROW'); + expect(err.message).toContain('Narrow the predicate'); + // The refused alternative, named so nobody re-proposes it: firing once for + // the batch would skip the hook for N-1 rows without saying so. + expect(err.message).toContain('NOT silently downgraded'); + + // Nothing ran and nothing was written — the check is before the driver call. + expect(fired).toEqual([]); + const stillTodo = await engine.count('task', { where: { status: 'todo' } } as any); + expect(stillTodo).toBe(over); + }); + + it('does not apply to an object with no after-hooks — a big batch still writes', async () => { + const { engine } = await boot([]); + const over = ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS + 1; + await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'todo' }))); + + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + + expect(affected).toBe(over); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 5. What did NOT change + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] the write\'s own contract is untouched', () => { + it('a predicate update still resolves the affected COUNT (#4639)', async () => { + const { engine } = await boot([hook('per_row', 'afterUpdate', () => {})]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + + // Per-row dispatch changed the HOOK granularity, not the write's return + // shape — a caller counting affected rows is unaffected. + expect(affected).toBe(2); + }); + + it('the rows are actually written', async () => { + const { engine } = await boot([hook('per_row', 'afterUpdate', () => {})]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(await engine.count('task', { where: { status: 'done' } } as any)).toBe(2); + }); + + it('`onError: abort` on a per-row handler fails the operation, as on every other path', async () => { + // `onError` needed no new per-row meaning: it governs a HANDLER on a + // record-scoped context, and per-row dispatch is what finally gives it one. + // The single-record and batch-insert paths both propagate; so does this. + const { engine } = await boot([hook('boom', 'afterUpdate', () => { throw new Error('handler exploded'); })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await expect( + engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any), + ).rejects.toThrow(/handler exploded/); + }); + + it('`onError: log` swallows per row and the remaining rows still fire', async () => { + const reached: string[] = []; + const { engine } = await boot([hook('noisy', 'afterUpdate', (ctx) => { + reached.push(String((ctx.input as any).id)); + throw new Error('handler exploded'); + }, undefined, 'task', { onError: 'log' })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + + expect(affected).toBe(2); + // Row 1's failure did not abort the batch's remaining dispatches. + expect(reached).toHaveLength(2); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 6. Bulk DELETE takes the same contract + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] a bulk delete fires afterDelete per row', () => { + it('one dispatch per deleted row, each carrying that row as `previous`', async () => { + const deleted: string[] = []; + const { engine } = await boot([hook('audit_delete', 'afterDelete', (ctx) => { + deleted.push(String((ctx.previous as any).title)); + })]); + + await seedTasks(engine, [ + { title: 'a', status: 'stale' }, + { title: 'b', status: 'stale' }, + { title: 'c', status: 'live' }, + ]); + + await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + + // The deleted rows are NAMED. Before this, a bulk delete fired once with a + // context that identified no row at all, so a `record-after-delete` flow + // could not say what it had just seen deleted. + expect(deleted.sort()).toEqual(['a', 'b']); + }); + + it('a delete-shaped condition evaluates against the deleted row', async () => { + const deleted: string[] = []; + const { engine } = await boot([hook('audit_done_deletes', 'afterDelete', (ctx) => { + deleted.push(String((ctx.previous as any).title)); + }, 'record.status == "done"')]); + + await seedTasks(engine, [ + { title: 'finished', status: 'done' }, + { title: 'abandoned', status: 'todo' }, + ]); + + await engine.delete('task', { multi: true, where: {} } as any); + + // `record` on a delete-shaped context is the row that was removed. + expect(deleted).toEqual(['finished']); + }); + + it('does not read the doomed rows when nothing hooks afterDelete', async () => { + const { engine, driver } = await boot([hook('other_event', 'afterUpdate', () => {})]); + await seedTasks(engine, [{ title: 'a', status: 'stale' }]); + + driver.findCalls.length = 0; + await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + + expect(driver.findCalls).toHaveLength(0); + }); + + it('the delete still resolves the affected count and removes the rows', async () => { + const { engine } = await boot([hook('audit_delete', 'afterDelete', () => {})]); + await seedTasks(engine, [{ title: 'a', status: 'stale' }, { title: 'b', status: 'stale' }]); + + const affected = await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + + expect(affected).toBe(2); + expect(await engine.count('task', {} as any)).toBe(0); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * Harness + * ──────────────────────────────────────────────────────────────────────────── */ + +function hook( + name: string, + event: string, + handler: (ctx: HookContext) => void, + condition?: string, + object = 'task', + extra: Record = {}, +): Hook { + return { + name, object, events: [event], priority: 100, + ...(condition ? { condition } : {}), + handler, + ...extra, + } as unknown as Hook; +} + +async function seedTasks(engine: ObjectQL, rows: Record[]): Promise { + const written = await engine.insert('task', rows as any); + return Array.isArray(written) ? written : [written]; +} + +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: {}, + /** Every `find` the engine issues, so a test can pin the read count. */ + findCalls: [] as unknown[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + d.findCalls.push(ast); + 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 Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).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 = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of rows) storeFor(o).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany(o: string, ast: any) { + const rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of rows) storeFor(o).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +async function boot(hooks: Hook[]): Promise<{ engine: ObjectQL; driver: any }> { + const engine = new ObjectQL(); + const driver = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + engine.registry.registerObject(otherObject as any); + if (hooks.length > 0) { + bindHooksToEngine(engine, hooks, { packageId: 'app:test', logger: silentLogger }); + } + return { engine, driver }; +} diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 044099b02b..a842ad70f4 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1136,6 +1136,131 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * [#5038] Would `triggerHooks(event, ctx)` reach ANY handler for `object`? + * + * Mirrors the per-object filter in `triggerHooks` exactly (an entry with no + * `object` is global; an array or `'*'` widens it), because this answer gates + * a READ of the whole matched row set on the bulk write path. Getting it + * looser than the dispatch loop only costs a wasted query; getting it + * TIGHTER would silently drop hooks that were going to fire, so the two must + * be read together. + * + * `session.skipAutomations` is deliberately NOT consulted: it suppresses only + * metadata-bound entries, and code-registered hooks (audit, sharing) still + * run, so the row set is still needed. Over-reading in that case is a cost, + * never a correctness loss. + */ + private hasHooksFor(event: string, object: string): boolean { + const entries = this.hooks.get(event); + if (!entries || entries.length === 0) return false; + return entries.some((entry) => { + if (!entry.object) return true; + const targets = Array.isArray(entry.object) ? entry.object : [entry.object]; + return targets.includes('*') || targets.includes(object); + }); + } + + /** + * [#5038] The per-row after-hook contexts a predicate (`multi: true`) write + * dispatches, one per matched row. + * + * ## Why per row at all + * + * ADR-0058's bulk-write addendum (the 2026-08-04 ruling on #4800 / #4862) + * records the contract: **a bulk write is N record changes, so after-hooks + * and the record-change flow triggers riding them evaluate and fire PER + * ROW**, with `previous` = that row's pre-write state and `record` = that + * row's actual state. Before it, the engine fired the hook ONCE, never + * assigned `previous`, and left `record` degraded to the bare payload — so + * the transition condition both the docs and ten showcase flows teach + * (`status == "done" && previous.status != "done"`) was unevaluable on a + * bulk write, and the audit/notification flows behind it silently did not + * happen. + * + * ## The shape is the SINGLE-RECORD shape, deliberately + * + * Each context is exactly what a single-id write builds — `input.id` is the + * row, `input.data` is this write's payload, `result` is the row's state, + * `previous` is its pre-image. That is #2922's ruling for batch INSERT + * restated: a single array-shaped context "broke every consumer built for + * the single shape", so a per-row context must be indistinguishable from a + * record-scoped one. It is also what makes this fix land at the PRODUCER: + * `hook-wrappers`' `record`/`previous` bindings, the record-change trigger's + * `buildContext`, and plugin-audit's diff all read these same four fields and + * need no bulk-aware branch of their own. + * + * `input.data` is a shallow COPY per row: the batch has one payload, but an + * after-handler that writes through the flat-input proxy must not have its + * mutation leak into the next row's view. + * + * ## `result` is composed, not re-read + * + * The row's post-write state is `row ⊕ payload` — the pre-image the matched + * set already gave us, overlaid with the payload the driver just applied. + * Composing keeps the issue's performance guardrail literal: the matched row + * set is read ONCE and reused for every per-row evaluation. Re-reading the + * batch after the write to capture driver-side stamps would be a second + * full-set query for fields the after-view has never carried on this path. + * + * For a DELETE there is no post-state, so `result` is left unset and + * `input` carries no `data`: consumers fall back to `previous` (the deleted + * row), which is what `record` means for a delete. + */ + private buildPerRowAfterContexts( + object: string, + event: 'afterUpdate' | 'afterDelete', + rows: Record[], + batchCtx: HookContext, + payload?: Record, + ): HookContext[] { + const schema = this._registry.getObject(object); + const options = (batchCtx.input as { options?: unknown } | undefined)?.options; + return rows.map((row) => ({ + ...batchCtx, + event, + input: payload + ? { id: (row as { id?: unknown }).id, data: { ...payload }, options } + : { id: (row as { id?: unknown }).id, options }, + previous: coerceBooleanFields(schema as any, row as any), + result: payload + ? coerceBooleanFields(schema as any, { ...row, ...payload } as any) + : undefined, + }) as unknown as HookContext); + } + + /** + * [#5038] Ceiling on the matched-row set a predicate write fires per-row + * after-hooks over. + * + * The consequence ADR-0058's addendum told this implementation to price: an + * after-hook that used to run once per batch now runs once per row, so a + * notification hook sends N messages and a cache-invalidation hook runs N + * times. Unbounded, a single `multi: true` update matching a whole table + * turns into an unbounded fan-out of handler executions inside one write. + * + * Exceeding it REJECTS the write, before `updateMany`/`deleteMany` runs, so + * nothing is written. The alternative — quietly falling back to firing once + * for the batch — is the silent degradation this whole family exists to + * abolish (#4649/#4775): the hooks would not fire for N-1 rows and nothing + * would say so. The rejection names the count, the ceiling and both routes + * out (narrow the predicate, or drop the after-hook). + */ + private assertBulkPerRowHookBudget(object: string, event: string, matched: number): void { + if (matched <= ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS) return; + throw Object.assign( + new Error( + `Refusing the bulk write on '${object}': it matches ${matched} rows, and '${event}' hooks are ` + + `contracted to fire PER ROW on a predicate write (ADR-0058, bulk-write addendum), which is ` + + `over the ${ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS}-row ceiling for one write. Nothing was written. ` + + `Narrow the predicate so the batch matches fewer rows (paginate the write), or remove the ` + + `'${event}' hook from this object. The write is NOT silently downgraded to one hook call for ` + + `the batch — that would skip the hook for ${matched - 1} rows without saying so.`, + ), + { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT', object, event, matched, limit: ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS }, + ); + } + // ======================================== // Action System // ======================================== @@ -3727,6 +3852,12 @@ export class ObjectQL implements IObjectQLEngine { /** Maximum depth for recursive expand to prevent infinite loops */ private static readonly MAX_EXPAND_DEPTH = 3; private static readonly MAX_CASCADE_DEPTH = 10; + /** + * [#5038] Most rows one predicate write may fire per-row after-hooks over. + * Public so a test — and an operator reading a rejection — can name the same + * number the engine enforces. See `assertBulkPerRowHookBudget`. + */ + public static readonly MAX_BULK_PER_ROW_HOOK_ROWS = 10_000; /** In-memory next-value cache per `object.field` for autonumber generation, * lazily seeded from the current max in the store. */ private readonly autonumberCounters = new Map(); @@ -4988,6 +5119,13 @@ export class ObjectQL implements IObjectQLEngine { // reading `previous` must be counted into the new demand test — // pinned by `hook-condition-previous-scope.test.ts`. let priorRecord: Record | null = null; + // [#5038] The matched rows a PREDICATE write fires its per-row + // `afterUpdate` contexts over — set only when this object actually + // has `afterUpdate` hooks, so a bulk write with none pays for no + // read and keeps its single (no-op) batch dispatch. `[]` is + // meaningful and distinct from `null`: zero matched rows is zero + // record changes, hence zero hook calls. + let bulkPerRowRows: Record[] | null = null; const updateSchema = this._registry.getObject(object); const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(updateSchema); const valueShapeStrict = await this.valueShapeStrictFor(updateSchema); @@ -5063,10 +5201,28 @@ export class ObjectQL implements IObjectQLEngine { // it), so a rule-free schema still pays nothing here. const rulesNeedRows = needsPriorRecord(updateSchema as any); const payloadHasReadonlyWhen = hasReadonlyWhenInPayload(updateSchema as any, hookContext.input.data as Record); + // [#5038] The THIRD demand on that same read: after-hooks are + // contracted to fire PER ROW on a predicate write (ADR-0058, + // bulk-write addendum), and a per-row context needs the row's + // pre-image for `previous`. Folded into the existing gate on + // purpose — one `driver.find` serves validation, the + // `readonlyWhen` strip AND the hook dispatch, which is the + // issue's performance guardrail ("行集读取一次完成,求值批内复用") + // stated as code. The demand is uniform across after-hooks: it + // is NOT keyed on whether any condition mentions `previous`, + // which the ruling rejected explicitly as a hidden rule that + // makes a hook's firing count depend on its condition text. + const perRowAfterHooks = this.hasHooksFor('afterUpdate', object); let priorRows: Record[] | null = null; - if (rulesNeedRows || payloadHasReadonlyWhen) { + if (rulesNeedRows || payloadHasReadonlyWhen || perRowAfterHooks) { priorRows = await driver.find(object, ast, hookContext.input.options as any) as Record[]; } + if (perRowAfterHooks) { + // Refuse an unbounded fan-out BEFORE the write, so a batch + // over the ceiling changes nothing at all. + this.assertBulkPerRowHookBudget(object, 'afterUpdate', priorRows?.length ?? 0); + bulkPerRowRows = priorRows ?? []; + } // [#3042] Enforce conditional `readonlyWhen` on the bulk path too. // Unlike static `readonly` (below), a `readonlyWhen` lock is PER // ROW — drop any field locked in ≥1 matched row: a bulk write @@ -5142,7 +5298,30 @@ export class ObjectQL implements IObjectQLEngine { ? result.map((r) => coerceBooleanFields(updateSchema as any, r as any)) : coerceBooleanFields(updateSchema as any, result as any); if (priorRecord) hookContext.previous = coerceBooleanFields(updateSchema as any, priorRecord as any); - await this.triggerHooks('afterUpdate', hookContext); + if (bulkPerRowRows) { + // [#5038] N record changes ⇒ N `afterUpdate` dispatches, each on a + // single-record-shaped context (see `buildPerRowAfterContexts`). + // The batch `hookContext` still carries the affected COUNT as + // `result` and is what this call returns — the write's own contract + // (a predicate update resolves a count, #4639) is unchanged; only + // the hook dispatch became per row. + // + // Rows outer, hooks inner — the same order batch INSERT uses + // (#2922) — so a handler observes one whole record at a time. + // A per-row handler that throws propagates and fails the operation, + // exactly as it does on the single-record and batch-insert paths; + // `onError: 'log'` still swallows it per row. `onError` needed no + // new per-row meaning: it governs a HANDLER on a record-scoped + // context, and that is now what it always gets. + for (const rowCtx of this.buildPerRowAfterContexts( + object, 'afterUpdate', bulkPerRowRows, hookContext, + hookContext.input.data as Record, + )) { + await this.triggerHooks('afterUpdate', rowCtx); + } + } else { + await this.triggerHooks('afterUpdate', hookContext); + } // Roll-up: recompute parent summaries; pass priorRecord too so a child // that moved to a different parent updates BOTH old and new parent. @@ -5361,6 +5540,11 @@ export class ObjectQL implements IObjectQLEngine { // [#4639] See update()'s twin: recorded at the branch that chose the // driver call, not inferred later from a missing id. let isPredicateWrite = false; + // [#5038] Matched rows for the per-row `afterDelete` dispatch — see + // the twin in update(). A bulk delete is N record changes too, so a + // `record-after-delete` flow must see each deleted row rather than + // one context that names none of them. + let bulkPerRowRows: Record[] | null = null; // Capture the row's FK values BEFORE deletion so roll-up summaries can // recompute the (now-orphaned) parent. Only when a summary aggregates // this object — avoids an extra read on every delete. @@ -5387,6 +5571,15 @@ export class ObjectQL implements IObjectQLEngine { `(a hook cleared the target id after the security filter was composed).`, ); } + // [#5038] Read the doomed rows ONCE, before they are gone — + // the only moment their pre-image exists. Gated on this object + // actually having `afterDelete` hooks, so a bulk delete with + // none pays for no read (this path did no read at all before). + if (this.hasHooksFor('afterDelete', object)) { + const doomed = await driver.find(object, ast, hookContext.input.options as any) as Record[]; + this.assertBulkPerRowHookBudget(object, 'afterDelete', doomed?.length ?? 0); + bulkPerRowRows = doomed ?? []; + } result = await driver.deleteMany(object, ast, hookContext.input.options as any); isPredicateWrite = true; } else { @@ -5398,7 +5591,21 @@ export class ObjectQL implements IObjectQLEngine { hookContext.event = 'afterDelete'; hookContext.result = result; - await this.triggerHooks('afterDelete', hookContext); + if (bulkPerRowRows) { + // [#5038] One dispatch per deleted row. No payload is passed, so + // each context carries `previous` = the deleted row and no + // `result`: after a delete there IS no post-state, and every + // consumer (hook `condition`, the record-change trigger, the audit + // diff) already falls back to the pre-image for `record` on a + // delete-shaped context. + for (const rowCtx of this.buildPerRowAfterContexts( + object, 'afterDelete', bulkPerRowRows, hookContext, + )) { + await this.triggerHooks('afterDelete', rowCtx); + } + } else { + await this.triggerHooks('afterDelete', hookContext); + } // Roll-up: recompute the parent summary now that the child is gone. const summaryFailures = summaryPrev diff --git a/packages/objectql/src/hook-condition-bulk-previous.test.ts b/packages/objectql/src/hook-condition-bulk-previous.test.ts index 87de7318fe..c4d5d38e72 100644 --- a/packages/objectql/src/hook-condition-bulk-previous.test.ts +++ b/packages/objectql/src/hook-condition-bulk-previous.test.ts @@ -1,28 +1,39 @@ // 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. + * [#5038, rescoping #5037] Where the bulk-write hook-condition diagnostic still + * applies, and where the per-row contract retired it. * - * 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: + * The 2026-08-04 ruling on #4800 / #4862 settled the contract, recorded as + * ADR-0058's bulk-write addendum: on a predicate (`multi: true`) write, + * after-hooks and the record-change flow triggers riding them **evaluate and + * fire per row**. #5037 shipped an rc-window stopgap for the gap — a rejection + * that named the limitation instead of blaming the author, and promised to + * retire when the contract landed. #5038 landed it. * - * 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. + * The promise is kept ASYMMETRICALLY, and that asymmetry is the point of this + * file. Per-row dispatch is an AFTER-phase idea: a bulk write's after-hooks now + * get one single-record-shaped context per matched row, so a transition + * condition on `afterUpdate`/`afterDelete` evaluates as authored and never + * reaches the diagnostic. Its `before*` hooks still fire ONCE for the whole + * batch — not a version gap, but what the phase IS: a `before*` hook may still + * rewrite the payload, and one `updateMany` carries one payload, so there is + * nothing per-row to hand it. The diagnostic therefore survives, rescoped, for + * exactly that dispatch, and its message no longer promises an expiry it would + * now be breaking. + * + * a. batch (`before*`) dispatch + a condition reading `previous` → the named + * `limitation`, the phase as the reason, and the after-type event as the + * first route out; + * b. the SAME hook on a single-record write → completely unchanged; + * c. a batch dispatch whose condition does NOT read `previous` → unchanged, + * including the plain typo report; + * d. the generic `No such key` / `Unknown variable` riddle is never the WHOLE + * story for (a) — including when the fault names some other key the same + * condition reads, which is what AST-based detection + * (`collectCelRootIdentifiers`) buys over reading cel-js's prose; + * e. RETIRED: the same condition on an `afterUpdate` hook, through the real + * engine, now evaluates per row and the bulk write SUCCEEDS. */ import { describe, it, expect } from 'vitest'; @@ -49,18 +60,21 @@ 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, + name: 'audit_task_completion', object: 'hook_task', events: ['beforeUpdate'], 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 { +/** + * The BATCH dispatch of a predicate (`multi: true`) write: no id, no prior + * record — exactly what the engine's bulk branch builds for `beforeUpdate` / + * `beforeDelete`, one call standing for N matched rows. + */ +function batchCtx(data: Record, event = 'beforeUpdate'): HookContext { return { object: 'hook_task', - event: 'afterUpdate', + event, input: { data, options: { multi: true } }, previous: undefined, ql: qlStub, @@ -79,10 +93,10 @@ function singleCtx(data: Record): HookContext { } /* ──────────────────────────────────────────────────────────────────────────── - * a. The bulk write reading `previous` gets the named limitation + * a. The batch (`before*`) dispatch reading `previous` keeps the named limitation * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5037] a bulk write whose hook condition reads `previous`', () => { +describe('[#5038] the batch dispatch of a bulk write, whose condition reads `previous`', () => { const TRANSITION = 'previous.done != true && record.done == true'; it('rejects with a machine-readable `limitation`, not just prose', async () => { @@ -91,7 +105,7 @@ describe('[#5037] a bulk write whose hook condition reads `previous`', () => { makeHook(TRANSITION), (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); expect(err).toBeInstanceOf(HookConditionError); // The discriminator a caller branches on. Deliberately NOT `code`: ADR-0112 @@ -108,44 +122,85 @@ describe('[#5037] a bulk write whose hook condition reads `previous`', () => { expect(ran).toEqual([]); }); - it('names the batch, the missing binding, and that the VERSION is what is behind', async () => { + it('names the batch, the missing binding, and the PHASE as the reason', async () => { const wrapped = wrapDeclarativeHook( makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ 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("'beforeUpdate'"); + // The reason is now the phase, not a missing release. A `before*` hook may + // still rewrite the shared payload, so it cannot be per-row. + expect(err.message).toContain('before any row is written'); 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 () => { + it('no longer promises an expiry that has already happened', async () => { + // #5037's message said "this rejection retires when #5038 lands". It has + // landed. Repeating that sentence would be a promise the platform is now + // breaking — an author would wait for a release that already shipped. const wrapped = wrapDeclarativeHook( makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); - expect(err.message).toContain('target the write at one record (update by id)'); + expect(err.message).not.toContain('CURRENT-VERSION limitation'); + expect(err.message).not.toMatch(/retires when/); + }); + + it('leads with the after-type event — the route the contract just made real', async () => { + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + + // The first route out is the one per-row dispatch created: the same + // condition on the matching after-type event evaluates as authored. + expect(err.message).toContain("'afterUpdate'"); + expect(err.message).toContain('PER MATCHED ROW'); + // Single-record targeting still works and is still named. + expect(err.message).toContain('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 () => { + it('now DOES name the record-change flow trigger — the fact it was refused over changed', async () => { + // #5037 deliberately refused this route on measured evidence: the trigger + // subscribes to these same lifecycle hooks, so on a bulk write it fired once + // with the same unbound `previous` (#4862). #5038 fixed it at the producer, + // so an after-type record-change trigger rides the per-row dispatch. The + // message follows the fact rather than the other way round. + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, + ); + const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + + expect(err.message).toContain('record-change flow trigger'); + expect(err.message).not.toContain('A record-change flow trigger is NOT a way around this'); + }); + + it('names `beforeDelete` for a delete-shaped batch dispatch', async () => { + const wrapped = wrapDeclarativeHook( + makeHook('previous.done != true', { events: ['beforeDelete'] } as any), + (async () => {}) as any, { logger: silentLogger }, + ); + const ctx = { + object: 'hook_task', event: 'beforeDelete', + input: { 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'); + expect(err.message).toContain("'afterDelete'"); + }); + + it('still ABORTS the write — the rescoping is not an exemption', async () => { const engine = await bootEngine([{ - name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 90, + name: 'audit_task_completion', object: 'hook_task', events: ['beforeUpdate'], priority: 90, condition: TRANSITION, handler: () => {}, } as unknown as Hook]); @@ -159,20 +214,9 @@ describe('[#5037] a bulk write whose hook condition reads `previous`', () => { 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'); + // Fail loud takes no exception: nothing was written. + const rows: any[] = await engine.find('hook_task', {} as any); + expect(rows.every((r) => r.done === false)).toBe(true); }); }); @@ -180,13 +224,14 @@ describe('[#5037] a bulk write whose hook condition reads `previous`', () => { * b. The single-record write of the SAME hook is untouched * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5037] a single-record write is completely unchanged', () => { +describe('[#5038] 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 }, + makeHook(TRANSITION, { events: ['afterUpdate'] } as any), + (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, ); await wrapped(singleCtx({ done: true })); @@ -197,7 +242,8 @@ describe('[#5037] a single-record write is completely unchanged', () => { 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 }, + makeHook(TRANSITION, { events: ['afterUpdate'] } as any), + (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 }); @@ -225,17 +271,17 @@ describe('[#5037] a single-record write is completely unchanged', () => { }); /* ──────────────────────────────────────────────────────────────────────────── - * c. A bulk write whose condition does not read `previous` is untouched + * c. A batch dispatch whose condition does not read `previous` is untouched * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5037] a bulk write with no `previous` in the condition is unaffected', () => { +describe('[#5038] a batch dispatch 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 })); + await wrapped(batchCtx({ done: true })); expect(ran).toEqual(['ran']); }); @@ -246,7 +292,7 @@ describe('[#5037] a bulk write with no `previous` in the condition is unaffected makeHook('record.done == true'), (async () => { ran.push('ran'); }) as any, { logger: silentLogger }, ); - await wrapped(bulkCtx({ done: false })); + await wrapped(batchCtx({ done: false })); expect(ran).toEqual([]); }); @@ -256,7 +302,7 @@ describe('[#5037] a bulk write with no `previous` in the condition is unaffected makeHook('record.stauts == "x"'), (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); expect(err.limitation).toBeUndefined(); expect(err.predicateBulkWrite).toBeUndefined(); @@ -271,27 +317,29 @@ describe('[#5037] a bulk write with no `previous` in the condition is unaffected makeHook('record.archived == true'), (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ 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'); + // Same rescoping: the after-type event is where `record` holds the row's + // real state, so it is named here too. + expect(err.message).toContain("'afterUpdate'"); }); }); /* ──────────────────────────────────────────────────────────────────────────── - * d. The riddle no longer reaches the caller for the `previous` case + * d. The riddle is never the whole story for the `previous` case * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5037] the generic CEL fault is never the whole story on this path', () => { +describe('[#5038] 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); + const err = await wrapped(batchCtx({ 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. @@ -303,7 +351,7 @@ describe('[#5037] the generic CEL fault is never the whole story on this path', }); 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 + // Both halves are unevaluable on a batch dispatch, 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 @@ -313,7 +361,7 @@ describe('[#5037] the generic CEL fault is never the whole story on this path', (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); expect(err.limitation).toBe('bulk_write_previous_unbound'); expect(err.message).toContain("The condition reads 'previous'"); @@ -331,7 +379,7 @@ describe('[#5037] the generic CEL fault is never the whole story on this path', (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ 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"); @@ -347,7 +395,7 @@ describe('[#5037] the generic CEL fault is never the whole story on this path', (async () => {}) as any, { logger: silentLogger }, ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); + const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); expect(err.limitation).toBe('bulk_write_previous_unbound'); if (err.missingKey === 'stauts') { @@ -368,12 +416,65 @@ describe('[#5037] the generic CEL fault is never the whole story on this path', (async () => { ran.push('ran'); }) as any, { logger: silentLogger }, ); - await wrapped(bulkCtx({ done: true })); + await wrapped(batchCtx({ done: true })); expect(ran).toEqual(['ran']); }); }); +/* ──────────────────────────────────────────────────────────────────────────── + * e. RETIRED — the after-type dispatch of the very same write now succeeds + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5038] the diagnostic is RETIRED for after-type hooks', () => { + const TRANSITION = 'previous.done != true && record.done == true'; + + it('the bulk write that #5037 rejected now succeeds, firing the hook per row', async () => { + // The exact scenario `hook-condition-bulk-previous.test.ts` used to pin as a + // rejection, verbatim except for the event. This is the contract landing. + const audited: string[] = []; + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: TRANSITION, + handler: (ctx: any) => { audited.push(String(ctx.previous?.id ?? ctx.input?.id)); }, + } 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 affected = await engine.update( + 'hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any, + ); + + // The write's own contract is untouched — a predicate update still resolves + // the affected COUNT (#4639), not a list of rows. + expect(affected).toBe(2); + // …and the transition hook fired once per matched row. + expect(audited).toHaveLength(2); + expect(new Set(audited).size).toBe(2); + }); + + it('does not fire for rows the transition did not happen on', async () => { + // The whole reason `previous` exists: an already-done row is not a + // transition. Per-row evaluation is what makes that distinction possible on + // a batch — one payload, N different prior states. + const audited: string[] = []; + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition: TRANSITION, + handler: (ctx: any) => { audited.push(String(ctx.previous?.title)); }, + } as unknown as Hook]); + + await engine.insert('hook_task', { title: 'fresh', status: 'todo', done: false }); + await engine.insert('hook_task', { title: 'already', status: 'todo', done: true }); + + await engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any); + + // Both rows matched the predicate; only ONE of them transitioned. + expect(audited).toEqual(['fresh']); + }); +}); + /* ──────────────────────────────────────────────────────────────────────────── * Real-engine harness (mirrors hook-condition-fail-loud.test.ts) * ──────────────────────────────────────────────────────────────────────────── */ @@ -420,6 +521,11 @@ function makeMemoryDriver(): any { for (const r of rows) storeFor(o).set(r.id as string, { ...r, ...data, id: r.id }); return rows.length; }, + async deleteMany(o: string, ast: any) { + const rows = await this.find(o, ast); + for (const r of rows) storeFor(o).delete(r.id as string); + return rows.length; + }, async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {}, }; diff --git a/packages/objectql/src/hook-condition-fail-loud.test.ts b/packages/objectql/src/hook-condition-fail-loud.test.ts index f7f600f58a..648dad7077 100644 --- a/packages/objectql/src/hook-condition-fail-loud.test.ts +++ b/packages/objectql/src/hook-condition-fail-loud.test.ts @@ -314,13 +314,19 @@ describe('[#4775] a condition fault never enters `onError`', () => { * 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', () => { +describe('[#4775 / #4800 B1, rescoped by #5038] the BATCH dispatch of a predicate write gets its own diagnosis', () => { + // [#5038] The batch dispatch is now the `before*` phase only: after-hooks on + // a predicate write are dispatched once per matched row, on a + // single-record-shaped context, so they never land here. A `beforeUpdate` + // still fires ONCE for the whole batch — it may rewrite the shared payload, + // and there is one payload — so this diagnosis is its standing answer. const bulkCtx = (data: Record) => makeCtx({ + event: 'beforeUpdate', 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 () => { + it('`previous` on a batch dispatch 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 }, @@ -333,22 +339,25 @@ describe('[#4775 / #4800 B1] a predicate bulk write gets its own diagnosis', () 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)'); + expect(err.message).toContain('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. + it('DOES point at the after-type event, and at a record-change flow trigger', async () => { + // #5037 refused both on measured evidence: the record-change trigger rides + // these same lifecycle hooks, so on a `multi: true` update it fired ONCE + // with `ctx.previous` undefined (#4862) — naming it would have made that + // message the next `declared ≠ delivered`. #5038 fixed it at the PRODUCER, + // so the per-row dispatch reaches the trigger and the route became real. + // The message follows the fact. 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'); + expect(err.message).toContain("'afterUpdate'"); + expect(err.message).toContain('record-change flow trigger'); + expect(err.message).not.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 () => { @@ -378,8 +387,11 @@ describe('[#4775 / #4800 B1] a predicate bulk write gets its own diagnosis', () }); it('fail loud takes NO exception for the batch — the bulk write still fails', async () => { + // [#5038] On a `beforeUpdate` hook, which is the dispatch that is still + // batch-scoped. The same condition on `afterUpdate` now evaluates per row + // and the write SUCCEEDS — pinned in `hook-condition-bulk-previous.test.ts`. const engine = await bootEngine([{ - name: 'bulk_breaker', object: 'hook_task', events: ['afterUpdate'], priority: 90, + name: 'bulk_breaker', object: 'hook_task', events: ['beforeUpdate'], priority: 90, condition: 'previous.done != true && record.done == true', handler: () => {}, } as unknown as Hook]); diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index cd079fa4e1..2295f1a595 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -193,20 +193,24 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { expect(conditionWarnings()).toEqual([]); }); - it('fabricates nothing on a predicate bulk update — `previous` stays unbound', async () => { + it('fabricates nothing on the BATCH dispatch of a bulk update — `previous` stays unbound', async () => { const calls: string[] = []; 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 + // The `beforeUpdate` of a `multi: true` update fires ONCE for N matched + // rows; there is no single prior record. Binding `{}` or `null` would make // `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`. + // + // [#5038] The AFTER dispatch of that same write is per row and DOES bind + // `previous` — the case below. const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), + makeHook(TRANSITION, ['beforeUpdate']), (async () => { calls.push('ran'); }) as any, { logger }, ); await expect(wrapped(makeCtx({ + event: 'beforeUpdate', previous: undefined, input: { data: { done: true }, options: { multi: true } }, } as any))).rejects.toThrow(/PREDICATE bulk write/); @@ -214,6 +218,28 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { expect(calls).toEqual([]); }); + it('binds `previous` on the PER-ROW dispatch of a bulk update (#5038)', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + // What the engine now hands an after-hook for each matched row of a + // predicate write: the single-record shape, carrying the row's id and its + // own pre-image. The transition condition evaluates exactly as it does on a + // single-record write — which is the contract ADR-0058's addendum records. + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx({ + input: { id: 't1', data: { done: true }, options: { multi: true } }, + previous: { id: 't1', title: 'Ship it', status: 'todo', done: false }, + } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + it('a delete-shaped context evaluates `previous` against the pre-image', async () => { const calls: string[] = []; const { logger, conditionWarnings } = captureLogger(); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index d8b13c3161..017c724809 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -76,24 +76,39 @@ const noopLogger = { * 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). + * Which 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. + * undeclared key is the AUTHOR's to fix and stays fixed, while these two are + * the PLATFORM's. A caller that wants to tell "your hook is wrong" from "this + * event cannot do that" — 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; + * predicate (`multi: true`) write whose hook fires once for the whole + * batch, 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 + * field this write does not set, and `record` is the bare payload on such a * write for the same reason (no single stored row to merge with). * + * ## What #5038 retired, and what it did NOT + * + * #5037 shipped these as a stopgap for the whole bulk-write surface, expiring + * when #5038 landed the per-row contract. #5038 retires them for **after-type** + * hooks, which now receive one single-record-shaped context per matched row — + * `previous` bound, `record` the row's real state — so a transition condition + * on `afterUpdate`/`afterDelete` evaluates on a bulk write exactly as authored + * and never reaches this error. + * + * They stay reachable, and correct, for **before-type** hooks. A + * `beforeUpdate`/`beforeDelete` on a predicate write fires ONCE, before any row + * is touched, because it may still rewrite the payload — and one payload cannot + * be edited per row. That is not a version gap that a later release closes; it + * is what a batch-scoped event is. So the message no longer promises expiry: + * it names the phase as the reason and points at the after-type event, which + * per-row semantics made a route that actually works. + * * ## Why this is not `error.code` * * Deliberately NOT named `code`. ADR-0112 makes `error.code` a CLOSED wire @@ -486,18 +501,26 @@ function isInsertEvent(event: unknown): boolean { } /** - * 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. + * Is this context a BATCH-SCOPED dispatch of a predicate (`multi: true`) write + * — one hook call standing for N matched rows? * * 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. + * `options.multi`), which survive into the event context: `input.options` is + * rebuilt by `buildDriverOptions` as a COPY of the caller's bag, so `multi` is + * still there. + * + * ## Why the `id` test is the whole test (#5038) + * + * Since the per-row contract landed, a bulk write's AFTER-hooks are dispatched + * on one single-record-shaped context per matched row, each carrying that row's + * `input.id`, `previous` and `result`. Those contexts therefore answer `false` + * here — correctly, because they are not batch-scoped at all: nothing about + * them stands for N rows. What still answers `true` is the `beforeUpdate` / + * `beforeDelete` dispatch, which genuinely is one call for the whole batch (it + * may rewrite the shared payload, and there is only one payload to rewrite). + * + * So this predicate did not need a phase test bolted on: the id it looks for is + * exactly what per-row dispatch supplies and batch dispatch cannot. */ function isPredicateBulkWrite(ctx: HookContext): boolean { const input: any = ctx.input ?? {}; @@ -507,6 +530,21 @@ function isPredicateBulkWrite(ctx: HookContext): boolean { return Boolean(options && typeof options === 'object' && options.multi); } +/** Hook events that fire BEFORE the write, once for the whole operation. */ +function isBeforeEvent(event: unknown): boolean { + return typeof event === 'string' && event.startsWith('before'); +} + +/** + * The after-type event a batch-scoped `before*` condition should move to, so + * the diagnostic can name it instead of describing one. + */ +function afterCounterpartEvent(event: unknown): string { + return typeof event === 'string' && event.startsWith('before') + ? `after${event.slice('before'.length)}` + : 'after* '; +} + /** * 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. @@ -546,10 +584,19 @@ function conditionReadsPrevious(source: string): boolean { * 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) + * ## The predicate-bulk-write branch (#4800 / B1, rescoped by #5038) * - * 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: + * A `multi: true` write matches N rows. Its AFTER hooks now fire once PER ROW + * on a single-record-shaped context (ADR-0058's bulk-write addendum, + * implemented in `engine.ts`), so they never reach this branch: `previous` is + * that row's pre-image and `record` is that row's state, and the transition + * condition evaluates as authored. + * + * Its BEFORE hooks still fire ONCE for the whole batch — that is not a gap + * waiting on a release, it is what the phase means: a `before*` hook may + * rewrite the payload, and one `updateMany` carries one payload, so there is + * nothing per-row to give it. For that dispatch two things the condition may + * legitimately name are still not in hand: * * - `previous` — unbound, because there is no single prior record; * - a DECLARED field the payload does not set — `record` is the payload @@ -570,28 +617,31 @@ function conditionReadsPrevious(source: string): boolean { * 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 rejection names a PHASE limit, not a version gap (#5038, 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 (#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. + * hooks and record-change flow triggers evaluate and fire PER ROW. #5037 shipped + * this rejection as the rc-window stopgap for the gap between that contract and + * the engine, and said so — "this retires when #5038 lands". + * + * #5038 has landed, and the honest message changed with it. The rejection is no + * longer a placeholder for missing work; it is the standing answer for the one + * dispatch that is batch-scoped by nature, the `before*` phase. So the message + * no longer promises expiry (a promise it would now be breaking), and its FIRST + * route is the one the contract just made real: move the condition to the + * matching `after*` event, where it evaluates per row exactly as authored. + * + * ⚠️ One escape route named here was added on evidence and one was refused on + * evidence. A record-change flow trigger IS now a real route — it subscribes to + * these very lifecycle hooks (`trigger-record-change/src/record-change-trigger.ts` + * → `engine.registerHook`), which is precisely why the per-row contract reaches + * it: an `after*` record-change trigger receives the row's bound `previous` on a + * bulk write (#4862, closed by #5038). #5037 refused to name it because at that + * time it did not; the fact changed, so the message did. What is still NOT + * offered is "drop `previous` from the condition" as a fix — it unblocks the + * batch by silently turning a transition ("just became done") into a state test + * ("is done"), which fires on rows that were already done. It is named only with + * that cost attached. */ function unevaluableConditionError( meta: Hook, @@ -620,31 +670,46 @@ function unevaluableConditionError( // for a caller that did not pass the AST answer through. const namesPrevious = readsPrevious || unknownVariable === 'previous'; + // Which dispatch is this? A `before*` hook on a predicate write is the one + // that is batch-scoped by nature (#5038); anything else reaching here is a + // context that named no row despite the per-row contract, so it gets the + // same facts without the phase explanation. + const beforePhase = isBeforeEvent(ctx.event); + const afterEvent = afterCounterpartEvent(ctx.event); + const perRowSentence = beforePhase + ? ` A '${afterEvent}' hook on this same write does NOT have this problem: after-hooks on a` + + ` predicate write fire once PER MATCHED ROW, each with that row's 'previous' bound and` + + ` 'record' holding its real state (ADR-0058, bulk-write addendum; ruling on #4800/#4862).` + + ` If the condition is a TRANSITION rather than a guard on the incoming payload, move the hook` + + ` to '${afterEvent}' — or express it as an after-type record-change flow trigger, which rides` + + ` the same per-row dispatch.` + : ` After-hooks on a predicate write are dispatched once per matched row, each carrying that` + + ` row's id, 'previous' and state (ADR-0058, bulk-write addendum); this context carries no row id,` + + ` so it was dispatched for the batch.`; + 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.` + - ` 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 (#4862).`; + ` The condition reads 'previous', but this is the ${beforePhase ? `'${ctx.event}'` : 'batch'} dispatch of a` + + ` PREDICATE bulk write (multi: true): it matches many rows and fires ONCE for the whole batch` + + ` — before any row is written, so it may still rewrite the shared payload — and one call has no` + + ` single prior record to bind.` + + perRowSentence + + ` Targeting the write at one record (update by id) also binds 'previous', so this very condition` + + ` evaluates as authored. Dropping 'previous' from the condition unblocks the batch too, 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.`; } 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):` + + ` '${declaredMissingKey}' IS declared on this object, but this is the` + + ` ${beforePhase ? `'${ctx.event}'` : 'batch'} dispatch of 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.`; + ` payload.` + + perRowSentence + + ` Otherwise reference only fields this write sets, or target the write at one record (update by id).`; } if (bulkDetail !== undefined) { return new HookConditionError(`${head}${typoDetail}${bulkDetail}`, { @@ -757,8 +822,13 @@ function declaredFieldsFor(ctx: HookContext): Record | undefine * * 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. + * + * Since #5038 a predicate bulk update's AFTER dispatch is per row and DOES + * carry the row's prior state, so it merges and materialises like any + * single-record write — which is exactly what "`record` is the row's real state, + * not the bare payload" means (#4862). Its `before*` dispatch still fires once + * for the batch with no prior row, so that 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. @@ -824,9 +894,12 @@ function pickRecordPayload(ctx: HookContext): any { * identifier from the CEL scope. Same here: * - **insert** — there is no prior state, so `previous` is unbound and any * reference to it is an author error, reported as such; - * - **predicate (`multi: true`) bulk update** — the engine matched N rows - * and fires the hook ONCE, so there is no single prior record to bind; - * `previous` stays unbound rather than being invented. + * - **the `before*` dispatch of a predicate (`multi: true`) bulk write** — + * it fires ONCE for N matched rows, so there is no single prior record to + * bind; `previous` stays unbound rather than being invented. The `after*` + * dispatch of that same write is per row since #5038 and binds the row's + * own pre-image, so a transition condition there reads exactly as it does + * on a single-record write. * Binding `null`/`{}` instead would make `previous.x == null` answer "yes" * for a record whose prior state is simply unknown — a fabricated fact, the * one thing materialisation is careful never to do. diff --git a/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts b/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts new file mode 100644 index 0000000000..8a4f6891cc --- /dev/null +++ b/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts @@ -0,0 +1,273 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4862, closed by #5038] A record-change flow trigger on a PREDICATE + * (`multi: true`) write is evaluated and fired PER ROW, with `previous` bound + * to that row's pre-write state and `record` holding that row's real state. + * + * ## What this used to do, measured + * + * #4862 recorded five facts about a `multi: true` update, all of them + * observable here before the fix: + * + * 1. the engine assigned `previous` only on the single-id branch, so on a + * bulk write `ctx.previous` was `undefined`; + * 2. plugin-audit's `__previous` fallback bailed out on bulk (`if (!id) + * return`), so the trigger's second source was empty too; + * 3. the trigger reads exactly those two, so `previous` was always absent; + * 4. `driver.updateMany` resolves an affected COUNT, so `ctx.result` was not + * an object and `record` degraded to the write's bare payload — not any + * row's state; + * 5. the hook fired ONCE, so however many rows matched, the flow was + * evaluated a single time. + * + * The consequence was not theoretical: the transition condition the docs and + * `skills/objectstack-formula` §5 teach — and that ten showcase flows use + * verbatim — is `status == "done" && previous.status != "done"`. On a bulk + * "mark these done" it either did not fire or fired once for a record that did + * not exist, and **nothing anywhere said so**. A missing audit row is the one + * failure nobody goes looking for. + * + * ## Why the fix has no code in this package + * + * All five facts were PRODUCER facts. The trigger already reads `ctx.previous`, + * `ctx.result` and `ctx.input.data`; it was handed a context that had nothing + * in them. #5038 made the engine dispatch one single-record-shaped context per + * matched row, so this consumer became correct without a bulk-aware branch of + * its own — which is the point of fixing it at the producer rather than teaching + * every consumer to cope. These tests exist because that correctness is now a + * CONTRACT across a package boundary, and nothing else pins it end to end. + */ +import { describe, it, expect } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { RecordChangeTriggerPlugin } from './plugin.js'; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** Memory driver with real `updateMany` / `deleteMany` (affected-count contract). */ +function makeDriver(): 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 n = 0; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const exp = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + const sel = (o: string, ast: any) => [...storeFor(o).values()].filter((r) => matches(r, ast?.where)); + return { + name: 'memory', version: '0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async create(o: string, data: any) { + n += 1; const id = data.id ?? `r_${n}`; + const full = { ...data, id }; storeFor(o).set(id, full); return { ...full }; + }, + async update(o: string, id: string, data: any) { + const cur = storeFor(o).get(id) ?? {}; const u = { ...cur, ...data, id }; + storeFor(o).set(id, u); return { ...u }; + }, + async find(o: string, ast: any) { return sel(o, ast).map((r) => ({ ...r })); }, + async findOne(o: string, ast: any) { const [r] = sel(o, ast); return r ? { ...r } : null; }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return sel(o, ast).length; }, + async upsert(o: string, d: any) { return this.create(o, d); }, + async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + // The contract that made `record` a bare payload: a COUNT, naming no row. + async updateMany(o: string, ast: any, data: any) { + const rows = sel(o, ast); + for (const r of rows) storeFor(o).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany(o: string, ast: any) { + const rows = sel(o, ast); + for (const r of rows) storeFor(o).delete(r.id as string); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; +} + +async function bootStack() { + const kernel = new ObjectKernel({ logLevel: 'silent' }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql') as any; + const data = kernel.getService('data') as any; + const automation = kernel.getService('automation'); + objectql.registerDriver(makeDriver(), true); + objectql.registry.registerObject({ + name: 'task', label: 'Task', + fields: { + title: { name: 'title', label: 'Title', type: 'text' }, + status: { name: 'status', label: 'Status', type: 'text' }, + owner: { name: 'owner', label: 'Owner', type: 'text' }, + }, + }, 'test', 'test'); + // Where the flow leaves its trace: one row per firing, carrying the bindings + // it saw. Writing to a DIFFERENT object keeps the assertion clean of the + // trigger's own re-entrancy guard. + objectql.registry.registerObject({ + name: 'task_audit', label: 'Task audit', + fields: { + task_title: { name: 'task_title', label: 'Task', type: 'text' }, + from_status: { name: 'from_status', label: 'From', type: 'text' }, + to_status: { name: 'to_status', label: 'To', type: 'text' }, + task_owner: { name: 'task_owner', label: 'Owner', type: 'text' }, + }, + }, 'test', 'test'); + + return { kernel, data, automation }; +} + +/** The transition flow shape the docs, the formula skill and the ten showcase + * flows all use verbatim. */ +function registerTransitionFlow(automation: AutomationEngine, event = 'record-after-update') { + automation.registerFlow('audit_task_completion', { + name: 'audit_task_completion', label: 'Audit completion', type: 'autolaunched', + nodes: [ + { + id: 'start', type: 'start', label: 'Start', + config: { + objectName: 'task', triggerType: event, + condition: 'status == "done" && previous.status != "done"', + }, + }, + { + id: 'log', type: 'create_record', label: 'Log', + config: { + objectName: 'task_audit', + fields: { + task_title: '{record.title}', + from_status: '{previous.status}', + to_status: '{record.status}', + task_owner: '{record.owner}', + }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'log' }, { id: 'e2', source: 'log', target: 'end' }], + } as any); +} + +describe('[#4862/#5038] a record-change trigger on a predicate bulk write', () => { + it('fires PER ROW, with `previous` bound to each row\'s own pre-write state', async () => { + const { data, automation } = await bootStack(); + registerTransitionFlow(automation); + + await data.insert('task', [ + { title: 'alpha', status: 'todo', owner: 'ann' }, + { title: 'beta', status: 'blocked', owner: 'bob' }, + ], { context: { userId: 'u1' } }); + + // One predicate write over both rows — exactly "mark this batch done". + await data.update('task', { status: 'done' }, { multi: true, where: {}, context: { userId: 'u1' } }); + await sleep(400); + + const audit: any[] = await data.find('task_audit', {}); + // Fact 5 reversed: two matched rows, two flow runs. + expect(audit).toHaveLength(2); + + const byTitle = new Map(audit.map((r) => [r.task_title, r])); + // Facts 1-3 reversed: `previous` is bound, and it is THIS row's pre-image — + // the two rows started in different states and each run saw its own. + expect(byTitle.get('alpha')?.from_status).toBe('todo'); + expect(byTitle.get('beta')?.from_status).toBe('blocked'); + // Fact 4 reversed: `record` is the row's real state. `owner` is a field this + // write never carried, so a bare payload could not have supplied it. + expect(byTitle.get('alpha')?.task_owner).toBe('ann'); + expect(byTitle.get('beta')?.task_owner).toBe('bob'); + expect(byTitle.get('alpha')?.to_status).toBe('done'); + }, 20000); + + it('the start condition discriminates per row — an already-done row does not fire', async () => { + // This is what `previous` is FOR, and what a bulk write could not express: + // `status == "done"` is true of the already-done row too, so without a + // per-row `previous` the flow either fires for it or for nobody. + const { data, automation } = await bootStack(); + registerTransitionFlow(automation); + + await data.insert('task', [ + { title: 'just_finished', status: 'todo', owner: 'ann' }, + { title: 'was_already_done', status: 'done', owner: 'bob' }, + ], { context: { userId: 'u1' } }); + + await data.update('task', { status: 'done' }, { multi: true, where: {}, context: { userId: 'u1' } }); + await sleep(400); + + const audit: any[] = await data.find('task_audit', {}); + expect(audit.map((r) => r.task_title)).toEqual(['just_finished']); + }, 20000); + + it('matches what the SAME flow does on a single-record write', async () => { + // The contract's real claim: an author writes one transition condition and + // it means the same thing whether the write carries an id or a predicate. + // Same flow, same rows, same starting states — only the write shape differs. + const bulk = await bootStack(); + registerTransitionFlow(bulk.automation); + await bulk.data.insert('task', [{ title: 'x', status: 'todo', owner: 'ann' }], { context: { userId: 'u1' } }); + await bulk.data.update('task', { status: 'done' }, { multi: true, where: {}, context: { userId: 'u1' } }); + await sleep(400); + const viaBulk: any[] = await bulk.data.find('task_audit', {}); + + const single = await bootStack(); + registerTransitionFlow(single.automation); + const [row]: any = await single.data.insert('task', [{ title: 'x', status: 'todo', owner: 'ann' }], { context: { userId: 'u1' } }); + await single.data.update('task', { status: 'done' }, { where: { id: row.id }, context: { userId: 'u1' } }); + await sleep(400); + const viaSingle: any[] = await single.data.find('task_audit', {}); + + const strip = (rows: any[]) => rows + .map(({ task_title, from_status, to_status, task_owner }) => ({ task_title, from_status, to_status, task_owner })); + expect(strip(viaBulk)).toEqual(strip(viaSingle)); + }, 30000); + + it('fires per deleted row for a predicate bulk DELETE', async () => { + const { data, automation } = await bootStack(); + automation.registerFlow('audit_task_removal', { + name: 'audit_task_removal', label: 'Audit removal', type: 'autolaunched', + nodes: [ + { + id: 'start', type: 'start', label: 'Start', + config: { objectName: 'task', triggerType: 'record-after-delete', condition: 'status == "stale"' }, + }, + { + id: 'log', type: 'create_record', label: 'Log', + config: { objectName: 'task_audit', fields: { task_title: '{record.title}', to_status: 'deleted' } }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'log' }, { id: 'e2', source: 'log', target: 'end' }], + } as any); + + await data.insert('task', [ + { title: 'old_a', status: 'stale', owner: 'ann' }, + { title: 'old_b', status: 'stale', owner: 'bob' }, + { title: 'keep', status: 'live', owner: 'cat' }, + ], { context: { userId: 'u1' } }); + + await data.delete('task', { multi: true, where: { status: 'stale' }, context: { userId: 'u1' } }); + await sleep(400); + + const audit: any[] = await data.find('task_audit', {}); + // A bulk delete used to fire once with a context naming no row, so a + // deletion audit could not record WHAT was deleted. + expect(audit.map((r) => r.task_title).sort()).toEqual(['old_a', 'old_b']); + }, 20000); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 54117488c0..d54d53a21c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -197,6 +197,9 @@ importers: '@objectstack/cli': specifier: workspace:* version: link:../../packages/cli + '@objectstack/formula': + specifier: workspace:* + version: link:../../packages/formula '@objectstack/objectql': specifier: workspace:* version: link:../../packages/objectql diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index 64ad269944..5f5dee3eae 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -9,7 +9,7 @@ { "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." + "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. Since #5038 that rejection is scoped to the BATCH dispatch — the `before*` phase, which fires once for N rows because it may still rewrite the shared payload; after-hooks are dispatched per row with `previous` bound (ADR-0058's bulk-write addendum), so they never reach it. The message must therefore name the phase as the reason and point at the matching `after*` event, and must NOT promise an expiry that already happened. 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", diff --git a/skills/objectstack-formula/SKILL.md b/skills/objectstack-formula/SKILL.md index c28085aeb8..7a23715a99 100644 --- a/skills/objectstack-formula/SKILL.md +++ b/skills/objectstack-formula/SKILL.md @@ -308,7 +308,9 @@ roots (#4784) — one scope, one meaning, whichever surface reads it. |:---|:---| | Update hook `condition` (single-record write), validation rule on update | the stored pre-write row | | Insert events (`beforeInsert` / `afterInsert`), validation rule on insert | **unbound** — there is no prior state | -| Predicate bulk update (`multi: true`) hook `condition` | **unbound** — one write matches N rows and the hook fires once, so there is no single prior record. `record` is the bare payload here too, so a *declared* field this write does not set is unevaluable as well | +| **`after*` hook `condition` / record-change flow trigger on a predicate (`multi: true`) write** | **that row's pre-write row** — a bulk write fires after-hooks once PER MATCHED ROW (#5038) | +| Validation rule on a predicate bulk update | that row's pre-write row — per row since #3106 | +| `before*` hook `condition` on a predicate (`multi: true`) write | **unbound** — a `before*` hook fires ONCE for the whole batch (it may still rewrite the shared payload), so there is no single prior record. `record` is the bare payload here too, so a *declared* field this write does not set is unevaluable as well | ⚠️ **An unevaluable condition ABORTS the operation (#4775).** Referencing `previous` where it is unbound — like a typo'd key (`record.stauts`), a retired @@ -325,19 +327,41 @@ the condition says" are now different outcomes and the second one is loud — any handler runs). A condition that does not even **compile** aborts the same way. -So write insert-event conditions over `record` alone, and keep transition -conditions to single-record writes — that mistake used to cost you a hook that -quietly never ran, and now costs you every write the hook is attached to. +So write insert-event conditions over `record` alone — that mistake used to cost +you a hook that quietly never ran, and now costs you every write the hook is +attached to. -**On a `multi: true` bulk update the cost lands on every batch (#4800/B1).** -One hook condition reading `previous.*` makes *every* predicate bulk update of -that object fail, and the failure names a hook that has nothing to do with the -write. Fail-loud takes no exception here, but the error is a diagnosis rather -than a raw `No such key: previous`: it says this is a predicate bulk write, that -the N matched rows have no single prior record to bind, and gives the two ways -out — 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 it: -it binds the same lifecycle hook and receives the same unbound `previous`. +**A transition condition needs no special handling for bulk writes (#5038).** +Write it once, on an `after*` event, and it means the same thing whether the +write carries an id or a predicate: + +```ts +// Fires once per row that ACTUALLY transitioned — on `update(id)` and on +// `update({multi: true})` alike. +P`previous.status != 'done' && record.status == 'done'` +``` + +A predicate (`multi: true`) write is N record changes, so the platform evaluates +and fires every record-scoped declaration on it **per row**: `previous` is that +row's own pre-write state and `record` is that row's real state, not the bare +payload (ADR-0058, bulk-write addendum). The matched rows are read once for the +whole batch, so this costs one extra query, not one per row. Record-change flow +triggers ride the same dispatch, so an `record-after-update` flow's start +condition behaves identically. + +⚠️ **The exception is a `before*` hook, and it is not a bug to be fixed later.** +`beforeUpdate` / `beforeDelete` fire ONCE for the whole batch — they may still +rewrite the payload, and one `updateMany` carries one payload — so `previous` is +unbound there and a condition reading it fails the write. The error is a +diagnosis rather than a raw `No such key: previous`: it names the batch, says the +`before*` phase is why, and points at the matching `after*` event, where the +same condition evaluates per row exactly as authored. Put transition conditions +on `after*`; keep `before*` conditions to the incoming payload +(`record.`). + +Above ~10 000 matched rows the platform refuses a predicate write on an object +with after-hooks rather than fan out that many handler runs inside one write — +paginate the write. It is a refusal, never a silent downgrade to one hook call. **`previous` is total over the object's declared fields.** A declared column the driver never returned reads as `null`, not as a fault. Guard with `!= null`, @@ -375,9 +399,10 @@ When migrating Salesforce-flavor metadata, apply these rules in order: | `MONTH_DIFF`, `MID`, `LEFT`, `RIGHT`, `SUBSTITUTE` | _not in stdlib — propose addition_ | > ⚠️ `OLD.x` and `ISCHANGED(x)` both land on `previous.x`, which exists only -> where `previous` is **bound** — see §5. On an insert event, or on a -> `multi: true` predicate bulk update, it is not; since #4775 that does not -> quietly skip the hook, it **fails the write**. +> where `previous` is **bound** — see §5. On an insert event, or in a `before*` +> hook condition on a `multi: true` predicate write, it is not; since #4775 that +> does not quietly skip the hook, it **fails the write**. On `after*` events it +> IS bound, per matched row, on bulk and single-record writes alike (#5038). --- From 160043d0b2df4f87cc7f992b566ee8836483e03f Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 4 Aug 2026 13:50:45 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(trigger-record-change):=20=E7=94=A8?= =?UTF-8?q?=E6=A7=BD=E4=BD=8D=E5=A5=91=E7=BA=A6=E7=B1=BB=E5=9E=8B=E6=9F=A5?= =?UTF-8?q?=E6=9C=8D=E5=8A=A1,=E4=B8=8D=E5=86=8D=E6=8A=8A=E6=9F=A5?= =?UTF-8?q?=E8=AF=A2=E7=BB=93=E6=9E=9C=E6=8A=B9=E6=88=90=20any=20(#5038)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 新增的 `bulk-write-per-row-context.test.ts` 照抄了同目录既有测试的 `kernel.getService('objectql') as any` 写法,而那两个文件是记在 `scripts/slot-lookup-baseline.json` 里的历史遗留项(按文件豁免、只减不增)。 新文件不在基线内,`no-restricted-syntax`(#4127/#4251)因此在 CI 上报了两处错误。 改为 `getService('objectql')` / `getService('data')`。 `registerObject` 不在 `EngineSchemaRegistryView`(objectql 槽位公布的 registry 契约,只读 + 包生命周期)上,它是引擎注册表的测试期接缝 —— 因此单独用一个结构化 类型 `TestObjectRegistry` 收窄,而不是把整个服务查询抹成 `any`:槽位查询保持完整 契约检查,只有这一个成员被断言。 --- .../src/bulk-write-per-row-context.test.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts b/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts index 8a4f6891cc..fd15a307a3 100644 --- a/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts +++ b/packages/triggers/trigger-record-change/src/bulk-write-per-row-context.test.ts @@ -42,8 +42,20 @@ import { describe, it, expect } from 'vitest'; import { ObjectKernel } from '@objectstack/core'; import { ObjectQLPlugin } from '@objectstack/objectql'; import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import type { IDataEngine, IObjectQLEngine } from '@objectstack/spec/contracts'; import { RecordChangeTriggerPlugin } from './plugin.js'; +/** + * `registerObject` is the engine registry's TEST-time seam and is not part of + * `EngineSchemaRegistryView` (the `objectql` slot's published registry + * contract, which is read-only plus package lifecycle). Narrowed structurally + * rather than through `any`, so the slot lookup itself stays fully contracted + * (#4127/#4251) and only this one member is asserted. + */ +type TestObjectRegistry = { + registerObject(schema: unknown, packageId?: string, namespace?: string): void; +}; + const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); /** Memory driver with real `updateMany` / `deleteMany` (affected-count contract). */ @@ -107,11 +119,12 @@ async function bootStack() { await kernel.use(new RecordChangeTriggerPlugin()); await kernel.bootstrap(); - const objectql = kernel.getService('objectql') as any; - const data = kernel.getService('data') as any; + const objectql = kernel.getService('objectql'); + const data = kernel.getService('data'); const automation = kernel.getService('automation'); + const registry = objectql.registry as unknown as TestObjectRegistry; objectql.registerDriver(makeDriver(), true); - objectql.registry.registerObject({ + registry.registerObject({ name: 'task', label: 'Task', fields: { title: { name: 'title', label: 'Title', type: 'text' }, @@ -122,7 +135,7 @@ async function bootStack() { // Where the flow leaves its trace: one row per firing, carrying the bindings // it saw. Writing to a DIFFERENT object keeps the assertion clean of the // trigger's own re-entrancy guard. - objectql.registry.registerObject({ + registry.registerObject({ name: 'task_audit', label: 'Task audit', fields: { task_title: { name: 'task_title', label: 'Task', type: 'text' },