From c3816ff5484830010b1636486539ae00a355def6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:56:37 +0000 Subject: [PATCH] chore(objectql): retire applyFormulaPlan's zero-caller nowSnapshot parameter (#5699) 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. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- ...ectql-retire-formula-now-snapshot-param.md | 36 +++++ .../engine-write-formula-hydration.test.ts | 147 +++++++++++++++++- packages/objectql/src/engine.ts | 43 +++-- 3 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 .changeset/objectql-retire-formula-now-snapshot-param.md diff --git a/.changeset/objectql-retire-formula-now-snapshot-param.md b/.changeset/objectql-retire-formula-now-snapshot-param.md new file mode 100644 index 0000000000..b06a515e27 --- /dev/null +++ b/.changeset/objectql-retire-formula-now-snapshot-param.md @@ -0,0 +1,36 @@ +--- +"@objectstack/objectql": patch +--- + +chore(objectql): retire `applyFormulaPlan`'s zero-caller `nowSnapshot` parameter and narrow its docstring to what actually holds (#5699) + +`applyFormulaPlan` declared a fourth optional parameter `nowSnapshot?: Date` +whose only effect was `nowSnapshot ?? new Date()`. Not one of its three call +sites ever passed it — `find`, `findOne`, and the write-response hydration +`hydrateWriteFormulas` added by #5504 — so the parameter went down the +`new Date()` branch from birth. Dormant code, removed rather than archived: a +parameter that looks live is worse than no parameter, because everyone +reasoning from it concludes the caller can pin the instant, and one caller +plainly should have. + +No behaviour change (the removed branch was unreachable), no public API change +(`applyFormulaPlan` is module-private and never exported). + +The docstring claimed the eval context "mirrors `applyFieldDefaults`". Half of +that was true — the same keys, so `formula` and `defaultValue` expressions share +one vocabulary — and half was not: the two pin their own `now`. +`applyFieldDefaults` is handed the insert's pre-write snapshot, while +`applyFormulaPlan` reads the clock once per call, because a formula is evaluated +when a record is materialized. So inside one `insert` a `NOW()` default and a +`now()` formula observe two instants a driver round-trip apart (sub-millisecond +in practice; across a second/day boundary they can land on different calendar +days). The docstring now says so, and names #5699 as where making them share one +instant would have to be argued — it would hand the write path a determinism +guarantee the read path cannot have, which is a semantic decision, not a cleanup. + +Adds the pins that the retired parameter's *appearance* was standing in for +(`engine-write-formula-hydration.test.ts`): one snapshot per call shared by every +row × every formula field, asserted by object identity on the eval context so a +per-evaluation `new Date()` fails even when the milliseconds agree, on the write +path and the read path alike; plus a tripwire that the default's instant and the +formula's instant stay independently sourced. diff --git a/packages/objectql/src/engine-write-formula-hydration.test.ts b/packages/objectql/src/engine-write-formula-hydration.test.ts index c430c6077d..5e32a5df9b 100644 --- a/packages/objectql/src/engine-write-formula-hydration.test.ts +++ b/packages/objectql/src/engine-write-formula-hydration.test.ts @@ -111,6 +111,58 @@ const PLAIN = { }, }; +/** + * Two `now()` formulas — the determinism surface (#5699). + * + * `applyFormulaPlan` builds ONE eval context per call and reuses it for every + * row × every formula field, so an object declaring two clock formulas is the + * smallest shape that can observe the guarantee in both directions at once. + * After the zero-caller `nowSnapshot` parameter was retired, this per-call + * snapshot is the ONLY thing pinning the function's determinism. + */ +const CLOCK = { + name: 'wf_clock', + label: 'Clock', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + seen_at: { + name: 'seen_at', label: 'Seen At', type: 'formula' as const, + expression: { dialect: 'cel', source: 'now()' }, + }, + seen_again: { + name: 'seen_again', label: 'Seen Again', type: 'formula' as const, + expression: { dialect: 'cel', source: 'now()' }, + }, + }, +}; + +/** + * A `defaultValue` Expression AND a `formula`, both reading `now()` — the two + * instants #5699 is about. + * + * `created_at` is `readonly`, the shape the ~100 platform `created_at` / + * `updated_at` declarations use; `validateRecord` skips readonly fields, so the + * Date the default resolves to reaches the driver unexamined and the test + * observes the engine's own snapshot rather than a validator's coercion of it. + */ +const STAMPED = { + name: 'wf_stamped', + label: 'Stamped', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + created_at: { + name: 'created_at', label: 'Created At', type: 'datetime' as const, readonly: true, + defaultValue: { dialect: 'cel', source: 'now()' }, + }, + stamped_at: { + name: 'stamped_at', label: 'Stamped At', type: 'formula' as const, + expression: { dialect: 'cel', source: 'now()' }, + }, + }, +}; + /** Formula referencing the caller — pins the context passthrough (#1979 status quo). */ const MEMO = { name: 'wf_memo', @@ -226,7 +278,7 @@ async function makeEngine() { const rig = makeStubDriver(); engine.registerDriver(rig.driver as never, true); await engine.init(); - for (const obj of [ACCOUNT, FORECAST, PLAIN, MEMO]) { + for (const obj of [ACCOUNT, FORECAST, PLAIN, MEMO, CLOCK, STAMPED]) { engine.registry.registerObject(obj as never); } const protocol = new ObjectStackProtocolImplementation(engine); @@ -516,3 +568,96 @@ describe('#5504 — cost threshold: same gate the read path uses', () => { expect(evaluate).toHaveBeenCalledTimes(3); }); }); + +/** + * #5699 — what pins `applyFormulaPlan`'s determinism once its zero-caller + * `nowSnapshot?: Date` parameter is gone. + * + * The parameter was dormant from birth: none of the three call sites (`find`, + * `findOne`, and the write hydration this file was written for) ever passed it, + * so `?? new Date()` was the only branch that ever ran. Retiring it changes no + * behaviour — which is exactly why the guarantee it appeared to provide has to + * be pinned somewhere real. It is the per-call snapshot, and nothing else: + * + * - ONE `new Date()` per call, shared by every row × every formula field, on + * the write path and the read path alike (they are the same helper); + * - and NOT shared with `applyFieldDefaults` — the insert's `defaultValue` + * instant and the response formula's instant stay independent. + * + * The second bullet is the observation #5699 recorded rather than changed, and + * the last test here is its tripwire: making the two share one snapshot would + * hand the write path a determinism guarantee the read path cannot have, so it + * is a semantic decision that belongs in that issue, not in a cleanup. + */ +describe('#5699 — one `now` per `applyFormulaPlan` call', () => { + let rig: Rig; + let evaluate: ReturnType; + beforeEach(async () => { + rig = await makeEngine(); + evaluate = vi.spyOn(ExpressionEngine, 'evaluate'); + }); + afterEach(() => { evaluate.mockRestore(); }); + + /** The `now` each observed evaluation was handed, in call order. */ + const nowsSeen = (): Date[] => + (evaluate.mock.calls as unknown as Array<[unknown, { now?: Date }]>) + .map(([, ctx]) => ctx.now as Date); + + it('a batch insert hydrates every row × every formula field from ONE snapshot', async () => { + const rows = await rig.engine.insert('wf_clock', [{ name: 'a' }, { name: 'b' }]) as Rec[]; + + // 2 formula fields × 2 rows, and `wf_clock` declares no `defaultValue` + // expression, so every evaluation observed here is the hydration's. + expect(evaluate).toHaveBeenCalledTimes(4); + + // The mechanism: one `new Date()`, handed to all four evaluations by + // IDENTITY. A per-evaluation `new Date()` would produce four distinct + // objects even when their milliseconds happen to agree — which is why this + // is asserted on the object and not on the value. + const nows = nowsSeen(); + expect(nows).toHaveLength(4); + expect(nows.every((n) => n === nows[0])).toBe(true); + + // …and the consequence a caller can see. + const values = [rows[0].seen_at, rows[0].seen_again, rows[1].seen_at, rows[1].seen_again]; + expect(values[0]).toBeDefined(); + for (const v of values) expect(v).toEqual(values[0]); + }); + + it('a find hydrates every row × every formula field from the SAME one-snapshot rule', async () => { + // Same helper, so the read path carries the guarantee for the same reason. + // Pinned here next to the write path because the retirement removed the one + // parameter that could ever have made the two differ. + await rig.engine.insert('wf_clock', [{ name: 'a' }, { name: 'b' }]); + evaluate.mockClear(); + + const found = await rig.engine.find('wf_clock', {} as never) as Rec[]; + expect(found).toHaveLength(2); + expect(evaluate).toHaveBeenCalledTimes(4); + + const nows = nowsSeen(); + expect(nows).toHaveLength(4); + expect(nows.every((n) => n === nows[0])).toBe(true); + }); + + it("the insert's `defaultValue` instant and the response formula's instant are INDEPENDENT", async () => { + const row = await rig.engine.insert('wf_stamped', { name: 'two clocks' }) as Rec; + expect(row.stamped_at).toBeDefined(); + + // Exactly two evaluations, and their order is structural rather than + // incidental: `applyFieldDefaults` runs at the top of the insert middleware + // (pre-write, from the insert's own `nowSnap`), `applyFormulaPlan` runs on + // the driver's readback (post-write, from its own clock read). + expect(evaluate).toHaveBeenCalledTimes(2); + const [defaultNow, formulaNow] = nowsSeen(); + expect(defaultNow).toBeInstanceOf(Date); + expect(formulaNow).toBeInstanceOf(Date); + + // Two `new Date()`s one driver round-trip apart — NOT one shared snapshot. + // Status quo, deliberately: see this block's header. If a later change makes + // them share one, this line goes red and the decision has to be made out + // loud. + expect(formulaNow).not.toBe(defaultNow); + expect(formulaNow.getTime()).toBeGreaterThanOrEqual(defaultNow.getTime()); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 18c414b806..7b53d08ed3 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -529,15 +529,39 @@ function planFormulaProjection( } /** - * Evaluate read-time formula virtual fields against the raw rows. + * Evaluate formula virtual fields against the raw rows a driver handed back — + * the read path (`find` / `findOne`) and, since #5504, the write path's + * response hydration. * - * The eval context mirrors `applyFieldDefaults` so formula and default - * expressions see the same shape: a `now` pinned ONCE per operation (every row - * and every formula field in one `find()` observes the same instant — - * determinism, and no per-eval `new Date()` drift), plus `os.user` / `os.org` - * resolved from the execution context (so a computed field can reference the - * caller, e.g. `os.user.id`). Previously this passed only `{ record }`, so - * `now()`/`today()` ran against live wall-clock and user/org were unreachable. + * The eval context is built ONCE per call and reused for every row × every + * formula field, and that is where this function's determinism comes from: one + * `now`, so a `now()`/`today()` formula cannot drift mid-operation, plus + * `os.user` / `os.org` resolved from the execution context (so a computed field + * can reference the caller, e.g. `os.user.id`). Previously this passed only + * `{ record }`, so `now()`/`today()` ran against live wall-clock and user/org + * were unreachable. + * + * That context has the same SHAPE as `applyFieldDefaults`' — the same keys, so + * one expression vocabulary serves `formula` and `defaultValue` alike — but NOT + * the same `now` value, and the two are sourced independently on purpose + * (#5699): + * - `applyFieldDefaults` is handed the insert's `nowSnapshot`, so every + * defaulted field of every row in one write carries the same PRE-write + * instant; + * - this function reads the clock itself, once per call, because a formula is + * evaluated when a record is MATERIALIZED — at read time, and on the write + * response — not at the moment that row's defaults were resolved. + * + * So inside a single `insert` a `NOW()` default and a `now()` formula observe + * two instants one driver round-trip apart (sub-millisecond in practice; across + * a second/day boundary they can land on different calendar days). Making them + * share one instant would hand the write path a determinism guarantee the read + * path cannot have — a semantic decision, not a tidy-up, argued in #5699. Until + * it is decided this function takes NO snapshot parameter: the zero-caller + * `nowSnapshot?: Date` it carried from birth was retired there, in the same + * enforce-or-remove reflex ADR-0049 applies to spec properties, because a + * dormant parameter reads as a live one and anyone reasoning from it concludes + * the two sides already share an instant. * * (ADR-0053 Phase 2 will additionally thread `timezone` here once * `ExecutionContext.timezone` exists — see #1980; this change is independent @@ -547,10 +571,9 @@ function applyFormulaPlan( plan: FormulaPlanEntry[], records: any[], execCtx?: ExecutionContextInput, - nowSnapshot?: Date, ): void { if (!plan.length) return; - const now = nowSnapshot ?? new Date(); + const now = new Date(); const timezone = execCtx?.timezone; const user = execCtx?.userId ? { id: String(execCtx.userId), positions: execCtx?.positions ?? [] } : undefined; const org = execCtx?.tenantId ? { id: String(execCtx.tenantId) } : undefined;