Skip to content

Commit 79e5931

Browse files
os-zhuangclaude
andauthored
fix(trigger-record-change): wire the hydration schema gate to the real getObject accessor (#8482) (#8548)
objectHasFormulaField gated the computed-field hydration re-read (a findOne on every afterInsert/afterUpdate dispatch) on an optional getObjectConfig accessor the concrete ObjectQL engine never implemented, so the gate always took its "re-read unconditionally" fallback in production, even for objects declaring no formula field where the re-read adds nothing. Points the gate at getObject instead -- 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 ObjectQL engine actually implements. Retires the now-unreferenced getObjectConfig interface member and its doc comment. Perf-only; no output change. Re-proves the gate against a REAL engine (record-change-integration.test.ts, ObjectQL + driver-sql on better-sqlite3 :memory:) rather than trusting the trigger's own hand-mocked fakeEngine tests, which passed regardless of whether the gate could ever engage in production -- exactly the vacuity this card exists to close. Measured findOne delta on a real afterUpdate dispatch: 1 -> 0 for an object with no formula field, unchanged (1) for one that declares a formula field. Existing fakeEngine hydration-guard tests updated to mock getObject instead of the retired getObjectConfig, so they test the accessor the trigger actually reads. Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk Co-authored-by: Claude <noreply@anthropic.com>
1 parent bbe05de commit 79e5931

4 files changed

Lines changed: 239 additions & 53 deletions

File tree

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
"@objectstack/trigger-record-change": patch
3+
---
4+
5+
fix(trigger-record-change): the hydration schema gate now actually engages on the real engine (#8482)
6+
7+
`RecordChangeTrigger`'s computed-field hydration re-read (a `findOne` on every
8+
`afterInsert`/`afterUpdate` dispatch, added to surface `formula` virtual fields
9+
in the seeded flow record) was meant to be skipped for objects that declare no
10+
`formula` field — the only thing the re-read adds. The skip was gated on an
11+
optional `getObjectConfig` accessor that the concrete ObjectQL engine never
12+
implemented, so on every real deployment the gate always fell through to its
13+
`true` fallback and the re-read ran **unconditionally** on every dispatch, even
14+
for the common case of an object with no formula field at all.
15+
16+
`objectHasFormulaField` now reads the object's field map through `getObject`
17+
the accessor the trigger already uses elsewhere (the unknown-object probe in
18+
`start()`, and `buildContext`'s declared-field materialization since #4953) and
19+
the one the real engine actually implements. The now-unreachable
20+
`getObjectConfig` interface member is retired.
21+
22+
This is a perf-only change — no output changes. Measured on a real ObjectQL
23+
engine (ObjectQL + `@objectstack/driver-sql` on better-sqlite3 `:memory:`,
24+
`record-change-integration.test.ts`): an `afterUpdate` dispatch on an object
25+
with no `formula` field now issues **0** hydration `findOne` calls, down from
26+
**1** before this fix; an object that does declare a `formula` field is
27+
unaffected (still exactly 1).

packages/triggers/trigger-record-change/src/record-change-integration.test.ts

Lines changed: 162 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
* that only ever held keys somebody set.
2424
*/
2525

26-
import { describe, it, expect, afterEach } from 'vitest';
26+
import { describe, it, expect, afterEach, vi } from 'vitest';
2727
import { ObjectKernel } from '@objectstack/core';
2828
import { ObjectQLPlugin } from '@objectstack/objectql';
2929
import { SqlDriver } from '@objectstack/driver-sql';
@@ -527,3 +527,164 @@ describe('record-change trigger — end-to-end (#1491)', () => {
527527
expect(audit[0]?.seen_tag).toBe('keep');
528528
}, 15000);
529529
});
530+
531+
/**
532+
* #8482 — the hydration schema gate's "measured on a real engine" claim,
533+
* actually measured on a real engine.
534+
*
535+
* `objectHasFormulaField` (record-change-trigger.ts) used to gate on a
536+
* `getObjectConfig` method the concrete ObjectQL engine never implemented, so
537+
* on every real deployment the gate always took its `true` fallback and
538+
* `hydrateComputedFields` re-read via `findOne` on EVERY afterInsert /
539+
* afterUpdate dispatch — even for objects declaring no `formula` field, where
540+
* the re-read (per the code's own doc comment) adds nothing. The trigger's
541+
* OWN unit tests (`record-change-trigger.test.ts`, "computed-field hydration
542+
* guards") only ever proved the gate against a HAND-ATTACHED mock
543+
* (`Object.assign(engine, { getObjectConfig })`) — a true statement about the
544+
* trigger's own logic that said nothing about the real engine, which is
545+
* exactly how a gate that never engaged in production kept a green suite.
546+
*
547+
* This block re-proves the (now `getObject`-based) gate against the REAL
548+
* ObjectQL engine, wired the exact way production is (this file's own
549+
* kernel-boot harness: ObjectQLPlugin + AutomationServicePlugin +
550+
* RecordChangeTriggerPlugin + `@objectstack/driver-sql` on better-sqlite3
551+
* `:memory:`), by spying on the engine's PUBLIC `findOne` — the same method
552+
* `RecordChangeDataEngine.findOne` structurally types, and the ONLY thing the
553+
* hydration re-read calls. The engine's own by-id-update prior-row fetch
554+
* (`engine.ts` `update()`) reads through `driver.findOne` directly, never the
555+
* public engine method, so this spy counts exactly the trigger's hydration
556+
* re-reads and nothing else — confirmed by the "no formula field" case below
557+
* asserting a hard zero, not just "fewer than before".
558+
*
559+
* Each `it` targets a DIFFERENT write than the flow's own effect (an audit
560+
* object, not the triggering object) so the flow's own write-back can never
561+
* re-fire itself — the self-trigger re-entrancy guard the `record-after-write`
562+
* tests above exercise is deliberately not in play here, so the only variable
563+
* under test is the schema gate.
564+
*/
565+
describe('hydration schema gate — real ObjectQL engine findOne count (#8482)', () => {
566+
const plainObjectDef = (name: string) => ({
567+
name,
568+
label: name,
569+
fields: {
570+
status: { name: 'status', label: 'Status', type: 'text' as const },
571+
},
572+
});
573+
574+
const formulaObjectDef = (name: string) => ({
575+
name,
576+
label: name,
577+
fields: {
578+
status: { name: 'status', label: 'Status', type: 'text' as const },
579+
full_name: {
580+
name: 'full_name',
581+
label: 'Full Name',
582+
type: 'formula' as const,
583+
expression: { dialect: 'cel', source: "'computed'" },
584+
},
585+
},
586+
});
587+
588+
const auditObjectDef = (name: string) => ({
589+
name,
590+
label: name,
591+
fields: {
592+
note: { name: 'note', label: 'Note', type: 'text' as const },
593+
},
594+
});
595+
596+
/** A `record-after-update` flow whose own write targets a DIFFERENT object
597+
* (an audit log) — proof the flow actually fired lives in the audit
598+
* table, not in the triggering object, so nothing here can self-fire. */
599+
function afterUpdateAuditFlow(name: string, object: string, auditObject: string) {
600+
return {
601+
name,
602+
label: name,
603+
type: 'record_change',
604+
nodes: [
605+
{ id: 'start', type: 'start', label: 'Start', config: { objectName: object, triggerType: 'record-after-update' } },
606+
{ id: 'log', type: 'create_record', label: 'Log', config: { objectName: auditObject, fields: { note: 'seen' } } },
607+
{ id: 'end', type: 'end', label: 'End' },
608+
],
609+
edges: [
610+
{ id: 'e1', source: 'start', target: 'log' },
611+
{ id: 'e2', source: 'log', target: 'end' },
612+
],
613+
};
614+
}
615+
616+
it('does NOT re-read via findOne on update for an object with no formula field', async () => {
617+
// `{ logger: { level: 'silent' } }`, NOT this file's other `{ logLevel:
618+
// 'silent' }` — see the doc comment on the #4953 test above for why (a
619+
// pre-existing TS2353 in the frozen TEST_DEBT ledger this new test must
620+
// not add to).
621+
const kernel = new ObjectKernel({ logger: { level: 'silent' } });
622+
await kernel.use(new ObjectQLPlugin());
623+
await kernel.use(new AutomationServicePlugin());
624+
await kernel.use(new RecordChangeTriggerPlugin());
625+
await kernel.bootstrap();
626+
627+
const objectql = kernel.getService<TestObjectQLEngine>('objectql');
628+
const data = kernel.getService<IDataEngine>('data');
629+
const automation = kernel.getService<AutomationEngine>('automation');
630+
631+
await attachSqlite(objectql);
632+
objectql.registry.registerObject(plainObjectDef('gate_plain'), 'test', 'test');
633+
objectql.registry.registerObject(auditObjectDef('gate_plain_audit'), 'test', 'test');
634+
await objectql.syncSchemas();
635+
automation.registerFlow(
636+
'gate_plain_audit_flow',
637+
afterUpdateAuditFlow('gate_plain_audit_flow', 'gate_plain', 'gate_plain_audit') as any,
638+
);
639+
640+
const created = await data.insert('gate_plain', { status: 'new' }, { context: { userId: 'u_trigger' } });
641+
const id = Array.isArray(created) ? created[0]?.id : (created as any)?.id ?? created;
642+
await sleep(200);
643+
644+
const findOneSpy = vi.spyOn(objectql, 'findOne');
645+
await data.update('gate_plain', { id, status: 'done' }, { context: { userId: 'u_trigger' } });
646+
await sleep(200);
647+
648+
// Proof the flow actually dispatched (the gate skipped the RE-READ, not
649+
// the trigger itself).
650+
const audit: any[] = await data.find('gate_plain_audit', {});
651+
expect(audit).toHaveLength(1);
652+
653+
expect(findOneSpy).not.toHaveBeenCalled();
654+
}, 15000);
655+
656+
it('DOES re-read via findOne on update for an object that declares a formula field', async () => {
657+
const kernel = new ObjectKernel({ logger: { level: 'silent' } });
658+
await kernel.use(new ObjectQLPlugin());
659+
await kernel.use(new AutomationServicePlugin());
660+
await kernel.use(new RecordChangeTriggerPlugin());
661+
await kernel.bootstrap();
662+
663+
const objectql = kernel.getService<TestObjectQLEngine>('objectql');
664+
const data = kernel.getService<IDataEngine>('data');
665+
const automation = kernel.getService<AutomationEngine>('automation');
666+
667+
await attachSqlite(objectql);
668+
objectql.registry.registerObject(formulaObjectDef('gate_calc'), 'test', 'test');
669+
objectql.registry.registerObject(auditObjectDef('gate_calc_audit'), 'test', 'test');
670+
await objectql.syncSchemas();
671+
automation.registerFlow(
672+
'gate_calc_audit_flow',
673+
afterUpdateAuditFlow('gate_calc_audit_flow', 'gate_calc', 'gate_calc_audit') as any,
674+
);
675+
676+
const created = await data.insert('gate_calc', { status: 'new' }, { context: { userId: 'u_trigger' } });
677+
const id = Array.isArray(created) ? created[0]?.id : (created as any)?.id ?? created;
678+
await sleep(200);
679+
680+
const findOneSpy = vi.spyOn(objectql, 'findOne');
681+
await data.update('gate_calc', { id, status: 'done' }, { context: { userId: 'u_trigger' } });
682+
await sleep(200);
683+
684+
const audit: any[] = await data.find('gate_calc_audit', {});
685+
expect(audit).toHaveLength(1);
686+
687+
expect(findOneSpy).toHaveBeenCalledTimes(1);
688+
expect(findOneSpy).toHaveBeenCalledWith('gate_calc', expect.objectContaining({ where: { id } }));
689+
}, 15000);
690+
});

packages/triggers/trigger-record-change/src/record-change-trigger.test.ts

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -687,15 +687,23 @@ describe('RecordChangeTrigger — skipTriggers suppression', () => {
687687
});
688688
});
689689

690-
// ─── Computed-field hydration guards (#3426 follow-up) ──────────────
690+
// ─── Computed-field hydration guards (#3426 follow-up, #8482) ───────
691691
//
692692
// The hydration re-read (#3426, #3445) is gated two ways: skipped when the
693693
// object declares no `formula` field, and memoized so N flows on one write
694-
// share a single re-read. These drive a fakeEngine with findOne/getObjectConfig
694+
// share a single re-read. These drive a fakeEngine with findOne/getObject
695695
// spies and a hook ctx whose result carries a real `id` (the default hookCtx
696696
// uses `_id`, so hydration's `record.id` is undefined and never re-reads).
697-
698-
describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)', () => {
697+
//
698+
// `getObject` — not a separate `getObjectConfig` — is the schema-gate
699+
// accessor since #8482: it is the ONE object-schema accessor the concrete
700+
// ObjectQL engine actually implements (confirmed by
701+
// `record-change-integration.test.ts`'s "hydration schema gate — real
702+
// ObjectQL engine findOne count" block below, which runs this exact gate
703+
// against a REAL engine rather than a hand-attached mock — the vacuity these
704+
// fakeEngine tests alone cannot rule out).
705+
706+
describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up, #8482)', () => {
699707
/** A hook ctx whose after-row has a real `id`, so hydration proceeds. */
700708
function idCtx(overrides: Partial<HookContext> = {}): HookContext {
701709
return hookCtx({ event: 'afterUpdate', result: { id: 't1', status: 'done' }, ...overrides });
@@ -704,22 +712,22 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)'
704712
it('skips the re-read when the object declares no formula field (schema gate)', async () => {
705713
const { engine, hooks } = fakeEngine();
706714
const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' });
707-
const getObjectConfig = vi.fn().mockReturnValue({ fields: { title: { type: 'text' } } });
708-
Object.assign(engine, { findOne, getObjectConfig });
715+
const getObject = vi.fn().mockReturnValue({ fields: { title: { type: 'text' } } });
716+
Object.assign(engine, { findOne, getObject });
709717
const trigger = new RecordChangeTrigger(engine, silentLogger());
710718

711719
trigger.start(binding(), async () => {});
712720
await hooks[0].handler(idCtx());
713721

714-
expect(getObjectConfig).toHaveBeenCalledWith('showcase_task');
722+
expect(getObject).toHaveBeenCalledWith('showcase_task');
715723
expect(findOne).not.toHaveBeenCalled();
716724
});
717725

718726
it('re-reads and hydrates when the object declares a formula field', async () => {
719727
const { engine, hooks } = fakeEngine();
720728
const findOne = vi.fn().mockResolvedValue({ id: 't1', status: 'done', full_name: 'Ada Lovelace' });
721-
const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
722-
Object.assign(engine, { findOne, getObjectConfig });
729+
const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
730+
Object.assign(engine, { findOne, getObject });
723731
const trigger = new RecordChangeTrigger(engine, silentLogger());
724732
let seen: AutomationContext | undefined;
725733

@@ -733,10 +741,10 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)'
733741
expect((seen?.record as Record<string, unknown>).status).toBe('done');
734742
});
735743

736-
it('re-reads unconditionally when the engine has no getObjectConfig (prior behavior)', async () => {
744+
it('re-reads unconditionally when the engine has no getObject (prior behavior)', async () => {
737745
const { engine, hooks } = fakeEngine();
738746
const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' });
739-
Object.assign(engine, { findOne }); // no getObjectConfig surface
747+
Object.assign(engine, { findOne }); // no getObject surface
740748
const trigger = new RecordChangeTrigger(engine, silentLogger());
741749

742750
trigger.start(binding(), async () => {});
@@ -748,8 +756,8 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)'
748756
it('memoizes the re-read across N flows sharing one write (single findOne)', async () => {
749757
const { engine, hooks } = fakeEngine();
750758
const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' });
751-
const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
752-
Object.assign(engine, { findOne, getObjectConfig });
759+
const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
760+
Object.assign(engine, { findOne, getObject });
753761
const trigger = new RecordChangeTrigger(engine, silentLogger());
754762

755763
// 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)'
768776
it('re-reads again for a DIFFERENT write (distinct ctx, not cross-write cached)', async () => {
769777
const { engine, hooks } = fakeEngine();
770778
const findOne = vi.fn().mockResolvedValue({ id: 't1', full_name: 'X' });
771-
const getObjectConfig = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
772-
Object.assign(engine, { findOne, getObjectConfig });
779+
const getObject = vi.fn().mockReturnValue({ fields: { full_name: { type: 'formula' } } });
780+
Object.assign(engine, { findOne, getObject });
773781
const trigger = new RecordChangeTrigger(engine, silentLogger());
774782

775783
trigger.start(binding(), async () => {});

0 commit comments

Comments
 (0)