diff --git a/.changeset/update-prior-read-per-object.md b/.changeset/update-prior-read-per-object.md new file mode 100644 index 0000000000..001c69ce54 --- /dev/null +++ b/.changeset/update-prior-read-per-object.md @@ -0,0 +1,42 @@ +--- +"@objectstack/objectql": patch +--- + +perf(objectql): `update()` 的单 id 前置行读取改为按对象判定需求(#5284) + +单记录 `update()` 在写入前会读一次「前置行」(`driver.findOne`),用于对象校验规则、 +`readonlyWhen` 剥离、`hookContext.previous`,以及 roll-up 汇总的旧父记录重算。这道门 +以前问的是 `this.hooks.get('afterUpdate').length > 0` —— **全部对象汇总在一起**的注册 +列表。于是只要有**任意一个**对象被观察,平台上**每一个**对象的**每一次**单 id update 都 +要多付一次数据库往返。同文件的批量路径早就按对象问同一个问题(`hasHooksFor`,#5038), +一个文件里存在两种精度。 + +现在按对象问,需求项恰好三条,全部是 `priorRecord` 在这条分支上的真实消费方: + +- `needsPriorRecord(schema)` —— 对象校验规则(ADR-0020 的 state_machine / cross_field / + script;PATCH 只带变更字段)以及它涵盖的 `readonlyWhen` / `requiredWhen` / 选项可见性; +- **本对象**存在 `afterUpdate` hook(`'*'` / 全局注册照样算,`hasHooksFor` 与 + `triggerHooks` 的过滤逻辑一致)—— 它的 handler 与声明式 `condition` 都读 `previous`; +- 本对象被某个 roll-up `summary` 聚合 —— `previous` 携带**旧的**父记录 id,子记录改挂父 + 记录时要同时重算两个父。 + +**省下多少取决于 hook 怎么注册,不取决于有多少个。** 按对象注册的那些(record-change flow +trigger、plugin-sharing 的按规则重算、plugin-auth 的 `sys_user` 快照刷新、所有元数据编写的 +hook)从此不再向邻居收税;而**完全不带 `object`** 的注册(plugin-audit 的 `writeAudit`、 +service-storage 的文件引用回收)本来就会在每个对象上真的执行,所以在装了它们的部署里这道门 +依然恒真、省不下读 —— 让这两处在注册面上表达它们 handler 里已经在做的过滤,是 #5846,不在 +本次改动内。 + +**正确性保证不变。** `previous` 的语义、fail-loud 的形态(#4775:求不出值即拒绝,绝不 +伪造 `{}`/`null`)、after-hook 的分发都与之前完全一致;有 `afterUpdate` 的对象付的读一次 +不少。变的只是**没有任何消费方**的对象不再替别人付账。 + +顺带修掉一个此前只是**偶然**成立的行为:子记录改挂父记录时的「旧父重算」依赖的正是这次 +读,而在一个没有任何 `afterUpdate` hook 的部署里它本来就不会发生(旧父的汇总值静默过期)。 +roll-up 现在自己声明这项需求,不再靠别的对象的 hook 捎带。 + +`beforeUpdate` **不**计入这道门,这是它与 `delete()`(#5272)唯一不对称的地方,原因是两条 +路径的读取时机不同:`delete()` 在派发 `beforeDelete` **之前**读前置行并绑定,before 阶段 +因此是真实读者;`update()` 先派发 `beforeUpdate`(它还可能改写这次读要比对的 payload), +`hookContext.previous` 要到写入之后才绑定 —— 所以在这条路径上 `beforeUpdate` 无论门怎么 +判都看不到 `previous`,计入它只会买一次没有读者的读。这一点由测试直接测量钉住。 diff --git a/packages/objectql/src/engine-update-prior-read-scope.test.ts b/packages/objectql/src/engine-update-prior-read-scope.test.ts new file mode 100644 index 0000000000..0de1b15fac --- /dev/null +++ b/packages/objectql/src/engine-update-prior-read-scope.test.ts @@ -0,0 +1,382 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5284] `update()`'s single-id prior-row read is demanded PER OBJECT. + * + * The gate used to ask `this.hooks.get('afterUpdate').length > 0` — the pooled + * registration list of EVERY object — so one plugin registering `afterUpdate` + * on one object (plugin-audit does exactly that) made every single-id update on + * every OTHER object pay an extra `driver.findOne`. The bulk paths in the same + * file already asked it per object (`hasHooksFor`, #5038). + * + * These cases pin the three demands that survive the narrowing, and the one + * that is deliberately absent: + * + * 1. `needsPriorRecord(schema)` — validation rules + the `readonlyWhen` / + * `requiredWhen` / option-visibility field predicates it subsumes; + * 2. an `afterUpdate` hook on THIS object (handler and declarative + * `condition` both read `previous`), `'*'`/global registrations included; + * 3. a roll-up `summary` aggregating this object — `previous` carries the OLD + * parent id, so a repointed child recomputes both parents. Before #5284 + * that read was only incidental: a deployment with no `afterUpdate` hook + * anywhere left the old parent silently stale. + * + * And the absent one: `beforeUpdate`. Unlike `delete()` (#5272), which reads the + * pre-image BEFORE dispatching `beforeDelete` and binds it there, `update()` + * dispatches `beforeUpdate` first and binds `hookContext.previous` only after + * the write — so a `beforeUpdate` hook observes `previous === undefined` + * whatever this gate decides. Two cases below measure that directly, so the + * "count before-hooks too" reflex is answered with evidence rather than + * symmetry. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import type { Hook } from '@objectstack/spec/data'; + +const TASK_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, +}; + +/** Two plain objects, neither declaring anything that needs a prior row. */ +const taskA = { name: 'scope_task_a', label: 'A', fields: TASK_FIELDS }; +const taskB = { name: 'scope_task_b', label: 'B', fields: TASK_FIELDS }; + +/** Declares a `readonlyWhen` field, so `needsPriorRecord` is true for it. */ +const lockedTask = { + name: 'scope_locked_task', + label: 'Locked', + fields: { + ...TASK_FIELDS, + title: { + name: 'title', label: 'Title', type: 'text' as const, + // Once the task is done its title is frozen — a predicate over the + // record's STORED state, which is exactly what the prior row is for. + readonlyWhen: 'record.done == true', + }, + }, +}; + +/** Parent/child pair for the roll-up demand. */ +const invoice = { + name: 'scope_invoice', + label: 'Invoice', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + line_total: { + name: 'line_total', label: 'Total', type: 'summary' as const, + summaryOperations: { object: 'scope_invoice_line', field: 'amount', function: 'sum' }, + }, + }, +}; +const invoiceLine = { + name: 'scope_invoice_line', + label: 'Line', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + invoice: { name: 'invoice', label: 'Invoice', type: 'master_detail' as const, reference: 'scope_invoice' }, + }, +}; + +/** + * A driver that counts every read, so "pays no extra read" is a measurement + * rather than an assertion about the code that was written. + */ +function makeCountingDriver() { + const stores = new Map>>(); + /** + * `findOneOn` is per object because a write can legitimately read a DIFFERENT + * object: repointing a master_detail FK makes `assertReferencesResolve` check + * the new parent exists (#4441). Counting only the total would conflate that + * with the prior-row read this file is about. + */ + const reads = { findOne: 0, find: 0, findOneOn: {} as Record }; + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((sub) => matchesWhere(row, sub))) return false; + continue; + } + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + reads.find += 1; + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + reads.findOne += 1; + reads.findOneOn[object] = (reads.findOneOn[object] ?? 0) + 1; + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, reads, storeFor }; +} + +async function boot(hooks: Hook[] = [], objects: unknown[] = [taskA, taskB]) { + const engine = new ObjectQL(); + const stub = makeCountingDriver(); + engine.registerDriver(stub.driver, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o as any); + const warn = vi.fn(); + if (hooks.length > 0) { + bindHooksToEngine(engine, hooks, { + packageId: 'app:scope', + logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + }); + } + return { engine, reads: stub.reads, storeFor: stub.storeFor, warn }; +} + +const observer = (name: string, object: string, event: string, sink: Array): Hook => ({ + name, object, events: [event], priority: 90, + handler: (ctx: any) => { sink.push(ctx.previous); }, +} as unknown as Hook); + +describe('[#5284] the prior-row demand is asked per object', () => { + it('object A pays NO prior read while only object B has an afterUpdate hook', async () => { + // The issue itself: before the narrowing this delta was 1 — one plugin's + // registration on ONE object taxed every single-id update on every other. + const { engine, reads } = await boot([ + observer('audits_b', 'scope_task_b', 'afterUpdate', []), + ]); + + const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('scope_task_a', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(reads.findOne - before).toBe(0); + }); + + it('the object that DOES have the hook still pays it, and `previous` is the stored row', async () => { + // The other half of the same measurement: narrowing must not cost the + // objects that are actually observed anything at all. + const seen: Array = []; + const { engine, reads } = await boot([ + observer('audits_b', 'scope_task_b', 'afterUpdate', seen), + ]); + + const row: any = await engine.insert('scope_task_b', { title: 'B', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('scope_task_b', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(reads.findOne - before).toBe(1); + expect(seen).toEqual([{ id: row.id, title: 'B', status: 'todo', done: false }]); + }); + + it('a hook registered for `*` still demands the read on every object', async () => { + // `hasHooksFor` mirrors `triggerHooks`' own filter: an entry that targets + // `'*'` DOES reach this object, so narrowing per object must not narrow it + // away. Getting this looser costs a query; getting it tighter would drop + // hooks that were going to fire. + const seen: Array = []; + const { engine, reads } = await boot([{ + name: 'audits_everything', object: '*', events: ['afterUpdate'], priority: 90, + handler: (ctx: any) => { seen.push(ctx.previous); }, + } as unknown as Hook]); + + const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('scope_task_a', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(reads.findOne - before).toBe(1); + expect(seen).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + }); + + it('`needsPriorRecord` alone still forces the read — no hook registered anywhere', async () => { + const { engine, reads, storeFor } = await boot([], [lockedTask]); + + const row: any = await engine.insert('scope_locked_task', { title: 'Ship it', status: 'done', done: true }); + const before = reads.findOne; + // `readonlyWhen: record.done == true` is TRUE for the STORED row, so the + // incoming title change is dropped — which is only decidable with the prior + // record in hand. The count and the effect are asserted together: a read + // that happened but was not used would pass the first and fail the second. + await engine.update('scope_locked_task', { title: 'Renamed' }, { where: { id: row.id } } as any); + + expect(reads.findOne - before).toBe(1); + expect(storeFor('scope_locked_task').get(row.id)?.title).toBe('Ship it'); + }); + + it('a roll-up summary keeps its OLD-parent recompute with no hooks registered anywhere', async () => { + // The demand the narrowing would otherwise have dropped on the floor: + // `recomputeSummaries` reads `previous[fk]` to find the parent a repointed + // child LEFT. Before #5284 this only worked when some object happened to + // have an afterUpdate hook; now the roll-up asks for itself. + const { engine, reads, storeFor } = await boot([], [invoice, invoiceLine]); + + const p1: any = await engine.insert('scope_invoice', { name: 'INV-1' }); + const p2: any = await engine.insert('scope_invoice', { name: 'INV-2' }); + const line: any = await engine.insert('scope_invoice_line', { invoice: p1.id, amount: 40 }); + expect(storeFor('scope_invoice').get(p1.id)?.line_total).toBe(40); + + const before = reads.findOneOn['scope_invoice_line'] ?? 0; + await engine.update('scope_invoice_line', { invoice: p2.id }, { where: { id: line.id } } as any); + + // Exactly one read OF THE CHILD — the prior row. (The write also reads the + // new PARENT once, for the #4441 dangling-reference check; that is a + // different object and a different demand.) + expect((reads.findOneOn['scope_invoice_line'] ?? 0) - before).toBe(1); + // BOTH parents: the one it joined, and the one it left. + expect(storeFor('scope_invoice').get(p2.id)?.line_total).toBe(40); + expect(storeFor('scope_invoice').get(p1.id)?.line_total).toBe(0); + }); +}); + +/** + * Why these run on a BARE engine, and what that does and does not prove. + * + * `new ObjectQL()` carries no `ObjectQLPlugin` builtins, so the only producer + * of `hookContext.previous` here is `update()` itself. That is exactly the + * subject: whether THIS gate's read can ever reach the before phase. + * + * A kernel-hosted engine has a second producer — `sys_fetch_previous_update` + * (`plugin.ts`, `object: '*'`, priority 5) makes its own `findOne` and assigns + * `previous` before any authored before-hook runs — so a `beforeUpdate` + * condition reading `previous` DOES evaluate there. That producer is untouched + * by this gate, which is why narrowing takes no binding away from anyone; the + * last case below drives its shape to show the two do not interfere. (That it + * is a duplicate read of the same row — a third one comes from plugin-audit's + * `captureBefore` — is filed as #5846, not fixed here.) + */ +describe('[#5284] `beforeUpdate` is not a reader of THIS read', () => { + it('observes nothing from the engine read even when the row IS in hand', async () => { + // Measured, not assumed: this object has an afterUpdate hook, so the prior + // row is fetched — and the beforeUpdate hook still sees nothing, because + // `update()` dispatches it BEFORE the read (it may still rewrite the very + // payload the read would be compared against) and binds + // `hookContext.previous` only after the write. + // + // This is why the gate does NOT count `beforeUpdate`: it would buy a read + // with no reader. `delete()` (#5272) counts `beforeDelete` because there + // the read genuinely precedes the dispatch and binds it. + const beforeSeen: Array = []; + const afterSeen: Array = []; + const { engine } = await boot([ + observer('pre', 'scope_task_a', 'beforeUpdate', beforeSeen), + observer('post', 'scope_task_a', 'afterUpdate', afterSeen), + ]); + + const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + await engine.update('scope_task_a', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(beforeSeen).toEqual([undefined]); + expect(afterSeen).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + }); + + it('rejects a `previous.*` beforeUpdate condition whether or not the prior row was read', async () => { + // Both configurations reject identically, which is the point: on an engine + // with no other producer of `previous`, the verdict is decided by the update + // path's dispatch ORDER, not by this gate. So narrowing the gate creates no + // new rejection. (Under a kernel the builtin binds `previous` first and the + // same condition evaluates — see the block comment above.) + const withRead = await boot([ + { name: 'pre_cond', object: 'scope_task_a', events: ['beforeUpdate'], priority: 90, + condition: 'previous.done != true && record.done == true', handler: () => {} } as unknown as Hook, + observer('post', 'scope_task_a', 'afterUpdate', []), // forces the prior read + ]); + const rowA: any = await withRead.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + await expect( + withRead.engine.update('scope_task_a', { done: true }, { where: { id: rowA.id } } as any), + ).rejects.toThrow(/previous/); + + const withoutRead = await boot([ + { name: 'pre_cond', object: 'scope_task_a', events: ['beforeUpdate'], priority: 90, + condition: 'previous.done != true && record.done == true', handler: () => {} } as unknown as Hook, + ]); + const rowB: any = await withoutRead.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + await expect( + withoutRead.engine.update('scope_task_a', { done: true }, { where: { id: rowB.id } } as any), + ).rejects.toThrow(/previous/); + }); + + it('leaves a `previous` supplied by an earlier before-hook alone — and still reads nothing', async () => { + // The kernel's `sys_fetch_previous_update` shape: a low-priority (= earlier) + // `beforeUpdate` hook that fetches the row itself and assigns + // `ctx.previous`. The narrowed gate must not fight it — neither by clobber- + // ing the binding (`priorRecord` is null, and the post-write assignment is + // guarded on it) nor by re-reading a row someone already has. + const supplied: Array = []; + const { engine, reads } = await boot([ + { name: 'fetch_previous', object: 'scope_task_a', events: ['beforeUpdate'], priority: 5, + handler: async (ctx: any) => { + // Verbatim the builtin's shape, through the engine the context + // carries — `ctx.ql` is what `plugin.ts` reaches for as `this.ql`. + if (ctx.input?.id && !ctx.previous) { + const prev = await ctx.ql.findOne(ctx.object, { + where: { id: ctx.input.id }, context: { isSystem: true }, + }); + if (prev) ctx.previous = prev; + } + } } as unknown as Hook, + { name: 'reads_previous', object: 'scope_task_a', events: ['beforeUpdate'], priority: 90, + condition: 'previous.done != true && record.done == true', + handler: (ctx: any) => { supplied.push(ctx.previous); } } as unknown as Hook, + ]); + + const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('scope_task_a', { done: true }, { where: { id: row.id } } as any); + + // The condition evaluated (the handler ran) against the hook-supplied row… + expect(supplied).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + // …and the engine added no read of its own: exactly the ONE the hook made. + expect(reads.findOne - before).toBe(1); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 548b321687..18c414b806 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2816,23 +2816,30 @@ export class ObjectQL implements IObjectQLEngine { const failures: any[] = []; for (const name of Object.keys(fields)) { - // A `readonly` field is never the caller's to answer for — BY - // CONSTRUCTION, not by exemption. + // [#4441, narrowing kept by #4743] A `readonly` field is never the + // caller's to answer for — BY CONSTRUCTION, not by exemption. // - // `stripReadonlyFields` removes a non-system caller's value from a - // readonly field before the write, and the create ingress does the same - // (`stripReadonlyForInsert`, #3043). So any value still sitting in one at - // this point was written by the PLATFORM, which puts it outside this - // check's own stated scope ("the reference the caller named"). + // This check answers for exactly one thing: "the reference the CALLER + // named". `stripReadonlyFields` removes a non-system caller's value from + // a readonly field before the write, and the create ingress does the same + // (`stripReadonlyForInsert`, #3043). So a value still sitting in one at + // this point was minted by the PLATFORM — outside this check's own stated + // scope, whatever it happens to hold. That argument stands on its own and + // depends on no particular field: deleting the `continue` would start + // rejecting the platform's own writes, which is why it is recorded here + // (AGENTS.md PD #13) rather than left to be re-derived. // - // Found by the dogfood gate rather than by reasoning: `sys_metadata_history. - // recorded_by` is `Field.lookup('sys_user', { readonly: true })` that the - // metadata repository fills with `actor ?? 'system'` — a SENTINEL STRING, - // not a user id, on a write that does not carry `isSystem`. Checking it - // rejected ordinary metadata authoring (package create / publish / clone). - // The sentinel-in-a-lookup is a real modelling wart and is filed - // separately; it is not this change's to fix, and rejecting the - // platform's own write is not the way to report it. + // Historical note, kept because it is how the narrowing was FOUND (the + // dogfood gate, not reasoning) and because the audit next door re-scoped + // itself on its removal: `sys_metadata_history.recorded_by` is a + // `Field.lookup('sys_user', { readonly: true })` that the metadata + // repository once filled with `actor ?? 'system'` — a SENTINEL STRING, + // not a user id, on a write carrying no `isSystem`; checking it rejected + // ordinary metadata authoring (package create / publish / clone). #4556 + // replaced that sentinel with NULL, so no platform write puts a non-id in + // a reference column any more. The narrowing survives it unchanged + // because it never rested on it — but #4551's blanket audit skip did, and + // #4743/#5719 duly re-scoped that one (see `inspectDanglingReferences`). // // This does NOT weaken #4441: the fields the issue names — // `sys_position_permission_set.permission_set_id` and @@ -3000,8 +3007,14 @@ export class ObjectQL implements IObjectQLEngine { * with one predicate, so the report can never be more or less strict than the * rule it reports on. * - * See {@link auditDanglingReferences} for the judgments (readonly skip, empty - * values, unknown ≠ absent) and the bounded-scan honesty of the report. + * See {@link auditDanglingReferences} for the judgments (readonly SPLIT — + * `readonly` references are read like any other and their findings filed + * under `provenance` / `provenanceUndetermined` since #4743/#5719, not + * skipped; empty values; unknown ≠ absent) and for the bounded-scan honesty + * of the report, whose incompleteness now has a bucket at every level: + * `truncatedObjects` inside a table, `unscannedObjects` for the tables the + * budget never reached (#5718), `unreadableObjects` for what the datasource + * refused, and `aborted` for a run called off (#4747). */ async inspectDanglingReferences( options?: DanglingReferenceAuditOptions, @@ -5492,25 +5505,23 @@ export class ObjectQL implements IObjectQLEngine { let isPredicateWrite = false; // Pre-update snapshot. Exposed to after-hooks via `hookContext.previous` // (the HookContext contract documents `previous` for update/delete) and - // reused for object-level validation rules. Fetched once, only for - // single-id updates, when either a rule needs it (ADR-0020: - // state_machine / cross_field / script — a PATCH carries only changed - // fields) OR an afterUpdate hook is registered. The latter is what makes - // record-change flow triggers work: their start-condition gate reads - // `previous.*` (e.g. `status == "done" && previous.status != "done"`), - // which silently fails when `previous` is absent. + // reused for object-level validation rules and the roll-up recompute. + // Fetched once, only for single-id updates, and only when something on + // THIS object actually consumes it — see `wantsPriorRecord` below. + // Binding `previous` is what makes record-change flow triggers work: + // their start-condition gate reads `previous.*` (e.g. `status == "done" + // && previous.status != "done"`), which fails when `previous` is absent. // // [#4784] It is ALSO what supplies the `previous` binding to a // declarative hook `condition` (`hook-wrappers.ts`), which is how a // TRANSITION is expressed there: `previous.done != true && - // record.done == true`. Note this needs NO second demand-driven - // fetch — the existing gate already fetches whenever an afterUpdate - // hook exists, and afterUpdate is the event whose context carries - // `previous`. Deliberately: adding a "does the condition reference - // `previous`?" analysis on top would be dead code today. If this - // gate is ever NARROWED (e.g. scoped per object), hook conditions - // reading `previous` must be counted into the new demand test — - // pinned by `hook-condition-previous-scope.test.ts`. + // record.done == true`. That needs NO second demand-driven fetch: the + // gate fetches whenever this object has an afterUpdate hook, and + // afterUpdate is the event whose context carries `previous`. Adding a + // "does the condition reference `previous`?" analysis on top would + // still be dead code — the demand is uniform across after-hooks, which + // #5038 records as a ruling (a hook's cost must not depend on its + // condition text). 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 @@ -5530,7 +5541,77 @@ export class ObjectQL implements IObjectQLEngine { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation }); - if (needsPriorRecord(updateSchema as any) || (this.hooks.get('afterUpdate')?.length ?? 0) > 0) { + // [#5284] Demand-driven, and the demand is asked PER OBJECT. + // + // This gate used to ask `this.hooks.get('afterUpdate').length > 0` + // — the whole registration list, every object's hooks pooled — so + // ONE object being observed made EVERY single-id update on EVERY + // object pay an extra `driver.findOne`. The bulk paths next door + // already asked the same question per object + // (`hasHooksFor('afterUpdate', object)`, #5038), so one file held + // two precisions of one question; this is the narrower one, which + // `hasHooksFor` answers by mirroring `triggerHooks`' own filter + // (an entry with no `object`, or `'*'`, is still global). + // + // Measured, so the expectation is right: what this saves depends + // on how the deployment's hooks are REGISTERED, not on how many + // there are. Object-scoped registrants stop taxing their + // neighbours — a record-change flow trigger (`object: + // binding.object`), plugin-sharing's per-rule recompute, plugin- + // auth's `sys_user` snapshot refresh, every metadata-authored + // hook. Registrants that pass no `object` at all (plugin-audit's + // `writeAudit`, service-storage's file-reference reconcile) DO + // reach every object, so where they are loaded this gate stays + // true for everything and saves nothing — correctly, since their + // handlers really do run. Making those two express in the + // registration what their handlers already decide at runtime is + // #5846, not this change. + // + // Every consumer of `priorRecord` on this branch is counted, and + // there are exactly three: + // * `needsPriorRecord(updateSchema)` — object validation rules + // (ADR-0020: state_machine / cross_field / script; a PATCH + // carries only changed fields) AND the `readonlyWhen` / + // `requiredWhen` / option-visibility field predicates, which + // it subsumes; + // * an `afterUpdate` hook on THIS object — `hookContext.previous` + // for its handler (plugin-audit, the record-change trigger) + // and for its declarative `condition`; + // * a roll-up `summary` aggregating this object — `previous` + // carries the OLD parent id, so a child that REPOINTS + // recomputes both parents (see `recomputeSummaries` below). + // Without this term the narrowing would have turned an + // incidental read into a silently stale parent summary: today + // a repointed child is only saved by some other object having + // an afterUpdate hook. + // + // `beforeUpdate` is deliberately NOT counted, and that is the one + // place this gate does NOT mirror `delete()`'s (#5272). The two + // paths order the read differently: `delete()` reads the pre-image + // BEFORE dispatching `beforeDelete` and binds it there, so a + // before-phase hook is a real reader of THAT read. `update()` + // dispatches `beforeUpdate` first (it may still rewrite the very + // payload this read would be compared against) and binds + // `hookContext.previous` only after the write — so no + // `beforeUpdate` hook can observe this row however the gate is + // written, and counting the event here would buy a read with no + // reader. + // + // What a kernel-hosted `beforeUpdate` hook DOES see comes from a + // different producer entirely: the `sys_fetch_previous_update` + // builtin (`plugin.ts`, `object: '*'`, priority 5) makes its own + // `findOne` and assigns `hookContext.previous` before any authored + // before-hook runs. That read is untouched by this gate — and it + // is why narrowing here cannot take a binding away from the before + // phase. It is also a duplicate of this one (plugin-audit's + // `captureBefore` makes a third): three reads of one row, filed + // as #5846 with the delete-side time-ordering (#5272) as the fix + // shape — not something to paper over by widening this gate. + const wantsPriorRecord = + needsPriorRecord(updateSchema as any) || + this.hasHooksFor('afterUpdate', object) || + this.getSummaryDescriptors(object).length > 0; + if (wantsPriorRecord) { const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 }; priorRecord = await driver.findOne(object, priorAst, hookContext.input.options as any); } diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index 9820482d8f..062a879504 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -439,10 +439,17 @@ describe('[#4784] transition condition over a real engine', () => { describe('[#4784] a condition that never mentions `previous` costs zero extra fetches', () => { /** - * The demand-driven prior fetch (`engine.ts`, the `needsPriorRecord(...) || - * afterUpdate hooks exist` gate) is the ONE mechanism that decides whether a - * prior row is read; #4784 adds no second one. These two pins are what a - * future narrowing of that gate has to keep true. + * The demand-driven prior fetch (`engine.ts`, the `wantsPriorRecord` gate) is + * the ONE mechanism that decides whether a prior row is read; #4784 adds no + * second one. These two pins are what a narrowing of that gate has to keep + * true — and #5284 has since narrowed it, from "ANY object has an afterUpdate + * hook" to "THIS object does (or its schema needs a prior row, or a roll-up + * aggregates it)". Both pins still hold, and the first one carries more + * weight than it did: the object it drives has a `beforeUpdate` hook, which + * the narrowed gate deliberately does not count (a `beforeUpdate` hook is + * dispatched before the read and observes no `previous` on this path, so + * counting it would buy a read with no reader — see + * `engine-update-prior-read-scope.test.ts`, which measures exactly that). */ async function bootWith(hooks: Hook[]) { const engine = new ObjectQL();