diff --git a/.changeset/flow-template-lint-and-hydrate-guards.md b/.changeset/flow-template-lint-and-hydrate-guards.md new file mode 100644 index 000000000..80c406fad --- /dev/null +++ b/.changeset/flow-template-lint-and-hydrate-guards.md @@ -0,0 +1,32 @@ +--- +"@objectstack/lint": patch +"@objectstack/cli": patch +"@objectstack/trigger-record-change": patch +--- + +fix(#3426): build-time warning for unresolvable flow template paths + guard the formula re-read + +Two follow-ups to #3426 (the formula/lookup `{record.}` template gap that #3445 began closing). + +**Build-time signal (the issue's fallback ask).** `os validate` now flags a +record-change flow node whose `{record.}` template cannot resolve — +turning the previous SILENT blank into an advisory warning. Two cases, via the +new `@objectstack/lint` rule `validateFlowTemplatePaths`: + +- `flow-template-unknown-field` — `{record.}` where `` is neither a + declared field nor a system column (a typo like `{record.full_naem}`). +- `flow-template-lookup-traversal` — `{record..}`, a cross-object + hop the seeded record carries only as a scalar id (still unsupported; tracked + on #3426). + +Deliberately quiet: formula fields, bare lookup ids, numeric indexes into +`multiple` lookups (#1872), `json` sub-paths, and system columns are NOT flagged, +and flows bound to an object this stack does not define are skipped (no schema to +compare against). + +**Hydration re-read guards.** The `trigger-record-change` computed-field re-read +(#3445) is now (a) skipped when the object declares no `formula` field — the only +thing it adds — via the engine's optional `getObjectConfig`, and (b) memoized per +write on the shared HookContext, so N flows on one written record share ONE +re-read instead of N. Any uncertainty falls back to the prior unconditional +re-read (correctness over the optimization). diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index cc2487246..75a5a608b 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -19,6 +19,7 @@ import { validateCapabilityReferences } from '@objectstack/lint'; import { validateVisibilityPredicates } from '@objectstack/lint'; import { validateSecurityPosture } from '@objectstack/lint'; import { validateFlowTriggerReadiness } from '@objectstack/lint'; +import { validateFlowTemplatePaths } from '@objectstack/lint'; import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js'; import { @@ -428,6 +429,22 @@ export default class Validate extends Command { } } + // 3g-bis. Flow template path references (#3426): a `{record.}` token + // in a node template that names an unknown field, or hops through a + // lookup relation the seeded record carries only as a scalar id, both + // render a SILENT empty string at runtime. Advisory: the head object + // may come from another package (skipped there), and the runtime still + // produces output (a blank), so nothing is fully broken. + if (!flags.json) printStep('Checking flow template references...'); + const flowTemplateFindings = validateFlowTemplatePaths(normalized as Record); + const flowTemplateWarnings = flowTemplateFindings.filter((f) => f.severity === 'warning'); + if (!flags.json) { + for (const w of flowTemplateWarnings.slice(0, 50)) { + console.log(chalk.yellow(` ⚠ ${w.where}: ${w.message}`)); + console.log(chalk.dim(` ${w.hint}`)); + } + } + // 3e2. [#3425] Readonly flow-write guardrail. A `runAs:user` update_record // that writes a static-`readonly` field is a SILENT no-op — the engine // strips it from the UPDATE payload (#2948) yet the step reports @@ -545,7 +562,7 @@ export default class Validate extends Command { valid: true, manifest: config.manifest, stats, - warnings: [...exprWarnings, ...widgetWarnings, ...actionRefWarnings, ...styleWarnings, ...jsxWarnings, ...capWarnings, ...flowReadinessWarnings, ...securityAdvisories, ...capProviderWarnings], + warnings: [...exprWarnings, ...widgetWarnings, ...actionRefWarnings, ...styleWarnings, ...jsxWarnings, ...capWarnings, ...flowReadinessWarnings, ...flowTemplateWarnings, ...securityAdvisories, ...capProviderWarnings], conversions: conversionNotices, specVersionGap: specGap, duration: timer.elapsed(), diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 592906c5c..b0fa10ff6 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -39,6 +39,16 @@ export type { FlowTriggerReadinessSeverity, } from './validate-flow-trigger-readiness.js'; +export { + validateFlowTemplatePaths, + FLOW_TEMPLATE_UNKNOWN_FIELD, + FLOW_TEMPLATE_LOOKUP_TRAVERSAL, +} from './validate-flow-template-paths.js'; +export type { + FlowTemplatePathFinding, + FlowTemplatePathSeverity, +} from './validate-flow-template-paths.js'; + export { validateReadonlyFlowWrites, FLOW_UPDATE_READONLY_FIELD, diff --git a/packages/lint/src/validate-flow-template-paths.test.ts b/packages/lint/src/validate-flow-template-paths.test.ts new file mode 100644 index 000000000..d8af2953c --- /dev/null +++ b/packages/lint/src/validate-flow-template-paths.test.ts @@ -0,0 +1,192 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + validateFlowTemplatePaths, + FLOW_TEMPLATE_UNKNOWN_FIELD, + FLOW_TEMPLATE_LOOKUP_TRAVERSAL, +} from './validate-flow-template-paths.js'; + +type AnyRec = Record; + +/** A crm_lead object with a scalar, a formula, a lookup, and a multi-lookup. */ +const LEAD_OBJECT: AnyRec = { + name: 'crm_lead', + fields: { + first_name: { name: 'first_name', type: 'text' }, + last_name: { name: 'last_name', type: 'text' }, + company: { name: 'company', type: 'text' }, + full_name: { name: 'full_name', type: 'formula' }, + crm_account: { name: 'crm_account', type: 'lookup', reference_to: 'crm_account' }, + target_channels: { name: 'target_channels', type: 'lookup', reference_to: 'channel', multiple: true }, + payload: { name: 'payload', type: 'json' }, + }, +}; + +/** Build a record-change flow with one notify node carrying the given templates. */ +function flowWith(notify: AnyRec, objectName = 'crm_lead'): AnyRec { + return { + objects: [LEAD_OBJECT], + flows: [ + { + name: 'notify_lead', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName, triggerType: 'record-created' } }, + { id: 'n1', type: 'notify', notify }, + ], + }, + ], + }; +} + +describe('validateFlowTemplatePaths', () => { + it('flags an unknown field in a {record.} template (typo)', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: 'New lead: {record.full_naem}', body: 'x' }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].message).toContain('full_naem'); + }); + + it('flags a lookup cross-object hop {record..}', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: 'From {record.crm_account.name}', body: 'x' }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL); + expect(findings[0].message).toContain('crm_account.name'); + }); + + it('does NOT flag a formula field (valid, hydrated since #3445)', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: 'New lead: {record.full_name}', body: '{record.company}' }), + ); + expect(findings).toHaveLength(0); + }); + + it('does NOT flag a plain scalar field', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: '{record.first_name} {record.last_name}', body: '{record.company}' }), + ); + expect(findings).toHaveLength(0); + }); + + it('does NOT flag a bare lookup id (no sub-path)', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: 'acct {record.crm_account}', body: 'x' }), + ); + expect(findings).toHaveLength(0); + }); + + it('does NOT flag a numeric index into a multiple lookup (#1872)', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: 'ch {record.target_channels.0}', body: 'x' }), + ); + expect(findings).toHaveLength(0); + }); + + it('does NOT flag a sub-path into a json field', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: '{record.payload.foo}', body: 'x' }), + ); + expect(findings).toHaveLength(0); + }); + + it('does NOT flag system/audit columns', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: '{record.id} {record.created_at} {record.owner}', body: 'x' }), + ); + expect(findings).toHaveLength(0); + }); + + it('ignores non-record tokens (flow vars, NOW(), $User)', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: '{some_var.field} {NOW()} {$User.Email}', body: 'x' }), + ); + expect(findings).toHaveLength(0); + }); + + it('skips a flow whose object is not defined in this stack', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'external', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'sys_user', triggerType: 'record-created' } }, + { id: 'n1', type: 'notify', notify: { title: '{record.anything.deep}', body: 'x' } }, + ], + }, + ], + }); + expect(findings).toHaveLength(0); + }); + + it('skips non-record-triggered flows', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'manual', + type: 'screen', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'n1', type: 'notify', notify: { title: '{record.full_naem}', body: 'x' } }, + ], + }, + ], + }); + expect(findings).toHaveLength(0); + }); + + it('dedupes a repeated bad reference to one finding per node', () => { + const findings = validateFlowTemplatePaths( + flowWith({ title: '{record.full_naem}', body: 'again {record.full_naem}' }), + ); + expect(findings).toHaveLength(1); + }); + + it('resolves objectName from the typed start block too', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'typed_start', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', start: { objectName: 'crm_lead', triggerType: 'record-created' } }, + { id: 'n1', type: 'notify', notify: { title: '{record.crm_account.name}', body: 'x' } }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL); + }); + + it('detects references in freeform config and other node types (http url)', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'webhook', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } }, + { id: 'h1', type: 'http', http: { url: 'https://x.test/{record.full_naem}', method: 'GET' } }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD); + }); + + it('returns empty when there are no flows', () => { + expect(validateFlowTemplatePaths({ objects: [LEAD_OBJECT] })).toHaveLength(0); + }); +}); diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts new file mode 100644 index 000000000..5417366db --- /dev/null +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -0,0 +1,297 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Build-time guardrail for `{record.}` template references in a +// record-change flow's node config (#3426). +// +// A `notify` / `update_record` / `http` / ... node interpolates +// `{record.}` tokens against the triggering record. Two authoring +// mistakes render a SILENT empty string at runtime, with no design-time +// signal — exactly the failure #3426 reported: +// +// 1. `{record.}` — the path head is neither a declared field nor a +// system column. Almost always a typo (`{record.full_naem}`). The template +// engine resolves it to `undefined` -> '' with no warning. +// +// 2. `{record..}` — a cross-object hop through a lookup / +// master_detail / user / tree relation. The seeded flow record carries the +// relation as a SCALAR foreign-key id, not an expanded object (a default +// data-API read does not expand relations either — see #3426's hydration +// note and #1872). So `record.account.name` walks `.name` on a string id +// and yields '' silently. Not resolved today; tracked on #3426. +// +// A pure `(stack) => Finding[]` rule (ADR-0019), run from `os validate` and +// reusable by AI authoring. Both findings are warnings: the runtime still +// produces output (a blank), nothing is fully broken, and the head object may +// legitimately come from another installed package (skipped — see below). +// +// Deliberately conservative to keep false positives near zero: +// - Only `record.`-prefixed tokens are checked. Other `{var}` tokens address +// flow variables / node outputs the rule cannot resolve statically. +// - Only flows bound to an object THIS stack defines are checked; when the +// object is unknown here (another package, `sys_*`) the rule has no schema +// to compare against and skips the whole flow. +// - `formula` / `summary` fields are VALID heads (formula is hydrated onto the +// record since #3445; summary is stored on write) — never flagged. +// - A trailing NUMERIC segment (`{record.target_channels.0}`) is an array +// index into a `multiple` lookup (#1872), not a cross-object hop — allowed. +// - Structured scalar heads (`json` / `composite` / `repeater` / `record`) may +// carry legitimate sub-paths — their `.` access is left alone. + +export type FlowTemplatePathSeverity = 'error' | 'warning'; + +export interface FlowTemplatePathFinding { + severity: FlowTemplatePathSeverity; + rule: string; + /** Human-readable location, e.g. `flow "notify_lead" node "notify"`. */ + where: string; + /** Config path, e.g. `flows[0].nodes[2]`. */ + path: string; + message: string; + hint: string; +} + +// Rule ids (registry entries). +export const FLOW_TEMPLATE_UNKNOWN_FIELD = 'flow-template-unknown-field'; +export const FLOW_TEMPLATE_LOOKUP_TRAVERSAL = 'flow-template-lookup-traversal'; + +type AnyRec = Record; + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ + name, + ...(def as AnyRec), + })); + } + return []; +} + +// System/audit columns the platform injects on every object — always +// addressable in a `{record.}` template even though they are not authored +// fields. Mirrors `validateFormLayout`'s system-field set plus the audit/tenant +// columns from `FIELD_GROUP_SYSTEM_FIELDS`. +const SYSTEM_FIELDS: ReadonlySet = new Set([ + 'id', + 'name', + 'owner', + 'owner_id', + 'created_at', + 'created_by', + 'updated_at', + 'updated_by', + 'organization_id', + 'tenant_id', + 'is_deleted', + 'deleted_at', + 'record_type', +]); + +// Field types that address ANOTHER object — a `.` hop through one is +// a cross-object traversal the seeded flow record does not expand. +const RELATION_TYPES: ReadonlySet = new Set([ + 'lookup', + 'master_detail', + 'user', + 'tree', +]); + +/** Build a `fieldName -> type` map for an object (declared fields only). */ +function fieldTypesOf(obj: AnyRec): Map { + const types = new Map(); + for (const f of asArray(obj.fields)) { + if (typeof f.name === 'string') { + types.set(f.name, typeof f.type === 'string' ? f.type : ''); + } + } + return types; +} + +/** + * Extract the `record.` references from a template string. Mirrors the + * runtime interpolator's token grammar (service-automation builtin/template.ts): + * a `{...}` token whose body is a dotted path whose HEAD is `record`. Arithmetic + * / function tokens (`{NOW()}`, `{a + b}`) and non-`record` heads are ignored. + * + * Returns each reference's segment list AFTER the `record` head, e.g. + * `{record.account.name}` -> `[['account', 'name']]`. + */ +function recordRefsIn(text: string): string[][] { + const refs: string[][] = []; + const tokenRe = /\{([^{}]+)\}/g; + let m: RegExpExecArray | null; + while ((m = tokenRe.exec(text)) !== null) { + const body = m[1].trim(); + // Pure dotted path only (same shape the interpolator's fast path accepts): + // identifier head, then identifier-or-numeric segments. Anything with + // operators / spaces / quotes is an arithmetic token — not a bare field ref. + if (!/^[A-Za-z_$][\w$]*(?:\.(?:[A-Za-z_$][\w$]*|\d+))*$/.test(body)) continue; + const segments = body.split('.'); + if (segments[0] !== 'record') continue; + const rest = segments.slice(1); + if (rest.length > 0) refs.push(rest); + } + return refs; +} + +/** Recursively collect templated string leaves from a config-bearing block. */ +function stringLeaves(value: unknown, out: string[]): void { + if (typeof value === 'string') { + if (value.includes('{')) out.push(value); + return; + } + if (Array.isArray(value)) { + for (const v of value) stringLeaves(v, out); + return; + } + if (value && typeof value === 'object') { + for (const v of Object.values(value as AnyRec)) stringLeaves(v, out); + } +} + +// The typed config blocks + freeform `config` a node interpolates at runtime. +// We scan every string leaf under these (the runtime `interpolate()` walks the +// whole config recursively), NOT `id` / `type` / `label` / `position`, which are +// never templated. +const NODE_CONFIG_KEYS = [ + 'config', + 'notify', + 'update_record', + 'create_record', + 'http', + 'script', + 'screen', + 'wait', + 'approval', + 'connector_action', + 'subflow', + 'decision', + 'start', +]; + +/** True when the flow is armed by a record lifecycle event. */ +function isRecordTriggered(flow: AnyRec, startConfig: AnyRec): boolean { + if (flow.type === 'record_change') return true; + const triggerType = typeof startConfig.triggerType === 'string' ? startConfig.triggerType : undefined; + return !!triggerType && triggerType.startsWith('record-'); +} + +/** Resolve the object a record-change flow binds to, from its start node. */ +function boundObjectOf(flow: AnyRec): string | undefined { + const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + const start = nodes.find((n) => n?.type === 'start'); + if (!start) return undefined; + const config = (start.config ?? {}) as AnyRec; + const typed = (start.start ?? {}) as AnyRec; + const fromConfig = typeof config.objectName === 'string' ? config.objectName : undefined; + const fromTyped = typeof typed.objectName === 'string' ? typed.objectName : undefined; + return fromConfig ?? fromTyped; +} + +/** + * Validate `{record.}` template references across every record-change + * flow. Pure and dependency-free; safe on pre- or post-parse stacks. + */ +export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFinding[] { + const findings: FlowTemplatePathFinding[] = []; + const flows = asArray(stack.flows); + if (flows.length === 0) return findings; + + const objectsByName = new Map(); + for (const obj of asArray(stack.objects)) { + if (typeof obj.name === 'string') objectsByName.set(obj.name, obj); + } + + flows.forEach((flow, flowIndex) => { + const flowName = typeof flow.name === 'string' ? flow.name : `#${flowIndex}`; + const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + const start = (nodes.find((n) => n?.type === 'start')?.config ?? {}) as AnyRec; + if (!isRecordTriggered(flow, start)) return; + + const objectName = boundObjectOf(flow); + if (!objectName) return; + const obj = objectsByName.get(objectName); + // Unknown object here -> no schema to compare against (another package / + // `sys_*`). The trigger-readiness rule already flags a wrong name; we can't + // meaningfully classify field paths, so skip the whole flow. + if (!obj) return; + + const fieldTypes = fieldTypesOf(obj); + + nodes.forEach((node, nodeIndex) => { + if (typeof node !== 'object' || !node) return; + const nodeLabel = + typeof node.type === 'string' ? node.type : typeof node.id === 'string' ? node.id : `#${nodeIndex}`; + + // Collect templated string leaves from the config-bearing blocks only. + const leaves: string[] = []; + for (const key of NODE_CONFIG_KEYS) { + if (key in node) stringLeaves((node as AnyRec)[key], leaves); + } + if (leaves.length === 0) return; + + // Dedupe references so one repeated typo yields one finding per node. + const seenUnknown = new Set(); + const seenTraversal = new Set(); + + for (const leaf of leaves) { + for (const rest of recordRefsIn(leaf)) { + const head = rest[0]; + const hasSubPath = rest.length > 1; + // A trailing numeric segment is an array index (#1872), not a hop. + const nextIsIdentifier = hasSubPath && !/^\d+$/.test(rest[1]); + + const isKnown = fieldTypes.has(head) || SYSTEM_FIELDS.has(head); + + if (!isKnown) { + if (seenUnknown.has(head)) continue; + seenUnknown.add(head); + findings.push({ + severity: 'warning', + rule: FLOW_TEMPLATE_UNKNOWN_FIELD, + where: `flow "${flowName}" node "${nodeLabel}"`, + path: `flows[${flowIndex}].nodes[${nodeIndex}]`, + message: + `template references '{record.${rest.join('.')}}', but '${head}' is not a field on ` + + `object '${objectName}' — it resolves to an empty string at runtime (silently).`, + hint: + `Check the field name against the object's field definitions (e.g. '{record.full_name}', ` + + `not '{record.full_naem}'). System columns like id/created_at/owner are also addressable.`, + }); + continue; + } + + if (nextIsIdentifier) { + const headType = fieldTypes.get(head) ?? ''; + if (RELATION_TYPES.has(headType)) { + const key = rest.join('.'); + if (seenTraversal.has(key)) continue; + seenTraversal.add(key); + findings.push({ + severity: 'warning', + rule: FLOW_TEMPLATE_LOOKUP_TRAVERSAL, + where: `flow "${flowName}" node "${nodeLabel}"`, + path: `flows[${flowIndex}].nodes[${nodeIndex}]`, + message: + `template references '{record.${key}}', a cross-object hop through the ${headType} field ` + + `'${head}' — the flow record carries '${head}' as a scalar id, not an expanded object, so ` + + `this resolves to an empty string at runtime (silently).`, + hint: + `Single-hop lookup traversal in templates is not resolved yet (tracked on #3426). ` + + `Reference the foreign-key id directly ('{record.${head}}'), or add a formula field on ` + + `'${objectName}' that projects the related value and reference that instead.`, + }); + } + // STRUCTURED_TYPES + any other scalar `.sub` access is left alone: + // json/composite/record sub-paths are legitimate in-row reads, and + // a plain scalar `.sub` is rare enough that flagging it would risk + // more false positives than it prevents. + } + } + } + }); + }); + + return findings; +} 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 aa86b0408..74a036ff1 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 @@ -579,3 +579,96 @@ describe('RecordChangeTrigger — skipTriggers suppression', () => { expect(fired).toBe(1); }); }); + +// ─── Computed-field hydration guards (#3426 follow-up) ────────────── +// +// 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 +// 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)', () => { + /** 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 }); + } + + 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 trigger = new RecordChangeTrigger(engine, silentLogger()); + + trigger.start(binding(), async () => {}); + await hooks[0].handler(idCtx()); + + expect(getObjectConfig).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 trigger = new RecordChangeTrigger(engine, silentLogger()); + let seen: AutomationContext | undefined; + + trigger.start(binding(), async (ctx) => { seen = ctx; }); + await hooks[0].handler(idCtx()); + + expect(findOne).toHaveBeenCalledTimes(1); + expect(findOne).toHaveBeenCalledWith('showcase_task', expect.objectContaining({ where: { id: 't1' } })); + // Formula virtual is now visible to the flow; raw scalar still wins. + expect((seen?.record as Record).full_name).toBe('Ada Lovelace'); + expect((seen?.record as Record).status).toBe('done'); + }); + + it('re-reads unconditionally when the engine has no getObjectConfig (prior behavior)', async () => { + const { engine, hooks } = fakeEngine(); + const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' }); + Object.assign(engine, { findOne }); // no getObjectConfig surface + const trigger = new RecordChangeTrigger(engine, silentLogger()); + + trigger.start(binding(), async () => {}); + await hooks[0].handler(idCtx()); + + expect(findOne).toHaveBeenCalledTimes(1); + }); + + 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 trigger = new RecordChangeTrigger(engine, silentLogger()); + + // Two flows on the same object/event → two hooks, one trigger instance. + trigger.start(binding({ flowName: 'flow_a' }), async () => {}); + trigger.start(binding({ flowName: 'flow_b' }), async () => {}); + expect(hooks).toHaveLength(2); + + // The engine passes ONE ctx ref to every handler for a single write. + const ctx = idCtx(); + await hooks[0].handler(ctx); + await hooks[1].handler(ctx); + + expect(findOne).toHaveBeenCalledTimes(1); + }); + + 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 trigger = new RecordChangeTrigger(engine, silentLogger()); + + trigger.start(binding(), async () => {}); + await hooks[0].handler(idCtx()); + await hooks[0].handler(idCtx()); // a fresh ctx object = a new write + + expect(findOne).toHaveBeenCalledTimes(2); + }); +}); 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 42ae2df81..9e0d7ba22 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.ts @@ -65,6 +65,17 @@ export interface RecordChangeDataEngine { object: string, options: { where?: Record; fields?: string[]; context?: unknown }, ): Promise | null | undefined>; + /** + * Optional object-config accessor (the ObjectQL engine's `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). + */ + getObjectConfig?(object: string): { fields?: Record } | undefined; } /** Minimal logger surface (matches core's `ctx.logger`). */ @@ -142,6 +153,15 @@ export class RecordChangeTrigger implements FlowTrigger { private readonly logger: TriggerLogger; /** flowName → packageId used for its hook(s), so stop() can unregister it. */ private readonly bound = new Map(); + /** + * Per-write re-read cache for computed-field hydration (#3426 follow-up). + * The engine passes ONE HookContext reference to every binding's handler for + * a single write, so keying on it lets N flows bound to the same written + * record share a SINGLE re-read instead of issuing N. A WeakMap, so a context + * GC'd after its write drops the entry automatically — no manual eviction, + * no unbounded growth. + */ + private readonly hydrationCache = new WeakMap | undefined>>>(); constructor(engine: RecordChangeDataEngine, logger: TriggerLogger) { this.engine = engine; @@ -281,7 +301,7 @@ export class RecordChangeTrigger implements FlowTrigger { // Hydrate read-time computed fields (formula virtuals) onto the seeded // record so the flow's start condition and every `{record.}` // template resolve them — the raw hook row never carries them (#3426). - const hydrated = await this.hydrateComputedFields(object, ctx.event, record); + const hydrated = await this.hydrateComputedFields(object, ctx.event, record, ctx); return { record: hydrated, @@ -333,6 +353,15 @@ export class RecordChangeTrigger implements FlowTrigger { * object, breaking templates/conditions that use the bare id (e.g. * #1872's `{record.target_channels.0}`). Tracked separately on #3426. * + * 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 + * {@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} + * and {@link readFullRecordOnce}). + * * Any failure (no read surface, no id, a throw, an empty read) falls back to * the raw record — hydration must never break the flow it feeds. */ @@ -340,29 +369,98 @@ export class RecordChangeTrigger implements FlowTrigger { object: string | undefined, hookEvent: string | undefined, record: Record, + hookCtx?: HookContext, ): Promise> { if (typeof this.engine.findOne !== 'function') return record; if (hookEvent !== 'afterInsert' && hookEvent !== 'afterUpdate') return record; if (!object) return record; const id = (record as { id?: unknown }).id; if (id == null || id === '') return record; + + // Schema gate: the re-read exists ONLY to add read-time `formula` + // virtuals. When the object positively declares none it can add nothing + // the raw hook row lacks, so skip the query. Most objects have no formula + // field, so this removes the re-read from the common write path. + if (!this.objectHasFormulaField(object)) return record; + + const full = await this.readFullRecordOnce(object, id, hookCtx); + if (full) { + // Raw hook fields win; the re-read only contributes keys the raw + // row lacks (the formula virtuals + any other read-time field). + return { ...full, ...record }; + } + return record; + } + + /** + * 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. + */ + private objectHasFormulaField(object: string): boolean { + const getCfg = this.engine.getObjectConfig; + if (typeof getCfg !== 'function') return true; try { - const full = await this.engine.findOne(object, { - where: { id }, - // Elevated read: adds computed fields without RLS/FLS masking the - // ones the raw row already carried. - context: { isSystem: true, positions: [], permissions: [] }, - }); - if (full && typeof full === 'object') { - // Raw hook fields win; the re-read only contributes keys the raw - // row lacks (the formula virtuals + any other read-time field). - return { ...(full as Record), ...record }; + const cfg = getCfg.call(this.engine, object); + const fields = cfg?.fields; + if (!fields || typeof fields !== 'object') return true; + for (const f of Object.values(fields)) { + if (f && typeof f === 'object' && (f as { type?: unknown }).type === 'formula') return true; } - } catch (err) { - this.logger.debug?.( - `[record-change] computed-field hydration skipped for '${object}': ${(err as Error)?.message ?? String(err)}`, - ); + return false; + } catch { + return true; } - return record; + } + + /** + * Re-read the just-written record as an elevated system principal, memoized + * per write. Keying the cache on the shared HookContext means N flow bindings + * on the same written record issue ONE query, not N. Any failure resolves to + * `undefined` (the caller falls back to the raw record) — hydration must + * never break its flow. Without a HookContext key it reads directly. + */ + private async readFullRecordOnce( + object: string, + id: unknown, + hookCtx?: HookContext, + ): Promise | undefined> { + const run = async (): Promise | undefined> => { + const findOne = this.engine.findOne; + if (typeof findOne !== 'function') return undefined; + try { + const full = await findOne.call(this.engine, object, { + where: { id }, + // Elevated read: adds computed fields without RLS/FLS masking + // the ones the raw row already carried. + context: { isSystem: true, positions: [], permissions: [] }, + }); + return full && typeof full === 'object' ? (full as Record) : undefined; + } catch (err) { + this.logger.debug?.( + `[record-change] computed-field hydration skipped for '${object}': ${(err as Error)?.message ?? String(err)}`, + ); + return undefined; + } + }; + + if (!hookCtx) return run(); + const ctxKey = hookCtx as unknown as object; + let perWrite = this.hydrationCache.get(ctxKey); + if (!perWrite) { + perWrite = new Map(); + this.hydrationCache.set(ctxKey, perWrite); + } + const cacheKey = `${object}:${String(id)}`; + const existing = perWrite.get(cacheKey); + if (existing) return existing; + const pending = run(); + perWrite.set(cacheKey, pending); + return pending; } }