diff --git a/.changeset/trigger-record-change-formula-gate-getobject.md b/.changeset/trigger-record-change-formula-gate-getobject.md new file mode 100644 index 0000000000..016105324c --- /dev/null +++ b/.changeset/trigger-record-change-formula-gate-getobject.md @@ -0,0 +1,27 @@ +--- +"@objectstack/trigger-record-change": patch +--- + +fix(trigger-record-change): the hydration schema gate now actually engages on the real engine (#8482) + +`RecordChangeTrigger`'s computed-field hydration re-read (a `findOne` on every +`afterInsert`/`afterUpdate` dispatch, added to surface `formula` virtual fields +in the seeded flow record) was meant to be skipped for objects that declare no +`formula` field — the only thing the re-read adds. The skip was gated on an +optional `getObjectConfig` accessor that the concrete ObjectQL engine never +implemented, so on every real deployment the gate always fell through to its +`true` fallback and the re-read ran **unconditionally** on every dispatch, even +for the common case of an object with no formula field at all. + +`objectHasFormulaField` now reads the object's field map through `getObject` — +the accessor the trigger already uses elsewhere (the unknown-object probe in +`start()`, and `buildContext`'s declared-field materialization since #4953) and +the one the real engine actually implements. The now-unreachable +`getObjectConfig` interface member is retired. + +This is a perf-only change — no output changes. Measured on a real ObjectQL +engine (ObjectQL + `@objectstack/driver-sql` on better-sqlite3 `:memory:`, +`record-change-integration.test.ts`): an `afterUpdate` dispatch on an object +with no `formula` field now issues **0** hydration `findOne` calls, down from +**1** before this fix; an object that does declare a `formula` field is +unaffected (still exactly 1). diff --git a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts index ed7b4ad005..7602cb9b10 100644 --- a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts @@ -23,7 +23,7 @@ * that only ever held keys somebody set. */ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect, afterEach, vi } from 'vitest'; import { ObjectKernel } from '@objectstack/core'; import { ObjectQLPlugin } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; @@ -527,3 +527,164 @@ describe('record-change trigger — end-to-end (#1491)', () => { expect(audit[0]?.seen_tag).toBe('keep'); }, 15000); }); + +/** + * #8482 — the hydration schema gate's "measured on a real engine" claim, + * actually measured on a real engine. + * + * `objectHasFormulaField` (record-change-trigger.ts) used to gate on a + * `getObjectConfig` method the concrete ObjectQL engine never implemented, so + * on every real deployment the gate always took its `true` fallback and + * `hydrateComputedFields` re-read via `findOne` on EVERY afterInsert / + * afterUpdate dispatch — even for objects declaring no `formula` field, where + * the re-read (per the code's own doc comment) adds nothing. The trigger's + * OWN unit tests (`record-change-trigger.test.ts`, "computed-field hydration + * guards") only ever proved the gate against a HAND-ATTACHED mock + * (`Object.assign(engine, { getObjectConfig })`) — a true statement about the + * trigger's own logic that said nothing about the real engine, which is + * exactly how a gate that never engaged in production kept a green suite. + * + * This block re-proves the (now `getObject`-based) gate against the REAL + * ObjectQL engine, wired the exact way production is (this file's own + * kernel-boot harness: ObjectQLPlugin + AutomationServicePlugin + + * RecordChangeTriggerPlugin + `@objectstack/driver-sql` on better-sqlite3 + * `:memory:`), by spying on the engine's PUBLIC `findOne` — the same method + * `RecordChangeDataEngine.findOne` structurally types, and the ONLY thing the + * hydration re-read calls. The engine's own by-id-update prior-row fetch + * (`engine.ts` `update()`) reads through `driver.findOne` directly, never the + * public engine method, so this spy counts exactly the trigger's hydration + * re-reads and nothing else — confirmed by the "no formula field" case below + * asserting a hard zero, not just "fewer than before". + * + * Each `it` targets a DIFFERENT write than the flow's own effect (an audit + * object, not the triggering object) so the flow's own write-back can never + * re-fire itself — the self-trigger re-entrancy guard the `record-after-write` + * tests above exercise is deliberately not in play here, so the only variable + * under test is the schema gate. + */ +describe('hydration schema gate — real ObjectQL engine findOne count (#8482)', () => { + const plainObjectDef = (name: string) => ({ + name, + label: name, + fields: { + status: { name: 'status', label: 'Status', type: 'text' as const }, + }, + }); + + const formulaObjectDef = (name: string) => ({ + name, + label: name, + fields: { + status: { name: 'status', label: 'Status', type: 'text' as const }, + full_name: { + name: 'full_name', + label: 'Full Name', + type: 'formula' as const, + expression: { dialect: 'cel', source: "'computed'" }, + }, + }, + }); + + const auditObjectDef = (name: string) => ({ + name, + label: name, + fields: { + note: { name: 'note', label: 'Note', type: 'text' as const }, + }, + }); + + /** A `record-after-update` flow whose own write targets a DIFFERENT object + * (an audit log) — proof the flow actually fired lives in the audit + * table, not in the triggering object, so nothing here can self-fire. */ + function afterUpdateAuditFlow(name: string, object: string, auditObject: string) { + return { + name, + label: name, + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: object, triggerType: 'record-after-update' } }, + { id: 'log', type: 'create_record', label: 'Log', config: { objectName: auditObject, fields: { note: 'seen' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'log' }, + { id: 'e2', source: 'log', target: 'end' }, + ], + }; + } + + it('does NOT re-read via findOne on update for an object with no formula field', async () => { + // `{ logger: { level: 'silent' } }`, NOT this file's other `{ logLevel: + // 'silent' }` — see the doc comment on the #4953 test above for why (a + // pre-existing TS2353 in the frozen TEST_DEBT ledger this new test must + // not add to). + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql'); + const data = kernel.getService('data'); + const automation = kernel.getService('automation'); + + await attachSqlite(objectql); + objectql.registry.registerObject(plainObjectDef('gate_plain'), 'test', 'test'); + objectql.registry.registerObject(auditObjectDef('gate_plain_audit'), 'test', 'test'); + await objectql.syncSchemas(); + automation.registerFlow( + 'gate_plain_audit_flow', + afterUpdateAuditFlow('gate_plain_audit_flow', 'gate_plain', 'gate_plain_audit') as any, + ); + + const created = await data.insert('gate_plain', { status: 'new' }, { context: { userId: 'u_trigger' } }); + const id = Array.isArray(created) ? created[0]?.id : (created as any)?.id ?? created; + await sleep(200); + + const findOneSpy = vi.spyOn(objectql, 'findOne'); + await data.update('gate_plain', { id, status: 'done' }, { context: { userId: 'u_trigger' } }); + await sleep(200); + + // Proof the flow actually dispatched (the gate skipped the RE-READ, not + // the trigger itself). + const audit: any[] = await data.find('gate_plain_audit', {}); + expect(audit).toHaveLength(1); + + expect(findOneSpy).not.toHaveBeenCalled(); + }, 15000); + + it('DOES re-read via findOne on update for an object that declares a formula field', async () => { + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql'); + const data = kernel.getService('data'); + const automation = kernel.getService('automation'); + + await attachSqlite(objectql); + objectql.registry.registerObject(formulaObjectDef('gate_calc'), 'test', 'test'); + objectql.registry.registerObject(auditObjectDef('gate_calc_audit'), 'test', 'test'); + await objectql.syncSchemas(); + automation.registerFlow( + 'gate_calc_audit_flow', + afterUpdateAuditFlow('gate_calc_audit_flow', 'gate_calc', 'gate_calc_audit') as any, + ); + + const created = await data.insert('gate_calc', { status: 'new' }, { context: { userId: 'u_trigger' } }); + const id = Array.isArray(created) ? created[0]?.id : (created as any)?.id ?? created; + await sleep(200); + + const findOneSpy = vi.spyOn(objectql, 'findOne'); + await data.update('gate_calc', { id, status: 'done' }, { context: { userId: 'u_trigger' } }); + await sleep(200); + + const audit: any[] = await data.find('gate_calc_audit', {}); + expect(audit).toHaveLength(1); + + expect(findOneSpy).toHaveBeenCalledTimes(1); + expect(findOneSpy).toHaveBeenCalledWith('gate_calc', expect.objectContaining({ where: { id } })); + }, 15000); +}); diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts index ad8c963436..3e3ee3ea9c 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts @@ -687,15 +687,23 @@ describe('RecordChangeTrigger — skipTriggers suppression', () => { }); }); -// ─── Computed-field hydration guards (#3426 follow-up) ────────────── +// ─── Computed-field hydration guards (#3426 follow-up, #8482) ─────── // // The hydration re-read (#3426, #3445) is gated two ways: skipped when the // object declares no `formula` field, and memoized so N flows on one write -// share a single re-read. These drive a fakeEngine with findOne/getObjectConfig +// share a single re-read. These drive a fakeEngine with findOne/getObject // spies and a hook ctx whose result carries a real `id` (the default hookCtx // uses `_id`, so hydration's `record.id` is undefined and never re-reads). - -describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)', () => { +// +// `getObject` — not a separate `getObjectConfig` — is the schema-gate +// accessor since #8482: it is the ONE object-schema accessor the concrete +// ObjectQL engine actually implements (confirmed by +// `record-change-integration.test.ts`'s "hydration schema gate — real +// ObjectQL engine findOne count" block below, which runs this exact gate +// against a REAL engine rather than a hand-attached mock — the vacuity these +// fakeEngine tests alone cannot rule out). + +describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up, #8482)', () => { /** A hook ctx whose after-row has a real `id`, so hydration proceeds. */ function idCtx(overrides: Partial = {}): HookContext { return hookCtx({ event: 'afterUpdate', result: { id: 't1', status: 'done' }, ...overrides }); @@ -704,22 +712,22 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)' it('skips the re-read when the object declares no formula field (schema gate)', async () => { const { engine, hooks } = fakeEngine(); const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' }); - const getObjectConfig = vi.fn().mockReturnValue({ fields: { title: { type: 'text' } } }); - Object.assign(engine, { findOne, getObjectConfig }); + const getObject = vi.fn().mockReturnValue({ fields: { title: { type: 'text' } } }); + Object.assign(engine, { findOne, getObject }); const trigger = new RecordChangeTrigger(engine, silentLogger()); trigger.start(binding(), async () => {}); await hooks[0].handler(idCtx()); - expect(getObjectConfig).toHaveBeenCalledWith('showcase_task'); + expect(getObject).toHaveBeenCalledWith('showcase_task'); expect(findOne).not.toHaveBeenCalled(); }); it('re-reads and hydrates when the object declares a formula field', async () => { const { engine, hooks } = fakeEngine(); const findOne = vi.fn().mockResolvedValue({ id: 't1', status: 'done', full_name: 'Ada Lovelace' }); - const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); - Object.assign(engine, { findOne, getObjectConfig }); + const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); + Object.assign(engine, { findOne, getObject }); const trigger = new RecordChangeTrigger(engine, silentLogger()); let seen: AutomationContext | undefined; @@ -733,10 +741,10 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)' expect((seen?.record as Record).status).toBe('done'); }); - it('re-reads unconditionally when the engine has no getObjectConfig (prior behavior)', async () => { + it('re-reads unconditionally when the engine has no getObject (prior behavior)', async () => { const { engine, hooks } = fakeEngine(); const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' }); - Object.assign(engine, { findOne }); // no getObjectConfig surface + Object.assign(engine, { findOne }); // no getObject surface const trigger = new RecordChangeTrigger(engine, silentLogger()); trigger.start(binding(), async () => {}); @@ -748,8 +756,8 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)' it('memoizes the re-read across N flows sharing one write (single findOne)', async () => { const { engine, hooks } = fakeEngine(); const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' }); - const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); - Object.assign(engine, { findOne, getObjectConfig }); + const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); + Object.assign(engine, { findOne, getObject }); const trigger = new RecordChangeTrigger(engine, silentLogger()); // Two flows on the same object/event → two hooks, one trigger instance. @@ -768,8 +776,8 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)' it('re-reads again for a DIFFERENT write (distinct ctx, not cross-write cached)', async () => { const { engine, hooks } = fakeEngine(); const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' }); - const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); - Object.assign(engine, { findOne, getObjectConfig }); + const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } }); + Object.assign(engine, { findOne, getObject }); const trigger = new RecordChangeTrigger(engine, silentLogger()); trigger.start(binding(), async () => {}); diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.ts index 14df1189f2..983efe3d9c 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.ts @@ -45,8 +45,8 @@ export interface RecordChangeDataEngine { unregisterHooksByPackage?(packageId: string): number; /** * Optional object-schema accessor (the ObjectQL engine's `getObject`, - * `IObjectQLEngine.getObject` — confirmed WIRED on the concrete engine, - * unlike {@link getObjectConfig} below). Two independent consumers: + * `IObjectQLEngine.getObject` — confirmed WIRED on the concrete engine). + * Three independent consumers: * * 1. {@link RecordChangeTrigger.start} probes existence to call out a * flow whose `objectName` matches no registered object — a hook @@ -56,11 +56,18 @@ export interface RecordChangeDataEngine { * result to make the seeded `record` / `previous` CEL roots TOTAL * over the object's DECLARED fields (#4953 services half) — see * {@link materializeDeclaredFields}. + * 3. {@link RecordChangeTrigger.objectHasFormulaField} reads `.fields` + * off the result to SKIP {@link hydrateComputedFields}'s re-read for + * objects that declare no `formula` field — the only thing that + * re-read adds (#8482). This consumer used to gate on a separate + * `getObjectConfig` member the concrete engine never implemented, so + * the skip was unreachable in production; retired in favor of this + * already-wired accessor. * * Typed loosely (not `ServiceObject`) so this plugin keeps its zero * build-time dependency on objectql; a fixture that returns only what a * probe needs (e.g. `{ name }`) is still a valid implementation for - * consumer (1) and simply contributes no fields to consumer (2). + * consumer (1) and simply contributes no fields to consumers (2)/(3). */ getObject?(name: string): { fields?: Record } | undefined; /** @@ -77,29 +84,6 @@ export interface RecordChangeDataEngine { object: string, options: { where?: Record; fields?: string[]; context?: unknown }, ): Promise | null | undefined>; - /** - * Optional object-config accessor (`getObjectConfig`). When present, - * {@link RecordChangeTrigger} uses it to SKIP the hydration re-read for - * objects that declare no `formula` field — the only thing the re-read - * adds (`summary` fields are stored on write, not read-time computed). - * Returns the object's field map; typed loosely so this plugin keeps its - * zero build-time dependency on objectql. Absent (or unsure) ⇒ the - * trigger re-reads unconditionally (prior behavior — correctness over the - * optimization). - * - * ⚠️ Measured while wiring #4953 (services half): the concrete ObjectQL - * engine does NOT implement a method named `getObjectConfig` — only - * `getObject` (above) exists there. So on the real engine this optional - * hook is always absent and {@link objectHasFormulaField} always takes - * its `true` fallback; the schema-gate skip it describes is unreachable - * in production (harmless — "correctness over the optimization" is - * exactly the documented fallback — but it is dead code, not a working - * gate). Out of scope here (a hydration-perf question, not a - * materialization one); filed separately rather than folded into this - * fix. Left as-is, and deliberately NOT reused for #4953's field lookup — - * {@link getObject} is the one actually wired. - */ - getObjectConfig?(object: string): { fields?: Record } | undefined; } /** Minimal logger surface (matches core's `ctx.logger`). */ @@ -499,8 +483,9 @@ export class RecordChangeTrigger implements FlowTrigger { * * Two guards keep the re-read off the hot path (#3426 follow-up): * - Schema gate: skip entirely when the object declares no `formula` field — - * the only thing the re-read adds. Most objects have none. Needs the - * engine's optional `getObjectConfig`; when unsure, re-reads (see + * the only thing the re-read adds. Most objects have none. Uses the + * engine's `getObject` accessor (the same one {@link buildContext}'s + * materialization step uses, #8482); when unsure, re-reads (see * {@link objectHasFormulaField}). * - Per-write memoization: N flows bound to the same written record share * one re-read, keyed on the shared HookContext (see {@link hydrationCache} @@ -539,18 +524,23 @@ export class RecordChangeTrigger implements FlowTrigger { /** * True unless the engine can POSITIVELY confirm `object` declares no * read-time `formula` field — the only thing {@link hydrateComputedFields}'s - * re-read adds. Uses the engine's synchronous optional `getObjectConfig`; - * when that surface is absent, returns nothing usable, or throws, returns - * `true` so hydration still runs (correctness over the optimization). Not - * cached: `getObjectConfig` is an in-memory lookup, and skipping a cache - * avoids a stale answer if an object's schema is hot-registered with a - * formula field after first use. + * re-read adds. Uses the engine's synchronous optional `getObject` — the + * SAME accessor {@link buildContext}'s materialization step already reads + * (#8482; previously this gated on a separate `getObjectConfig` member the + * concrete ObjectQL engine never implemented, so on the real engine this + * always fell through to the `true` fallback below and the re-read ran + * unconditionally on every afterInsert/afterUpdate dispatch). When + * `getObject` is absent, returns nothing usable, or throws, returns `true` + * so hydration still runs (correctness over the optimization). Not cached: + * `getObject` is an in-memory lookup, and skipping a cache avoids a stale + * answer if an object's schema is hot-registered with a formula field + * after first use. */ private objectHasFormulaField(object: string): boolean { - const getCfg = this.engine.getObjectConfig; - if (typeof getCfg !== 'function') return true; + const getObj = this.engine.getObject; + if (typeof getObj !== 'function') return true; try { - const cfg = getCfg.call(this.engine, object); + const cfg = getObj.call(this.engine, object); const fields = cfg?.fields; if (!fields || typeof fields !== 'object') return true; for (const f of Object.values(fields)) {