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;