Skip to content

Commit b9cc17d

Browse files
baozhoutaoclaude
andauthored
chore(objectql): retire applyFormulaPlan's zero-caller nowSnapshot parameter (#5699) (#5894)
The fourth optional parameter `nowSnapshot?: Date` had exactly one effect, `nowSnapshot ?? new Date()`, and not one of the three call sites (find, findOne, and #5504's write-response hydration) ever passed it. Dormant from birth, so it is removed rather than archived: a parameter that looks live makes every reader conclude the caller can pin the instant. Narrows the docstring's "mirrors applyFieldDefaults" claim to the half that holds (same context shape, one expression vocabulary) and records the half that does not (each side pins its own `now`, so an insert's `NOW()` default and its `now()` formula are one driver round-trip apart), naming #5699 as where sharing one instant would have to be argued. Adds the determinism pins the parameter's appearance stood in for: one snapshot per call across every row x every formula field, asserted by object identity so a per-evaluation clock read fails even when the milliseconds agree, on both the write and read paths, plus a tripwire on the two instants staying independent. Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We Co-authored-by: Claude <noreply@anthropic.com>
1 parent c15fcee commit b9cc17d

3 files changed

Lines changed: 215 additions & 11 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
chore(objectql): retire `applyFormulaPlan`'s zero-caller `nowSnapshot` parameter and narrow its docstring to what actually holds (#5699)
6+
7+
`applyFormulaPlan` declared a fourth optional parameter `nowSnapshot?: Date`
8+
whose only effect was `nowSnapshot ?? new Date()`. Not one of its three call
9+
sites ever passed it — `find`, `findOne`, and the write-response hydration
10+
`hydrateWriteFormulas` added by #5504 — so the parameter went down the
11+
`new Date()` branch from birth. Dormant code, removed rather than archived: a
12+
parameter that looks live is worse than no parameter, because everyone
13+
reasoning from it concludes the caller can pin the instant, and one caller
14+
plainly should have.
15+
16+
No behaviour change (the removed branch was unreachable), no public API change
17+
(`applyFormulaPlan` is module-private and never exported).
18+
19+
The docstring claimed the eval context "mirrors `applyFieldDefaults`". Half of
20+
that was true — the same keys, so `formula` and `defaultValue` expressions share
21+
one vocabulary — and half was not: the two pin their own `now`.
22+
`applyFieldDefaults` is handed the insert's pre-write snapshot, while
23+
`applyFormulaPlan` reads the clock once per call, because a formula is evaluated
24+
when a record is materialized. So inside one `insert` a `NOW()` default and a
25+
`now()` formula observe two instants a driver round-trip apart (sub-millisecond
26+
in practice; across a second/day boundary they can land on different calendar
27+
days). The docstring now says so, and names #5699 as where making them share one
28+
instant would have to be argued — it would hand the write path a determinism
29+
guarantee the read path cannot have, which is a semantic decision, not a cleanup.
30+
31+
Adds the pins that the retired parameter's *appearance* was standing in for
32+
(`engine-write-formula-hydration.test.ts`): one snapshot per call shared by every
33+
row × every formula field, asserted by object identity on the eval context so a
34+
per-evaluation `new Date()` fails even when the milliseconds agree, on the write
35+
path and the read path alike; plus a tripwire that the default's instant and the
36+
formula's instant stay independently sourced.

packages/objectql/src/engine-write-formula-hydration.test.ts

Lines changed: 146 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,58 @@ const PLAIN = {
111111
},
112112
};
113113

114+
/**
115+
* Two `now()` formulas — the determinism surface (#5699).
116+
*
117+
* `applyFormulaPlan` builds ONE eval context per call and reuses it for every
118+
* row × every formula field, so an object declaring two clock formulas is the
119+
* smallest shape that can observe the guarantee in both directions at once.
120+
* After the zero-caller `nowSnapshot` parameter was retired, this per-call
121+
* snapshot is the ONLY thing pinning the function's determinism.
122+
*/
123+
const CLOCK = {
124+
name: 'wf_clock',
125+
label: 'Clock',
126+
fields: {
127+
id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true },
128+
name: { name: 'name', label: 'Name', type: 'text' as const },
129+
seen_at: {
130+
name: 'seen_at', label: 'Seen At', type: 'formula' as const,
131+
expression: { dialect: 'cel', source: 'now()' },
132+
},
133+
seen_again: {
134+
name: 'seen_again', label: 'Seen Again', type: 'formula' as const,
135+
expression: { dialect: 'cel', source: 'now()' },
136+
},
137+
},
138+
};
139+
140+
/**
141+
* A `defaultValue` Expression AND a `formula`, both reading `now()` — the two
142+
* instants #5699 is about.
143+
*
144+
* `created_at` is `readonly`, the shape the ~100 platform `created_at` /
145+
* `updated_at` declarations use; `validateRecord` skips readonly fields, so the
146+
* Date the default resolves to reaches the driver unexamined and the test
147+
* observes the engine's own snapshot rather than a validator's coercion of it.
148+
*/
149+
const STAMPED = {
150+
name: 'wf_stamped',
151+
label: 'Stamped',
152+
fields: {
153+
id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true },
154+
name: { name: 'name', label: 'Name', type: 'text' as const },
155+
created_at: {
156+
name: 'created_at', label: 'Created At', type: 'datetime' as const, readonly: true,
157+
defaultValue: { dialect: 'cel', source: 'now()' },
158+
},
159+
stamped_at: {
160+
name: 'stamped_at', label: 'Stamped At', type: 'formula' as const,
161+
expression: { dialect: 'cel', source: 'now()' },
162+
},
163+
},
164+
};
165+
114166
/** Formula referencing the caller — pins the context passthrough (#1979 status quo). */
115167
const MEMO = {
116168
name: 'wf_memo',
@@ -226,7 +278,7 @@ async function makeEngine() {
226278
const rig = makeStubDriver();
227279
engine.registerDriver(rig.driver as never, true);
228280
await engine.init();
229-
for (const obj of [ACCOUNT, FORECAST, PLAIN, MEMO]) {
281+
for (const obj of [ACCOUNT, FORECAST, PLAIN, MEMO, CLOCK, STAMPED]) {
230282
engine.registry.registerObject(obj as never);
231283
}
232284
const protocol = new ObjectStackProtocolImplementation(engine);
@@ -516,3 +568,96 @@ describe('#5504 — cost threshold: same gate the read path uses', () => {
516568
expect(evaluate).toHaveBeenCalledTimes(3);
517569
});
518570
});
571+
572+
/**
573+
* #5699 — what pins `applyFormulaPlan`'s determinism once its zero-caller
574+
* `nowSnapshot?: Date` parameter is gone.
575+
*
576+
* The parameter was dormant from birth: none of the three call sites (`find`,
577+
* `findOne`, and the write hydration this file was written for) ever passed it,
578+
* so `?? new Date()` was the only branch that ever ran. Retiring it changes no
579+
* behaviour — which is exactly why the guarantee it appeared to provide has to
580+
* be pinned somewhere real. It is the per-call snapshot, and nothing else:
581+
*
582+
* - ONE `new Date()` per call, shared by every row × every formula field, on
583+
* the write path and the read path alike (they are the same helper);
584+
* - and NOT shared with `applyFieldDefaults` — the insert's `defaultValue`
585+
* instant and the response formula's instant stay independent.
586+
*
587+
* The second bullet is the observation #5699 recorded rather than changed, and
588+
* the last test here is its tripwire: making the two share one snapshot would
589+
* hand the write path a determinism guarantee the read path cannot have, so it
590+
* is a semantic decision that belongs in that issue, not in a cleanup.
591+
*/
592+
describe('#5699 — one `now` per `applyFormulaPlan` call', () => {
593+
let rig: Rig;
594+
let evaluate: ReturnType<typeof vi.spyOn>;
595+
beforeEach(async () => {
596+
rig = await makeEngine();
597+
evaluate = vi.spyOn(ExpressionEngine, 'evaluate');
598+
});
599+
afterEach(() => { evaluate.mockRestore(); });
600+
601+
/** The `now` each observed evaluation was handed, in call order. */
602+
const nowsSeen = (): Date[] =>
603+
(evaluate.mock.calls as unknown as Array<[unknown, { now?: Date }]>)
604+
.map(([, ctx]) => ctx.now as Date);
605+
606+
it('a batch insert hydrates every row × every formula field from ONE snapshot', async () => {
607+
const rows = await rig.engine.insert('wf_clock', [{ name: 'a' }, { name: 'b' }]) as Rec[];
608+
609+
// 2 formula fields × 2 rows, and `wf_clock` declares no `defaultValue`
610+
// expression, so every evaluation observed here is the hydration's.
611+
expect(evaluate).toHaveBeenCalledTimes(4);
612+
613+
// The mechanism: one `new Date()`, handed to all four evaluations by
614+
// IDENTITY. A per-evaluation `new Date()` would produce four distinct
615+
// objects even when their milliseconds happen to agree — which is why this
616+
// is asserted on the object and not on the value.
617+
const nows = nowsSeen();
618+
expect(nows).toHaveLength(4);
619+
expect(nows.every((n) => n === nows[0])).toBe(true);
620+
621+
// …and the consequence a caller can see.
622+
const values = [rows[0].seen_at, rows[0].seen_again, rows[1].seen_at, rows[1].seen_again];
623+
expect(values[0]).toBeDefined();
624+
for (const v of values) expect(v).toEqual(values[0]);
625+
});
626+
627+
it('a find hydrates every row × every formula field from the SAME one-snapshot rule', async () => {
628+
// Same helper, so the read path carries the guarantee for the same reason.
629+
// Pinned here next to the write path because the retirement removed the one
630+
// parameter that could ever have made the two differ.
631+
await rig.engine.insert('wf_clock', [{ name: 'a' }, { name: 'b' }]);
632+
evaluate.mockClear();
633+
634+
const found = await rig.engine.find('wf_clock', {} as never) as Rec[];
635+
expect(found).toHaveLength(2);
636+
expect(evaluate).toHaveBeenCalledTimes(4);
637+
638+
const nows = nowsSeen();
639+
expect(nows).toHaveLength(4);
640+
expect(nows.every((n) => n === nows[0])).toBe(true);
641+
});
642+
643+
it("the insert's `defaultValue` instant and the response formula's instant are INDEPENDENT", async () => {
644+
const row = await rig.engine.insert('wf_stamped', { name: 'two clocks' }) as Rec;
645+
expect(row.stamped_at).toBeDefined();
646+
647+
// Exactly two evaluations, and their order is structural rather than
648+
// incidental: `applyFieldDefaults` runs at the top of the insert middleware
649+
// (pre-write, from the insert's own `nowSnap`), `applyFormulaPlan` runs on
650+
// the driver's readback (post-write, from its own clock read).
651+
expect(evaluate).toHaveBeenCalledTimes(2);
652+
const [defaultNow, formulaNow] = nowsSeen();
653+
expect(defaultNow).toBeInstanceOf(Date);
654+
expect(formulaNow).toBeInstanceOf(Date);
655+
656+
// Two `new Date()`s one driver round-trip apart — NOT one shared snapshot.
657+
// Status quo, deliberately: see this block's header. If a later change makes
658+
// them share one, this line goes red and the decision has to be made out
659+
// loud.
660+
expect(formulaNow).not.toBe(defaultNow);
661+
expect(formulaNow.getTime()).toBeGreaterThanOrEqual(defaultNow.getTime());
662+
});
663+
});

packages/objectql/src/engine.ts

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -529,15 +529,39 @@ function planFormulaProjection(
529529
}
530530

531531
/**
532-
* Evaluate read-time formula virtual fields against the raw rows.
532+
* Evaluate formula virtual fields against the raw rows a driver handed back —
533+
* the read path (`find` / `findOne`) and, since #5504, the write path's
534+
* response hydration.
533535
*
534-
* The eval context mirrors `applyFieldDefaults` so formula and default
535-
* expressions see the same shape: a `now` pinned ONCE per operation (every row
536-
* and every formula field in one `find()` observes the same instant —
537-
* determinism, and no per-eval `new Date()` drift), plus `os.user` / `os.org`
538-
* resolved from the execution context (so a computed field can reference the
539-
* caller, e.g. `os.user.id`). Previously this passed only `{ record }`, so
540-
* `now()`/`today()` ran against live wall-clock and user/org were unreachable.
536+
* The eval context is built ONCE per call and reused for every row × every
537+
* formula field, and that is where this function's determinism comes from: one
538+
* `now`, so a `now()`/`today()` formula cannot drift mid-operation, plus
539+
* `os.user` / `os.org` resolved from the execution context (so a computed field
540+
* can reference the caller, e.g. `os.user.id`). Previously this passed only
541+
* `{ record }`, so `now()`/`today()` ran against live wall-clock and user/org
542+
* were unreachable.
543+
*
544+
* That context has the same SHAPE as `applyFieldDefaults`' — the same keys, so
545+
* one expression vocabulary serves `formula` and `defaultValue` alike — but NOT
546+
* the same `now` value, and the two are sourced independently on purpose
547+
* (#5699):
548+
* - `applyFieldDefaults` is handed the insert's `nowSnapshot`, so every
549+
* defaulted field of every row in one write carries the same PRE-write
550+
* instant;
551+
* - this function reads the clock itself, once per call, because a formula is
552+
* evaluated when a record is MATERIALIZED — at read time, and on the write
553+
* response — not at the moment that row's defaults were resolved.
554+
*
555+
* So inside a single `insert` a `NOW()` default and a `now()` formula observe
556+
* two instants one driver round-trip apart (sub-millisecond in practice; across
557+
* a second/day boundary they can land on different calendar days). Making them
558+
* share one instant would hand the write path a determinism guarantee the read
559+
* path cannot have — a semantic decision, not a tidy-up, argued in #5699. Until
560+
* it is decided this function takes NO snapshot parameter: the zero-caller
561+
* `nowSnapshot?: Date` it carried from birth was retired there, in the same
562+
* enforce-or-remove reflex ADR-0049 applies to spec properties, because a
563+
* dormant parameter reads as a live one and anyone reasoning from it concludes
564+
* the two sides already share an instant.
541565
*
542566
* (ADR-0053 Phase 2 will additionally thread `timezone` here once
543567
* `ExecutionContext.timezone` exists — see #1980; this change is independent
@@ -547,10 +571,9 @@ function applyFormulaPlan(
547571
plan: FormulaPlanEntry[],
548572
records: any[],
549573
execCtx?: ExecutionContextInput,
550-
nowSnapshot?: Date,
551574
): void {
552575
if (!plan.length) return;
553-
const now = nowSnapshot ?? new Date();
576+
const now = new Date();
554577
const timezone = execCtx?.timezone;
555578
const user = execCtx?.userId ? { id: String(execCtx.userId), positions: execCtx?.positions ?? [] } : undefined;
556579
const org = execCtx?.tenantId ? { id: String(execCtx.tenantId) } : undefined;

0 commit comments

Comments
 (0)