From 1d0120e3c0852c34bb3da11a46706c9da6652671 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 00:33:38 +0000 Subject: [PATCH] fix(objectql): hydrate formula fields on the write response, not only on reads (#5504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applyFormulaPlan` had exactly two call sites — the `find` result and the `findOne` result — so `POST /data/:object` and `PATCH /data/:object/:id` answered with the stored document, in which a `formula` field is not `null` but ABSENT (formulas are virtual; no driver returns a column for one). The next `GET` of the same row carried every one of them: read-your-write broken in the direction hardest to notice, since the response calls itself `record` and consumers render it directly. Every object whose `nameField` points at a formula rendered blank until a second round-trip. `engine.insert` and `engine.update` now hydrate through one shared helper that reuses the read path's plan builder and evaluation — same formula semantic on both verbs, no write-path dialect. It evaluates over the row the driver already returned (`create` uses `RETURNING *`, `update` re-reads), so there is no extra round-trip and no formula sees a partial record. Coverage falls out of the placement rather than being enumerated per call site: single insert, batch insert, insertMany / createManyData / insertManyData and single-id update all pass through one hydration point per verb. A predicate (`multi`) update is unchanged — `driver.updateMany` resolves to an affected-row count and names no row. Ordering is pinned on both sides: after the same-day write-path strips and refusals (#5503 runtime-owned autonumber, #2948 readonly, #5126 strictReadonlyWrites), and before the afterInsert/afterUpdate dispatch, which mirrors the read path's applyFormulaPlan → afterFind order. `packages/rest` needed no production change; its end-to-end test proves the handlers pass the hydrated record through, so a future reshaping of the write response fails there instead of silently reopening this. Also corrects two comments in other packages that asserted the now-inverted fact ("the write result does not carry formula fields"): plugin-audit's computed-field exclusion is keyed on the field TYPE and is unaffected, and trigger-record-change's re-read still earns its keep for summary/rollup. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx --- .../write-response-formula-hydration.md | 49 ++ .../engine-write-formula-hydration.test.ts | 517 ++++++++++++++++++ packages/objectql/src/engine.ts | 86 +++ .../plugin-audit/src/audit-writers.test.ts | 11 +- .../plugins/plugin-audit/src/audit-writers.ts | 20 +- .../src/rest-write-response-formula.test.ts | 214 ++++++++ .../src/formula-context.test.ts | 16 +- 7 files changed, 897 insertions(+), 16 deletions(-) create mode 100644 .changeset/write-response-formula-hydration.md create mode 100644 packages/objectql/src/engine-write-formula-hydration.test.ts create mode 100644 packages/rest/src/rest-write-response-formula.test.ts diff --git a/.changeset/write-response-formula-hydration.md b/.changeset/write-response-formula-hydration.md new file mode 100644 index 0000000000..118f3e1d69 --- /dev/null +++ b/.changeset/write-response-formula-hydration.md @@ -0,0 +1,49 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a write response is a record, so it carries the record's `formula` fields (#5504) + +`POST /data/:object` and `PATCH /data/:object/:id` answered with the stored +document, in which a `formula` field is not `null` but **absent** — formulas are +virtual, so no driver ever returns a column for one. The very next `GET` of the +same row carried every one of them. Read-your-write was broken in the direction +that is hardest to notice: the response calls itself `record`, so consumers +render it directly, and every object whose `nameField` points at a formula +rendered blank until a second round-trip. + +Cause: `applyFormulaPlan` had exactly two call sites — the `find` result and the +`findOne` result. The write paths returned the driver's row untouched, and the +REST layer passed it straight through. + +**What changed.** `engine.insert` and `engine.update` now hydrate formula +virtuals onto what they hand back, using the *same* plan builder and the *same* +evaluation the read path uses — one formula semantic, not a write-path dialect. +Evaluation runs against the row the driver already returned (`create` uses +`RETURNING *`, `update` re-reads), so there is no extra round-trip and no +formula sees a partial record. Execution context is threaded exactly as `find` +threads it today. + +Covered by construction, not per call site: single insert, batch insert, +`insertMany` / `createManyData` / `insertManyData`, and single-id update all +flow through the one hydration point on each verb. A **predicate** (`multi`) +update is unchanged — `driver.updateMany` resolves to an affected-row COUNT and +names no row, so it has no record to materialize. + +Ordering, both halves pinned by tests: + +- **after** the write-path strips and refusals — a formula reading a stripped + `autonumber` (#5503) or `readonly` (#2948) field reports what was STORED, and + a write refused under `strictReadonlyWrites` (#5126) produces no response to + hydrate at all; +- **before** the `afterInsert` / `afterUpdate` dispatch, mirroring the read + path's `applyFormulaPlan` → `afterFind` order, so an after-hook observes the + same complete record on a write that it observes on a read. + +Cost is gated exactly as on the read path: an object that declares no formula +builds an empty plan and evaluates nothing. + +No API or schema change — a response that was missing keys now has them. +Consumers that worked around this with an extra `GET` after every write can drop +it; consumers that treated the absent key as "field not configured" should note +that an unevaluable formula is reported as `null`, as it always has been on read. diff --git a/packages/objectql/src/engine-write-formula-hydration.test.ts b/packages/objectql/src/engine-write-formula-hydration.test.ts new file mode 100644 index 0000000000..537a453465 --- /dev/null +++ b/packages/objectql/src/engine-write-formula-hydration.test.ts @@ -0,0 +1,517 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5504 — a write response is a RECORD, so it carries the record's `formula` + * fields. + * + * `applyFormulaPlan` used to hang off `find` and `findOne` only. `POST + * /data/:object` and `PATCH /data/:object/:id` therefore answered with the + * stored document, in which a formula field is not `null` but ABSENT (a + * formula is virtual — no driver returns a column for one), while the very + * next `GET` of the same row carried every one of them. Read-your-write was + * broken in the direction that is hardest to notice: the response calls itself + * `record`, so a client renders it, and every object whose `nameField` points + * at a formula rendered blank. + * + * These tests drive the real engine through the protocol surface the REST + * layer calls (`createData` / `updateData` / `createManyData` / + * `insertManyData`), because that — not a unit call on a helper — is the shape + * the issue reported. + * + * Two orderings are pinned deliberately, because both were constraints on + * WHERE the hydration could go, not incidental outcomes: + * + * - hydration runs AFTER the write-path strips and refusals landed the same + * day (#5503's runtime-owned `autonumber` strip, #5126/#5610's + * `strictReadonlyWrites`). A formula reading a stripped field must report + * what was STORED, and a refused write must produce no response to hydrate + * at all; + * - hydration runs BEFORE the afterInsert/afterUpdate dispatch, mirroring the + * read path's `applyFormulaPlan` → `afterFind` order, so an after-hook sees + * the same complete record on a write that it sees on a read. + */ + +import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ExpressionEngine } from '@objectstack/formula'; +import { ObjectQL } from './engine.js'; + +/** + * An account whose display title is a formula — the HotCRM shape from the + * issue (`nameField` pointing at a formula on account / product / case / + * campaign / quote / forecast / lead / contact). + * + * `account_number` is an `autonumber` so the formula reads a RUNTIME-owned + * value: that is what makes the #5503 interaction observable rather than + * asserted about internals. + */ +const ACCOUNT = { + name: 'wf_account', + label: 'Account', + nameField: 'display_title', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, + segment: { name: 'segment', label: 'Segment', type: 'text' as const }, + account_number: { + name: 'account_number', + label: 'Account No.', + type: 'autonumber' as const, + autonumberFormat: 'ACC-{0000}', + }, + // The locked column a non-system caller may not write (#2948) — a formula + // reads it so the strip's effect is visible in the response. + tier: { name: 'tier', label: 'Tier', type: 'text' as const, readonly: true, defaultValue: 'standard' }, + display_title: { + name: 'display_title', + label: 'Display Title', + type: 'formula' as const, + expression: { dialect: 'cel', source: 'record.account_number + " - " + record.name' }, + }, + tier_label: { + name: 'tier_label', + label: 'Tier Label', + type: 'formula' as const, + expression: { dialect: 'cel', source: '"tier:" + record.tier' }, + }, + }, +}; + +/** Multi-formula object — the `crm_forecast` shape from the issue. */ +const FORECAST = { + name: 'wf_forecast', + label: 'Forecast', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + probability: { name: 'probability', label: 'Probability', type: 'number' as const }, + quota: { name: 'quota', label: 'Quota', type: 'number' as const }, + expected_amount: { + name: 'expected_amount', label: 'Expected', type: 'formula' as const, + expression: { dialect: 'cel', source: 'record.amount * record.probability' }, + }, + attainment_pct: { + name: 'attainment_pct', label: 'Attainment', type: 'formula' as const, + expression: { dialect: 'cel', source: 'record.amount / record.quota' }, + }, + coverage_ratio: { + name: 'coverage_ratio', label: 'Coverage', type: 'formula' as const, + expression: { dialect: 'cel', source: 'record.quota / record.amount' }, + }, + }, +}; + +/** No formula at all — the perf threshold's control object. */ +const PLAIN = { + name: 'wf_plain', + label: 'Plain', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +/** Formula referencing the caller — pins the context passthrough (#1979 status quo). */ +const MEMO = { + name: 'wf_memo', + label: 'Memo', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + body: { name: 'body', label: 'Body', type: 'text' as const }, + author_ref: { + name: 'author_ref', label: 'Author', type: 'formula' as const, + expression: { dialect: 'cel', source: 'os.user.id' }, + }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: unknown): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where as Record)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as Record)) + ? (v as Record).$eq + : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + const driver = { + name: 'memory', + version: '0.0.0', + supports: {}, + async connect() {}, + async disconnect() {}, + async checkHealth() { return true; }, + async execute() { return null; }, + // Shallow COPIES, never live references into the backing table — the + // contract `driver-memory` states in as many words, and the SQL driver gets + // for free from knex. It is load-bearing for this file specifically: the + // READ path hydrates formulas by mutating the rows a driver hands back, so + // a harness leaking references would write `display_title` into its own + // store on the first GET and every later write response would echo it — + // these tests would then pass with the write-path hydration deleted, which + // is the "green because nothing is produced" failure mode, inverted. + async find(object: string, ast: { where?: unknown }) { + return Array.from(storeFor(object).values()) + .filter((r) => matchesWhere(r, ast?.where)) + .map((r) => ({ ...r })); + }, + async findOne(object: string, ast: { where?: unknown }) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return { ...r }; + return null; + }, + // `RETURNING *` semantics, like the SQL driver: a write hands back the + // whole stored row, which is what makes evaluating the formulas over it + // equivalent to evaluating them over a readback. + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return { ...row }; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return { ...updated }; + }, + async updateMany(object: string, ast: { where?: unknown }, data: Record) { + const s = storeFor(object); + let n = 0; + for (const [id, row] of s) { + if (!matchesWhere(row, ast?.where)) continue; + s.set(id, { ...row, ...data, id }); + n += 1; + } + return n; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: { where?: unknown }) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + const out: Record[] = []; + for (const r of rows) out.push(await this.create(object, r)); + return out; + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, + async rollback() {}, + }; + return { driver, stores }; +} + +async function makeEngine() { + const engine = new ObjectQL(); + const rig = makeMemoryDriver(); + engine.registerDriver(rig.driver as never, true); + await engine.init(); + for (const obj of [ACCOUNT, FORECAST, PLAIN, MEMO]) { + engine.registry.registerObject(obj as never); + } + const protocol = new ObjectStackProtocolImplementation(engine); + return { engine, protocol, ...rig }; +} + +type Rig = Awaited>; +type Rec = Record; + +describe('#5504 — CREATE response carries formula fields', () => { + let rig: Rig; + beforeEach(async () => { rig = await makeEngine(); }); + + it('the create response record is EQUIVALENT to the GET that follows it', async () => { + // The issue's exact repro, minus HTTP: + // POST /data/wf_account {"name":"PatchEcho"} → record.display_title + // GET /data/wf_account/ → display_title + const created = await rig.protocol.createData({ + object: 'wf_account', + data: { name: 'PatchEcho' }, + }); + const record = created.record as Rec; + + // The key EXISTS (the failure was an absent key, not a null value)… + expect('display_title' in record).toBe(true); + expect(record.display_title).toBe('ACC-0001 - PatchEcho'); + + // …and it is what the next read reports, field for field. + const fetched = await rig.protocol.getData({ object: 'wf_account', id: String(created.id) }); + expect((fetched.record as Rec).display_title).toBe(record.display_title); + expect((fetched.record as Rec).tier_label).toBe(record.tier_label); + }); + + it("a nameField pointing at a formula renders straight from the create response", async () => { + // The concrete consequence the issue reported: HotCRM's account / product / + // case / campaign / quote / forecast / lead / contact all name a formula, + // so "create then show the title" used to render blank. + const created = await rig.protocol.createData({ + object: 'wf_account', + data: { name: 'Northwind' }, + }); + const titleField = ACCOUNT.nameField; + const title = (created.record as Rec)[titleField]; + expect(typeof title).toBe('string'); + expect(title).toBe('ACC-0001 - Northwind'); + }); + + it('evaluates EVERY formula on a multi-formula object', async () => { + // `crm_forecast` in the issue: expected_amount / attainment_pct / + // coverage_ratio were all missing from the POST response together. + const created = await rig.protocol.createData({ + object: 'wf_forecast', + data: { amount: 200, probability: 0.5, quota: 400 }, + }); + const record = created.record as Rec; + expect(record.expected_amount).toBe(100); + expect(record.attainment_pct).toBe(0.5); + expect(record.coverage_ratio).toBe(2); + }); + + it('threads the execution context exactly as the read path does', async () => { + // Same passthrough find/findOne have today — `os.user` resolved from the + // execution context. Widening what the context carries is #1979 and is + // deliberately NOT touched here. + const created = await rig.protocol.createData({ + object: 'wf_memo', + data: { body: 'hello' }, + context: { userId: 'u-42' }, + }); + expect((created.record as Rec).author_ref).toBe('u-42'); + + const fetched = await rig.protocol.getData({ + object: 'wf_memo', + id: String(created.id), + context: { userId: 'u-42' }, + }); + expect((fetched.record as Rec).author_ref).toBe('u-42'); + }); + + it('a formula that faults resolves to null, not to a missing key', async () => { + // Same contract `applyFormulaPlan` gives the read path: an unevaluable + // expression is `null`. The point of the fix is that the KEY is present + // either way, so a consumer can tell "not evaluated" from "not configured". + const created = await rig.protocol.createData({ + object: 'wf_forecast', + data: { amount: 10 }, // no `quota` → division by an absent field + }); + const record = created.record as Rec; + expect('attainment_pct' in record).toBe(true); + expect(record.attainment_pct).toBeNull(); + }); +}); + +describe('#5504 — UPDATE response carries formula fields', () => { + let rig: Rig; + beforeEach(async () => { rig = await makeEngine(); }); + + it('the PATCH response record is EQUIVALENT to the GET that follows it', async () => { + const created = await rig.protocol.createData({ object: 'wf_account', data: { name: 'PatchEcho' } }); + const id = String(created.id); + + const updated = await rig.protocol.updateData({ + object: 'wf_account', + id, + data: { segment: 'growth' }, + }); + const record = updated.record as Rec; + expect('display_title' in record).toBe(true); + + const fetched = await rig.protocol.getData({ object: 'wf_account', id }); + expect((fetched.record as Rec).display_title).toBe(record.display_title); + }); + + it('recomputes from the POST-write values, not the pre-write ones', async () => { + const created = await rig.protocol.createData({ object: 'wf_account', data: { name: 'Before' } }); + const id = String(created.id); + expect((created.record as Rec).display_title).toBe('ACC-0001 - Before'); + + const updated = await rig.protocol.updateData({ + object: 'wf_account', + id, + data: { name: 'After' }, + }); + // Hydration sits on the driver's post-write readback, so the formula + // reflects the write that just happened. + expect((updated.record as Rec).display_title).toBe('ACC-0001 - After'); + }); + + it('a PREDICATE (multi) update keeps its affected-COUNT contract — nothing to hydrate', async () => { + // `driver.updateMany` returns a count and names no row (#4639), so a bulk + // update has no record to materialize. Pinned so a future reader does not + // read the absence as a missed call site: giving a bulk update a record + // response would be a contract change, not a hydration gap. + await rig.protocol.createData({ object: 'wf_account', data: { name: 'A', segment: 'smb' } }); + await rig.protocol.createData({ object: 'wf_account', data: { name: 'B', segment: 'smb' } }); + + const affected = await rig.engine.update( + 'wf_account', + { segment: 'growth' }, + { where: { segment: 'smb' }, multi: true }, + ); + expect(affected).toBe(2); + }); +}); + +describe('#5504 — batch write responses', () => { + let rig: Rig; + beforeEach(async () => { rig = await makeEngine(); }); + + it('every row of a batch insert is hydrated', async () => { + const rows = await rig.engine.insert('wf_forecast', [ + { amount: 100, probability: 0.5, quota: 200 }, + { amount: 300, probability: 0.25, quota: 600 }, + ]) as Rec[]; + expect(rows.map((r) => r.expected_amount)).toEqual([50, 75]); + expect(rows.map((r) => r.attainment_pct)).toEqual([0.5, 0.5]); + }); + + it('`createManyData` returns hydrated records', async () => { + const res = await rig.protocol.createManyData({ + object: 'wf_account', + records: [{ name: 'Alpha' }, { name: 'Beta' }], + }); + const records = res.records as Rec[]; + expect(records.map((r) => r.display_title)).toEqual([ + 'ACC-0001 - Alpha', + 'ACC-0002 - Beta', + ]); + }); + + it('`insertManyData` hydrates the surviving rows and fabricates nothing for the dead ones', async () => { + // Partial-success path (framework#3172): a culled row carries an error and + // no `record`, so there must be nothing to hydrate for it either. + const res = await rig.protocol.insertManyData({ + object: 'wf_account', + records: [{ name: 'Good' }, { segment: 'no-name' }], + }); + const [ok, dead] = res.outcomes; + expect(ok.ok).toBe(true); + expect((ok.record as Rec).display_title).toBe('ACC-0001 - Good'); + expect(dead.ok).toBe(false); + expect(dead.record).toBeUndefined(); + }); +}); + +describe('#5504 — ordering against the same-day write-path strips', () => { + let rig: Rig; + beforeEach(async () => { rig = await makeEngine(); }); + + it('a formula over a STRIPPED autonumber reports the issued value, not the forged one (#5503)', async () => { + // Direction of the proof: hydration runs AFTER `stripRuntimeOwnedFields`. + // Were it before (or over the caller's payload rather than the driver's + // readback), `display_title` would echo the forged 'ACC-777777'. + const created = await rig.protocol.createData({ + object: 'wf_account', + data: { name: 'Forger', account_number: 'ACC-777777' }, + }); + const record = created.record as Rec; + expect(record.account_number).toBe('ACC-0001'); + expect(record.display_title).toBe('ACC-0001 - Forger'); + expect(String(record.display_title)).not.toContain('777777'); + }); + + it('a formula over a STRIPPED readonly write reports the stored value (#2948)', async () => { + const created = await rig.protocol.createData({ object: 'wf_account', data: { name: 'Locked' } }); + const id = String(created.id); + // `tier` is `readonly`; the caller's write is dropped and the update + // commits without it, so the formula must show what is STORED. + const updated = await rig.protocol.updateData({ + object: 'wf_account', + id, + data: { segment: 'growth', tier: 'platinum' }, + }); + expect((updated.record as Rec).tier).toBe('standard'); + expect((updated.record as Rec).tier_label).toBe('tier:standard'); + }); + + it('a REFUSED strict write produces no response at all — nothing is hydrated (#5126/#5610)', async () => { + const created = await rig.protocol.createData({ object: 'wf_account', data: { name: 'Strict' } }); + const id = String(created.id); + await expect( + rig.engine.update( + 'wf_account', + { id, tier: 'platinum' }, + { strictReadonlyWrites: true }, + ), + ).rejects.toThrow(/read-only|readonly/i); + // …and the refusal left the row untouched, formula included. + const fetched = await rig.protocol.getData({ object: 'wf_account', id }); + expect((fetched.record as Rec).tier_label).toBe('tier:standard'); + }); +}); + +describe('#5504 — the after-hook view matches the read path', () => { + let rig: Rig; + beforeEach(async () => { rig = await makeEngine(); }); + + it('an afterInsert hook observes the same complete record `afterFind` gets', async () => { + // Hydration is placed BEFORE the dispatch on purpose — the read path runs + // `applyFormulaPlan` before `afterFind`, and a hook should not have to know + // which verb materialized the record it is handed. + const seen: unknown[] = []; + rig.engine.registerHook('afterInsert', (ctx) => { + seen.push((ctx.result as Rec)?.display_title); + }, { object: 'wf_account' }); + + await rig.protocol.createData({ object: 'wf_account', data: { name: 'Hooked' } }); + expect(seen).toEqual(['ACC-0001 - Hooked']); + }); + + it('an afterUpdate hook observes it too', async () => { + const created = await rig.protocol.createData({ object: 'wf_account', data: { name: 'Hooked' } }); + const seen: unknown[] = []; + rig.engine.registerHook('afterUpdate', (ctx) => { + seen.push((ctx.result as Rec)?.display_title); + }, { object: 'wf_account' }); + + await rig.protocol.updateData({ + object: 'wf_account', + id: String(created.id), + data: { name: 'Rehooked' }, + }); + expect(seen).toEqual(['ACC-0001 - Rehooked']); + }); +}); + +describe('#5504 — cost threshold: same gate the read path uses', () => { + let rig: Rig; + let evaluate: ReturnType; + beforeEach(async () => { + rig = await makeEngine(); + evaluate = vi.spyOn(ExpressionEngine, 'evaluate'); + }); + afterEach(() => { evaluate.mockRestore(); }); + + it('an object declaring NO formula evaluates nothing on write', async () => { + const created = await rig.protocol.createData({ object: 'wf_plain', data: { name: 'nothing derived' } }); + expect(evaluate).not.toHaveBeenCalled(); + // …and the response carries exactly the stored keys — no phantom columns. + expect(Object.keys(created.record as Rec).sort()).toEqual(['id', 'name']); + }); + + it('an object declaring formulas evaluates once per formula field per row', async () => { + await rig.protocol.createData({ object: 'wf_forecast', data: { amount: 2, probability: 1, quota: 4 } }); + // 3 formula fields × 1 row. No `defaultValue` expression on this object, so + // every evaluation observed here is the hydration's. + expect(evaluate).toHaveBeenCalledTimes(3); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 173b3e7c46..62930dd67a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -557,6 +557,57 @@ function applyFormulaPlan( } } +/** + * Hydrate `formula` virtual fields onto the records a WRITE hands back (#5504). + * + * `applyFormulaPlan` used to hang off the read path only — `find` and `findOne` + * — so `POST /data/:object` and `PATCH /data/:object/:id` answered with the + * stored document, in which a formula field is not merely `null` but ABSENT + * (formulas are virtual: no driver ever returns a column for one). The very + * next `GET` of that same row carried every one of them, so a caller that + * rendered the create response — the natural thing to do, the response calls + * itself `record` — got a blank title on every object whose `nameField` points + * at a formula, and could not tell "not configured" from "not evaluated". + * Read-your-write, restored: the write response is now the same materialization + * a read produces. + * + * Deliberately the SAME plan builder and the SAME evaluation the read path uses + * — one formula semantic, not a write-path dialect: + * - `planFormulaProjection(schema, undefined)` is exactly find's no-projection + * branch: every formula field the schema declares, and no `projected` + * rewrite (a write returns whole rows, so there is nothing to project). + * It also carries the perf threshold unchanged — an object declaring no + * formula yields an empty plan and `applyFormulaPlan` returns at its first + * line, so a write on an ordinary object pays a field-name loop and nothing + * else. + * - the execution context is threaded exactly as find threads it, so `os.user` + * / `os.org` resolve identically on both sides. Widening what that context + * carries is #1979's work and stays out of here. + * + * Evaluates against the record the driver returned (a full row: `create` uses + * `RETURNING *`, `update` re-reads), so no extra round-trip is needed and no + * formula sees a partial record. Mutates in place, like the read path. + * + * Non-record entries (a `null` readback when the write moved the row out of the + * caller's scope; the affected-row COUNT a predicate update resolves to) are + * skipped rather than special-cased at each call site — the primitive would + * otherwise take a property assignment, which throws under ES module strict + * mode. + */ +function hydrateWriteFormulas( + schema: any, + results: unknown[], + execCtx?: ExecutionContextInput, +): void { + const records = results.filter( + (r): r is Record => r != null && typeof r === 'object', + ); + if (records.length === 0) return; + const { plan } = planFormulaProjection(schema, undefined); + if (plan.length === 0) return; + applyFormulaPlan(plan, records, execCtx); +} + export type HookHandler = (context: HookContext) => Promise | void; /** @@ -5108,6 +5159,24 @@ export class ObjectQL implements IObjectQLEngine { // Only live rows have results — dead (partial-mode) rows never reach // afterInsert. const resultRows: any[] = isBatch ? (Array.isArray(result) ? result : [result]) : [result]; + // [#5504] Evaluate `formula` virtual fields onto what this write hands + // back, so a create response is the same materialization the following + // GET produces. Placed HERE for three reasons, all load-bearing: + // - AFTER every strip / refusal above (#5503's runtime-owned strip, + // #5126's `strictReadonlyWrites` throw): a payload the engine + // refuses never reaches a driver, so there is nothing to hydrate, + // and a stripped field must not reappear via a formula that read it. + // - AFTER the bulk contract guard, so `resultRows` is already known to + // be one row per live input row — no `undefined` slot to evaluate a + // formula against. + // - BEFORE the afterInsert dispatch, mirroring the read path's + // `applyFormulaPlan` → `afterFind` order. An after-hook therefore + // observes the same complete record on a write as it does on a read, + // and — because `coerceBooleanFields` copies the row it is handed — + // the caller-facing `rowCtx.result` carries the values too. + // Batch (`insertMany` / `createManyData`) is covered by construction: + // one hydration pass over every returned row, not one per call site. + hydrateWriteFormulas(schemaForValidation, resultRows, opCtx.context); for (let k = 0; k < liveIndexes.length; k++) { const rowCtx = rowHookContexts[liveIndexes[k]]; rowCtx.event = 'afterInsert'; @@ -5557,6 +5626,23 @@ export class ObjectQL implements IObjectQLEngine { } hookContext.event = 'afterUpdate'; + // [#5504] Same formula hydration the insert path runs, on the same + // terms: after both strip passes and `assertNoStrictDrops()` (which + // throw before any driver call), before the afterUpdate dispatch. + // + // Only the BY-ID branch has records to hydrate. A predicate write + // resolves to the affected-row COUNT `driver.updateMany` returns + // (#4639) — it names no row and returns none, so there is nothing to + // materialize and `isPredicateWrite` says so explicitly rather than + // letting a `typeof` sniff decide. Giving a bulk update a record + // response is a contract change, not a hydration gap. + if (!isPredicateWrite) { + hydrateWriteFormulas( + updateSchema, + Array.isArray(result) ? result : [result], + opCtx.context, + ); + } // Coerce boolean fields (SQLite 0/1 → JS bool) on the after-hook view // of both the new row and the prior row, so flow conditions comparing // `record.is_escalated`/`previous.status` against booleans behave. diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index 8cbeb55859..8e23905f29 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -778,10 +778,13 @@ describe('audit writers — enable.files server-side enforcement (#2727)', () => }); describe('audit writers — update diff hygiene (objectui detail-history report)', () => { - // gantt_plan-shaped object: a formula helper alongside real fields. The - // `before` snapshot is read through the query path (formula computed), but - // `after` (ctx.result) is the raw write result (formula absent) — the diff - // must not record that asymmetry as a change. + // gantt_plan-shaped object: a formula helper alongside real fields. The diff + // must not record a computed field as a change, whichever way the two sides + // disagree. The contexts below keep the ORIGINAL asymmetry (`before` from the + // query path carries the formula, `after` does not) because that is the shape + // the objectui History tab reported — #5504 later made the real write path + // hydrate `after` as well, and the exclusion is keyed on the field TYPE, so it + // holds in both worlds. const SCHEMA = { sys_audit_log: SINGLE_TENANT.sys_audit_log, sys_activity: SINGLE_TENANT.sys_activity, diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index b07b482604..d2264aeef2 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -177,13 +177,19 @@ function recordLabel(record: any, id: string): string { } /** - * Field types whose values the engine computes on READ (formula / summary / - * rollup / autonumber). The `before` snapshot is read back through the query - * path and therefore carries them, but `after` (`ctx.result`) is the raw - * write result and does not — so diffing them records a phantom - * "value → null" change on EVERY update (surfaced by the objectui record - * History tab). As derived values their changes are implied by their source - * fields anyway, so they are excluded from the audit diff. + * Field types whose values the engine computes rather than storing (formula / + * summary / rollup / autonumber). Excluded from the audit diff because a + * DERIVED value's change is already implied by the source fields that produced + * it — recording both says the same thing twice. + * + * The original symptom was narrower: `before` came back through the query path + * (computed) while `after` (`ctx.result`) was the raw write result (formula + * key ABSENT), so every update recorded a phantom "value → null" change on the + * objectui record History tab. Since #5504 the write path hydrates formulas + * too, so that particular asymmetry is gone and the two sides now agree. The + * exclusion is kept on its own merit — the derived-is-implied reason above — + * and it is keyed on the field TYPE from `fieldDefs`, never on a key being + * missing, which is why the fix one layer down did not disturb it. */ const COMPUTED_FIELD_TYPES = new Set(['formula', 'summary', 'rollup', 'autonumber', 'auto_number']); diff --git a/packages/rest/src/rest-write-response-formula.test.ts b/packages/rest/src/rest-write-response-formula.test.ts new file mode 100644 index 0000000000..1838c00fa8 --- /dev/null +++ b/packages/rest/src/rest-write-response-formula.test.ts @@ -0,0 +1,214 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5504] The issue's repro, walked over the REST routes a client actually +// calls: +// +// POST /api/v1/data/:object → 201, record. was ABSENT +// GET /api/v1/data/:object/:id → the same formula had a value +// PATCH /api/v1/data/:object/:id → 200, still absent +// +// The fix is entirely in `packages/objectql` (`engine.insert` / `engine.update` +// now hydrate formula virtual fields onto what they return, the way `find` / +// `findOne` always have). The REST layer needed NO production change — its +// POST/PATCH handlers pass the protocol result through verbatim. +// +// That is exactly why this file exists. "No change needed here" is a claim +// about a layer, and the only honest way to make it is to drive the layer: +// nothing hand-built below, a REAL `ObjectQL` + a REAL +// `ObjectStackProtocolImplementation` + the registered routes, so a future +// reshaping of the write response (a projection, a whitelist, a serializer) +// that drops virtual fields on the way out fails HERE rather than silently +// re-opening the issue one layer above the tests that cover it. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +const DATA_COLLECTION = '/api/v1/data/:object'; +const DATA_ITEM = '/api/v1/data/:object/:id'; + +/** The HotCRM shape from the issue: `nameField` points at a formula. */ +const ACCOUNT = { + name: 'rf_account', + label: 'Account', + nameField: 'display_title', + fields: { + id: { name: 'id', label: 'ID', type: 'text', primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text', required: true }, + segment: { name: 'segment', label: 'Segment', type: 'text' }, + display_title: { + name: 'display_title', + label: 'Display Title', + type: 'formula', + expression: { dialect: 'cel', source: '"ACC - " + record.name' }, + }, + }, +}; + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: Record = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: unknown) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +/** In-memory driver with `RETURNING *` write semantics and copy-on-read. */ +function memoryDriver() { + const rows = new Map>>(); + const table = (o: string) => { + let t = rows.get(o); + if (!t) { t = new Map(); rows.set(o, t); } + return t; + }; + let seq = 0; + const matches = (row: Record, where: unknown) => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where as Record)) { + if (k.startsWith('$')) continue; + const want = (v && typeof v === 'object' && '$eq' in (v as Record)) + ? (v as Record).$eq + : v; + if ((row[k] ?? null) !== (want ?? null)) return false; + } + return true; + }; + const driver = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + // Copies, never live references — `driver-memory`'s stated contract. + async find(object: string, ast: { where?: unknown }) { + return Array.from(table(object).values()).filter((r) => matches(r, ast?.where)).map((r) => ({ ...r })); + }, + async findOne(object: string, ast: { where?: unknown }) { + for (const r of table(object).values()) if (matches(r, ast?.where)) return { ...r }; + return null; + }, + async create(object: string, data: Record) { + seq += 1; + const id = (data.id as string) ?? `r_${seq}`; + const row = { ...data, id }; + table(object).set(id, row); + return { ...row }; + }, + async update(object: string, id: string, data: Record) { + const t = table(object); + const cur = t.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + t.set(id, next); + return { ...next }; + }, + async delete(object: string, id: string) { return table(object).delete(id); }, + async upsert(object: string, data: Record) { return this.create(object, data); }, + async count(object: string, ast: { where?: unknown }) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, list: Record[]) { + const out = []; + for (const r of list) out.push(await this.create(object, r)); + return out; + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return driver; +} + +async function bootRest() { + const engine = new ObjectQL(); + engine.registerDriver(memoryDriver() as never, true); + await engine.init(); + engine.registry.registerObject(ACCOUNT as never); + const protocol = new ObjectStackProtocolImplementation(engine as never); + const rest = new RestServer( + createMockServer() as never, + protocol as never, + { api: { requireAuth: false } } as never, + ); + (rest as unknown as { resolveExecCtx: () => Promise }).resolveExecCtx = + async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return rest; +} + +async function call( + rest: Awaited>, + method: string, + path: string, + req: Record, +) { + const route = (rest.getRoutes() as Array<{ method: string; path: string; handler: (rq: unknown, rs: unknown) => Promise }>) + .find((r) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + const res = makeRes(); + await route.handler({ method, params: {}, query: {}, body: {}, headers: {}, ...req }, res); + return res as { statusCode: number; body: Record }; +} + +const recordOf = (res: { body: Record }) => res.body.record as Record; + +describe('[#5504] write responses carry formula fields over REST', () => { + it('POST answers 201 with the formula present, equal to the GET that follows', async () => { + const rest = await bootRest(); + + const created = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'rf_account' }, + body: { name: 'PatchEcho' }, + }); + expect(created.statusCode).toBe(201); + // The reported symptom: the KEY was missing entirely, so a client could + // not tell "not evaluated" from "field not configured". + expect('display_title' in recordOf(created)).toBe(true); + expect(recordOf(created).display_title).toBe('ACC - PatchEcho'); + + const fetched = await call(rest, 'GET', DATA_ITEM, { + params: { object: 'rf_account', id: String(created.body.id) }, + }); + expect(recordOf(fetched).display_title).toBe(recordOf(created).display_title); + }, 60_000); + + it('PATCH answers 200 with the formula recomputed from the new values', async () => { + const rest = await bootRest(); + const created = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'rf_account' }, + body: { name: 'Before' }, + }); + const id = String(created.body.id); + + const patched = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'rf_account', id }, + body: { segment: 'growth' }, + }); + expect(patched.statusCode).toBe(200); + expect(recordOf(patched).display_title).toBe('ACC - Before'); + + const renamed = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'rf_account', id }, + body: { name: 'After' }, + }); + expect(recordOf(renamed).display_title).toBe('ACC - After'); + + const fetched = await call(rest, 'GET', DATA_ITEM, { params: { object: 'rf_account', id } }); + expect(recordOf(fetched).display_title).toBe('ACC - After'); + }, 60_000); + + it('the create response is renderable as-is for a formula `nameField`', async () => { + // No second round-trip: the consequence the issue led with. + const rest = await bootRest(); + const created = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'rf_account' }, + body: { name: 'Northwind' }, + }); + expect(recordOf(created)[ACCOUNT.nameField]).toBe('ACC - Northwind'); + }, 60_000); +}); diff --git a/packages/triggers/trigger-record-change/src/formula-context.test.ts b/packages/triggers/trigger-record-change/src/formula-context.test.ts index 58e20a9d15..cebccd818b 100644 --- a/packages/triggers/trigger-record-change/src/formula-context.test.ts +++ b/packages/triggers/trigger-record-change/src/formula-context.test.ts @@ -1,14 +1,20 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. /** - * #3426 — a `formula` field is a READ-time virtual: the engine evaluates it - * post-fetch on `find`/`findOne`, never on the write path, so it is absent from - * the raw after-create/after-update row a record-change flow is seeded with. - * `{record.full_name}` in a notify template (or a start condition) therefore - * resolved to an empty string. The trigger now re-reads the written record + * #3426 — a `formula` field is a virtual the engine evaluates post-fetch. It + * used to be evaluated on `find`/`findOne` ONLY, so it was absent from the raw + * after-create/after-update row a record-change flow is seeded with, and + * `{record.full_name}` in a notify template (or a start condition) resolved to + * an empty string. The trigger fixed that by re-reading the written record * through the data engine, so the seeded `record` carries the same computed * fields a data-API read returns. * + * #5504 later hydrated the write path's own return value as well, so the raw + * row is no longer the empty half of that pair. The re-read still earns its + * keep — `summary` / `rollup` virtuals and hook-side effects are only visible + * through a read — and this test keeps pinning the trigger's contract: the + * seeded record resolves computed fields, whatever the write path returned. + * * This exercises the whole stack (real ObjectQL + automation + record-change * trigger) with a formula field, proving the seeded record resolves it. The * notify node interpolates the very same variable map, so a formula that