diff --git a/.changeset/olive-moons-repeat.md b/.changeset/olive-moons-repeat.md new file mode 100644 index 0000000000..4ef300a09d --- /dev/null +++ b/.changeset/olive-moons-repeat.md @@ -0,0 +1,40 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): the insert-path runtime-owned strip now drops the value the CALLER submitted, not whatever value the key holds when it runs + +`stripRuntimeOwnedFields` runs after `beforeInsert`, but decided what to delete +from a snapshot of the caller's KEY NAMES. Those are different facts the moment a +hook writes to a runtime-owned column: `delete result[name]` took the hook's +value with it whenever the caller's payload happened to carry the same key. The +insert-side twin of the update-path defect fixed in the previous release, and +wrong for the identical reason. + +Measured, one object `{ title: text, code: autonumber }` and one `beforeInsert` +hook assigning `ctx.input.data.code`: + +- the caller omits `code` — the committed record holds the hook's value +- the caller sends `code` — the committed record holds `"1"`, the sequence value, + because the hook's write was deleted + +The two calls differ in nothing but whether the caller's payload happened to +carry a same-named key, and the first outcome is what the strip's own warning +text promises every hook author: "A beforeInsert/beforeUpdate hook does NOT need +either — hook-written keys are not caller-supplied." So this brings the code to +its own documented contract. Behaviour change — a whole-record POST (read a +template, edit fields, submit everything back) necessarily echoes the record +number it just read, so a hook that re-issues or normalizes that number no longer +loses its write to the sequence. + +The entry snapshot now carries the caller's values — as an explicit shallow copy +taken ahead of the hooks, so a hook mutating `ctx.input.data` in place cannot +rewrite the record of what the caller sent — and a runtime-owned key is stripped +only while it still holds the caller's own value. + +Not a relaxation of the runtime-owned write rule: a caller-seeded record number +that no hook overwrote is dropped exactly as before, on both the single-row and +batch insert paths, with the same warning, the same `onFieldsDropped` event and +the same `strictReadonlyWrites` refusal. `isSystem` and `preserveAudit` are +untouched. The comparison is `Object.is`, so a caller-forged `NaN` is still +recognised as the caller's own value and dropped. diff --git a/packages/objectql/src/engine-insert-runtime-owned-strip.test.ts b/packages/objectql/src/engine-insert-runtime-owned-strip.test.ts new file mode 100644 index 0000000000..136938cc69 --- /dev/null +++ b/packages/objectql/src/engine-insert-runtime-owned-strip.test.ts @@ -0,0 +1,301 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #6339 — the runtime-owned strip on the INSERT path must delete the value the +// CALLER SUBMITTED, never whatever value happens to sit on the key at the moment +// the strip runs. The insert-side twin of #5591 (update path), found while +// measuring that one, and wrong for the identical reason. +// +// `stripRuntimeOwnedFields` runs AFTER `beforeInsert` — `engine.insert` hands it +// the post-hook rows — but decided what to delete from a snapshot of the +// caller's KEY NAMES. Those are different facts the instant a hook writes to a +// runtime-owned column, and `delete result[name]` took whatever was standing +// there. Measured on `origin/main` (one object `{ title: text, code: autonumber +// }`, one hook assigning `ctx.input.data.code`): +// +// caller omits `code` ⇒ committed `code` = the hook's value (hook write lives) +// caller sends `code` ⇒ committed `code` = "1" (hook write dies) +// +// The two calls differ in nothing but whether the caller's payload happened to +// carry a same-named key — and the first outcome is what +// `runtimeOwnedStripWarning()` promises IN PROSE to every hook author: +// +// "A beforeInsert/beforeUpdate hook does NOT need either — hook-written keys +// are not caller-supplied." +// +// So the second is the code contradicting its own documented contract, not a +// deliberate policy. The user-visible shape is the whole-record POST: read a +// template, edit fields, submit everything back — the payload necessarily echoes +// the record-number column it just read, and a hook that re-issues or normalizes +// that number silently loses its write to the sequence. +// +// What this suite is NOT: a relaxation of #5503. A caller-seeded record number +// that no hook overwrote is still stripped, still warns, and still reports +// through `onFieldsDropped` / `strictReadonlyWrites` — pinned here next to the +// fix so the two verdicts are read together. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let n = 0; + const driver: any = { + // `supports: {}` — no native autonumber, so the ENGINE issues the sequence + // value in `applyAutonumbers`. That is the path the fallback shows up on: + // the strip deletes the hook's value, the field is then empty, and the + // sequence fills it. + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string) { return Array.from(storeFor(object).values()); }, + async findOne() { return null; }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update() { return null; }, + async updateMany() { return 0; }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +describe('insert strip acts on CALLER-submitted values (#6339)', () => { + let engine: ObjectQL; + let warns: string[]; + /** Every `ctx.input.data` the hook saw, in call order. */ + let hookSaw: Array>; + + beforeEach(async () => { + warns = []; + hookSaw = []; + const logger: any = { + warn: (m: string) => warns.push(String(m)), + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + child() { return logger; }, + }; + engine = new ObjectQL({ logger }); + engine.registerDriver(makeDriver().driver, true); + await engine.init(); + + engine.registry.registerObject({ + name: 'probe_num2', + fields: { title: { type: 'text' }, code: { type: 'autonumber' } }, + } as any); + + // The reported hook shape: a `beforeInsert` that OWNS the record number — + // it re-issues or normalizes it rather than letting the sequence decide. + // `code_source` is a plain text column recording that the hook ran, so a + // test can tell "the hook did not fire" from "the hook fired and lost". + engine.registerHook('beforeInsert', async (ctx: any) => { + hookSaw.push({ ...(ctx.input.data as Record) }); + if (ctx.input.data.title === 'no-hook') return; + ctx.input.data.code = `HOOK-${String(ctx.input.data.title)}`; + }, { object: 'probe_num2', priority: 50 }); + }); + + it('A (control, must not regress): a code the hook ADDS lands', async () => { + // The face that already worked before #6339, and the one + // `runtimeOwnedStripWarning` describes. The fix is worthless if it moved + // this one, so it is pinned first. + const row: any = await engine.insert('probe_num2', { title: 'A' }); + expect(row.code).toBe('HOOK-A'); + expect(warns).toEqual([]); + }); + + it('B (THE REPORT): a code the hook OVERWROTE lands, even though the caller sent the key', async () => { + // Identical to A except that the caller's payload also carries `code`. On + // `origin/main` this committed "1" — the sequence value, because the strip + // deleted the hook's write. Stated as the value it must NOT be, then as the + // value it must be. + const row: any = await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); + expect(row.code).not.toBe('1'); + expect(row.code).not.toBe('CALLER-FORGED'); + expect(row.code).toBe('HOOK-B'); + }); + + it('A and B now agree — the accident was the difference between them', async () => { + // The proof the old behaviour was never deliberate: the same hook, the same + // object, the same transition; only the caller's key set differed, and only + // one of the two hook writes survived. + const a: any = await engine.insert('probe_num2', { title: 'X' }); + const b: any = await engine.insert('probe_num2', { title: 'X', code: 'CALLER-FORGED' }); + expect(a.code).toBe(b.code); + expect(a.code).toBe('HOOK-X'); + }); + + it('#5503 UNCHANGED: a caller seed that NO hook overwrote is still stripped, with the same warning', async () => { + // `title: 'no-hook'` makes the hook return without writing, so the caller's + // value is the value on the key — and it goes, exactly as before. The + // sequence issues the number instead. + const row: any = await engine.insert('probe_num2', { title: 'no-hook', code: 'CALLER-FORGED' }); + expect(row.code).not.toBe('CALLER-FORGED'); + expect(row.code).toBe('1'); + expect(warns).toHaveLength(1); + // The contract of the text, not its wording (#5503's own pin discipline). + expect(warns[0]).toContain("Field 'code' on 'probe_num2'"); + expect(warns[0]).toContain('runtime-owned'); + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); + expect(warns[0]).toContain('hook-written keys are not caller-supplied'); + }); + + it('a hook-overwritten code produces NO warning — the log would otherwise lie', async () => { + // `runtimeOwnedStripWarning` says "the caller-supplied value was DROPPED and + // the write is being COMMITTED WITHOUT IT". After the fix the column IS + // committed, with the hook's value, so warning here would report a drop + // that did not happen. + await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); + expect(warns).toEqual([]); + }); + + it('P3: the caller-value snapshot is NOT the object the hook mutates in place', async () => { + // The insert-path detail the report flagged as needing measurement, pinned + // as an invariant rather than left as a coincidence. `suppliedPerRow` is now + // an explicit shallow COPY of `opCtx.data`, taken ahead of the hooks — so a + // hook writing `ctx.input.data.code = …` (in place, the ordinary spelling) + // cannot rewrite the record of what the caller sent. + // + // Measured direction: on `origin/main` these were ALREADY distinct objects, + // because `applyFieldDefaults` returns `{ ...record }` — but it hands the + // SAME reference back on its `!fields` early return, and + // `initializeSummaryFields` copies only when it seeds. The copy makes the + // separation a property of the insert path itself. + const payload: Record = { title: 'B', code: 'CALLER-FORGED' }; + const row: any = await engine.insert('probe_num2', payload); + + // The hook mutated a different object than the caller's... + expect(hookSaw[0]).not.toBe(payload); + // ...the caller's payload is unchanged by the write... + expect(payload).toEqual({ title: 'B', code: 'CALLER-FORGED' }); + // ...and the strip judged against 'CALLER-FORGED', not against the hook's + // value, which is why the hook's value survived. + expect(row.code).toBe('HOOK-B'); + }); + + it('a hook that REPLACES ctx.input.data wholesale is judged the same way', async () => { + // The other spelling a hook may use. `rows[i]` becomes an object the caller + // never touched, so no key on it holds the caller's value and nothing is + // stripped — including the record number the hook chose. + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data = { ...(ctx.input.data as Record), code: 'REPLACED-1' }; + }, { object: 'probe_num2', priority: 90 }); + const row: any = await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); + expect(row.code).toBe('REPLACED-1'); + expect(warns).toEqual([]); + }); + + it('BULK: one batch, mixed rows — each row is judged on its own values', async () => { + // The batch path runs the strip per row off one snapshot array, so a row + // whose hook overwrote the key and a row whose hook did not must come out + // differently in the SAME call. This is the shape a per-call flag or a + // shared snapshot would get wrong. + const rows: any = await engine.insert('probe_num2', [ + { title: 'r1' }, // hook ADDS ⇒ HOOK-r1 + { title: 'r2', code: 'CALLER-FORGED' }, // hook OVERWRITES ⇒ HOOK-r2 + { title: 'no-hook', code: 'CALLER-FORGED' }, // no hook write ⇒ stripped, sequence + { title: 'no-hook' }, // nothing at all ⇒ sequence + ]); + expect(rows[0].code).toBe('HOOK-r1'); + expect(rows[1].code).toBe('HOOK-r2'); + expect(rows[2].code).toBe('1'); + expect(rows[3].code).toBe('2'); + // Exactly one row was stripped, so exactly one warning. + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'code'"); + }); + + it('BULK: a caller-supplied value is never read from the WRONG row', async () => { + // Off-by-one insurance for the per-row snapshot: row 0 supplies the value + // row 1's hook happens to produce, and vice versa. A snapshot indexed wrong + // would strip one of them. + const rows: any = await engine.insert('probe_num2', [ + { title: 'p', code: 'HOOK-q' }, + { title: 'q', code: 'HOOK-p' }, + ]); + expect(rows[0].code).toBe('HOOK-p'); + expect(rows[1].code).toBe('HOOK-q'); + expect(warns).toEqual([]); + }); + + it('onFieldsDropped: silent for a hook-overwritten code, fires for a real drop', async () => { + // `DroppedFieldsEvent` is contracted as "dropped, and the write completed + // WITHOUT them" (#3407). A committed column is not a drop. + const kept: unknown[] = []; + await engine.insert( + 'probe_num2', + { title: 'B', code: 'CALLER-FORGED' }, + { onFieldsDropped: (e) => kept.push(e) }, + ); + expect(kept).toEqual([]); + + const dropped: unknown[] = []; + await engine.insert( + 'probe_num2', + { title: 'no-hook', code: 'CALLER-FORGED' }, + { onFieldsDropped: (e) => dropped.push(e) }, + ); + expect(dropped).toEqual([{ object: 'probe_num2', fields: ['code'], reason: 'readonly' }]); + }); + + it('strictReadonlyWrites refuses the real forge and admits the hook write', async () => { + // #5126 refuses rather than committing without the stripped column. A + // hook-overwritten key is not stripped, so there is nothing to refuse — the + // strict caller's contract is about columns that would be MISSING. + await expect(engine.insert( + 'probe_num2', + { title: 'no-hook', code: 'CALLER-FORGED' }, + { strictReadonlyWrites: true }, + )).rejects.toThrow(); + + const row: any = await engine.insert( + 'probe_num2', + { title: 'B', code: 'CALLER-FORGED' }, + { strictReadonlyWrites: true }, + ); + expect(row.code).toBe('HOOK-B'); + }); + + it('an isSystem caller keeps its own seeded record number', async () => { + // The whole pass is skipped for a trusted writer; the hook still runs, and + // still wins, because it assigns last. + const row: any = await engine.insert( + 'probe_num2', + { title: 'no-hook', code: 'SEED-9' }, + { context: { isSystem: true } }, + ); + expect(row.code).toBe('SEED-9'); + }); + + it('a preserveAudit historical import still reinstates a legacy record number', async () => { + const row: any = await engine.insert( + 'probe_num2', + { title: 'no-hook', code: 'LEGACY-7' }, + { context: { preserveAudit: true } }, + ); + expect(row.code).toBe('LEGACY-7'); + expect(warns).toEqual([]); + }); + + it('the hook can still SEE the caller-submitted record number', async () => { + // Why the fix compares values instead of stripping ahead of the hooks: a + // `beforeInsert` guard that reports on what the caller submitted reads + // `ctx.input.data`. Stripping first would empty that out and silently + // degrade every such diagnostic. + await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); + expect(hookSaw[0]).toEqual({ title: 'B', code: 'CALLER-FORGED' }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index b9abb482df..1f87bbd4f2 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5732,6 +5732,35 @@ export class ObjectQL implements IObjectQLEngine { // untouched, hooks run after and may override. const nowSnap = new Date(); const isBatch = Array.isArray(opCtx.data); + // [#4441] The RAW caller payload per row — before `applyFieldDefaults` + // resolves any `defaultValue` / `current_user` token and before the + // beforeInsert hooks stamp `owner_id` / `organization_id` / + // `created_by`. The reference check consults it to decide WHAT THE + // CALLER ACTUALLY SENT, so neither a platform stamp nor a backfilled + // default is ever reported as the caller's bad reference. + // + // [#6339] It carries the caller's VALUES, and it is taken HERE — ahead of + // the hooks — as an explicit shallow COPY. Both halves are load-bearing: + // - VALUES, because the runtime-owned strip below runs AFTER + // `beforeInsert`, so "the caller named this key" and "this key still + // holds the caller's value" are different facts, and only the second + // one licenses a delete (see `stripRuntimeOwnedFields`). + // - a COPY, taken ahead of the hooks, because `rows[i]` is a different + // object from `opCtx.data` only by the grace of two upstream helpers: + // `applyFieldDefaults` returns `{ ...record }` — except on its + // `!fields` early return, which hands the SAME reference back — and + // `initializeSummaryFields` copies only when it actually seeds. Hooks + // mutate `ctx.input.data` IN PLACE, so an aliased snapshot would + // answer "what did the caller send?" with the post-hook payload. + // Measured on `origin/main`: not aliased today, because that early + // return lines up with the strip's own `!fields` bail — a coincidence + // of three call sites, which the copy turns into an invariant of this + // one. Same spread, same reason, as the update path's `suppliedValues` + // (#5591). + const suppliedPerRow: Array> = + (isBatch ? (opCtx.data as any[]) : [opCtx.data]).map( + (row) => ({ ...((row ?? {}) as Record) }), + ); const defaultedData = isBatch ? (opCtx.data as any[]).map((row) => this.initializeSummaryFields( @@ -5814,16 +5843,6 @@ export class ObjectQL implements IObjectQLEngine { // Locale + translation hooks for the rejection messages (#3957) — // resolved once for the batch, identical for every row. const msgCtx = this.validationMessageContext(object, opCtx.context); - // [#4441] The RAW caller payload per row — before `applyFieldDefaults` - // resolved any `defaultValue` / `current_user` token and before the - // beforeInsert hooks stamped `owner_id` / `organization_id` / - // `created_by`. The reference check consults it to decide WHAT THE - // CALLER ACTUALLY SENT, so neither a platform stamp nor a backfilled - // default is ever reported as the caller's bad reference. - const suppliedPerRow: Array> = - (isBatch ? (opCtx.data as any[]) : [opCtx.data]).map( - (row) => (row ?? {}) as Record, - ); // [#5503] `autonumber` is RUNTIME-owned: the engine (or the driver's // persistent sequence) issues the value, so a non-system caller does not // get to supply or rewrite it. Until now nothing enforced that — a POST @@ -5842,14 +5861,21 @@ export class ObjectQL implements IObjectQLEngine { // path's, unchanged: `isSystem` (seed replay, migration) skips the whole // pass, and `preserveAudit` (#3493) lets a historical import reinstate // legacy record numbers. + // + // [#6339] `suppliedPerRow[i]` is handed over WHOLE — values included — + // rather than reduced to its key set. This pass runs after the + // beforeInsert hooks, so a key set could only say "the caller named + // this", and `delete` then took whatever value was standing there: a + // hook that RE-ISSUES the record number lost its write to any caller + // that had also submitted the key, while the same hook's write survived + // on a caller that had not. The update path's twin (#5591). const autonumberDropped: string[] = []; if (!opCtx.context?.isSystem) { const preserveAudit = opCtx.context?.preserveAudit === true; for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; - const supplied = new Set(Object.keys(suppliedPerRow[i] ?? {})); const stripped = stripRuntimeOwnedFields( - schemaForValidation as any, rows[i], supplied, this.logger, { preserveAudit }, + schemaForValidation as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, { preserveAudit }, ) as Record; if (stripped === rows[i]) continue; for (const k of Object.keys(rows[i])) { diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index edaf88cc7d..fe2d020c13 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -884,9 +884,8 @@ describe('stripReadonlyFields — implicit readonly on autonumber (#5503)', () = describe('stripRuntimeOwnedFields — the INSERT-side strip (#5503)', () => { it('drops a caller-supplied record number', () => { - const out = stripRuntimeOwnedFields( - numberedFields, { title: 'x', account_number: 'ACC-777777' }, new Set(['title', 'account_number']), - ); + const supplied = { title: 'x', account_number: 'ACC-777777' }; + const out = stripRuntimeOwnedFields(numberedFields, { ...supplied }, supplied); expect(out).toEqual({ title: 'x' }); }); @@ -895,27 +894,86 @@ describe('stripRuntimeOwnedFields — the INSERT-side strip (#5503)', () => { // strip lives (that is the #3043 protocol ingress); this narrower helper // must not quietly take over that job and start stripping columns the // trusted internal writers legitimately seed on create. - const out = stripRuntimeOwnedFields( - numberedFields, - { title: 'x', closed_at: '2021-01-01T00:00:00Z' }, - new Set(['title', 'closed_at']), - ); + const supplied = { title: 'x', closed_at: '2021-01-01T00:00:00Z' }; + const out = stripRuntimeOwnedFields(numberedFields, { ...supplied }, supplied); expect(out).toEqual({ title: 'x', closed_at: '2021-01-01T00:00:00Z' }); }); it('KEEPS a hook-stamped value and returns the SAME object when nothing is stripped', () => { const d = { title: 'x', account_number: 'HOOK-1' }; - expect(stripRuntimeOwnedFields(numberedFields, d, new Set(['title']))).toBe(d); + expect(stripRuntimeOwnedFields(numberedFields, d, { title: 'x' })).toBe(d); }); it('KEEPS it under preserveAudit', () => { + const supplied = { account_number: 'LEGACY-7' }; const out = stripRuntimeOwnedFields( - numberedFields, { account_number: 'LEGACY-7' }, new Set(['account_number']), undefined, { preserveAudit: true }, + numberedFields, { ...supplied }, supplied, undefined, { preserveAudit: true }, ); expect(out).toEqual({ account_number: 'LEGACY-7' }); }); }); +// #6339 — the insert-side twin of #5591, and wrong for the identical reason: +// the strip runs AFTER `beforeInsert`, so the value on the key at strip time is +// not necessarily the caller's. A key-only guard deleted whatever was standing +// there, which killed a hook that RE-ISSUED a record number the caller had also +// submitted — while the same hook's write survived on a caller that had not. +describe('stripRuntimeOwnedFields — supplied VALUE identity, not just key presence (#6339)', () => { + it('KEEPS a record number a hook OVERWROTE, even though the caller supplied it', () => { + // The caller forged `account_number`; a beforeInsert hook then wrote its + // own over it. What sits on the key now is a PLATFORM write. + const supplied = { title: 'x', account_number: 'ACC-777777' }; + const afterHooks = { title: 'x', account_number: 'HOOK-OVERWRITE' }; + const out = stripRuntimeOwnedFields(numberedFields, afterHooks, supplied); + expect(out).toEqual({ title: 'x', account_number: 'HOOK-OVERWRITE' }); + expect(out).toBe(afterHooks); // nothing dropped ⇒ same reference + }); + + it('STILL drops it when the hook wrote the caller value back unchanged', () => { + // Identity is the whole test: an unchanged value is indistinguishable from + // "no hook touched it", and the fail-safe direction is to strip. + const supplied = { account_number: 'ACC-777777' }; + const out = stripRuntimeOwnedFields(numberedFields, { account_number: 'ACC-777777' }, supplied); + expect(out).toEqual({}); + }); + + it('drops a caller-forged NaN — `Object.is`, not `===`', () => { + // `===` reports NaN !== NaN, which would read a forged NaN as "a hook + // rewrote this" and KEEP it. The one input where the loose operator + // inverts the verdict, so it is pinned rather than left to a reviewer. + const numeric = { fields: { seq: { type: 'autonumber' } } }; + const supplied = { seq: Number.NaN }; + const out = stripRuntimeOwnedFields(numeric, { seq: Number.NaN }, supplied); + expect(out).toEqual({}); + }); + + it('does not read an inherited `Object.prototype` key as caller-supplied', () => { + // `constructor` matches the machine-name regex, so it is a legal field + // name; `name in supplied` would be TRUE for it on any plain object and + // would strip a hook stamp. Own-property check, pinned. + const oddly = { fields: { constructor: { type: 'autonumber' } } }; + const out = stripRuntimeOwnedFields(oddly, { constructor: 'HOOK-1' }, {}); + expect(out).toEqual({ constructor: 'HOOK-1' }); + }); + + it('warns only for the value it really dropped', () => { + // The warning is the caller-visible half of the strip: a hook-overwritten + // key is COMMITTED, so warning about it would make the log lie in exactly + // the direction `runtimeOwnedStripWarning` promises it does not. + const warns: string[] = []; + const logger = { warn: (m: string) => warns.push(m) } as any; + stripRuntimeOwnedFields( + numberedFields, { account_number: 'HOOK-OVERWRITE' }, { account_number: 'ACC-777777' }, logger, + ); + expect(warns).toEqual([]); + stripRuntimeOwnedFields( + numberedFields, { account_number: 'ACC-777777' }, { account_number: 'ACC-777777' }, logger, + ); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'account_number'"); + }); +}); + describe('needsPriorRecord — field conditional rules (B2)', () => { it('is true when a field declares requiredWhen / readonlyWhen', () => { expect(needsPriorRecord(invoiceFields as any)).toBe(true); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 623904cb24..214ed463ee 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -808,8 +808,9 @@ export function stripReadonlyWhenFieldsMulti( * historical imports and seed data may carry pre-computed totals. * * DO NOT "fix the code to match this comment" by adding `summary` here. The - * insert-side strip ({@link stripRuntimeOwnedFields}) keys on the RAW caller - * payload and runs in `engine.insert` AFTER the seed pass, so a plain + * insert-side strip ({@link stripRuntimeOwnedFields}) judges against the RAW + * caller payload (its keys AND their values, #6339) and runs in + * `engine.insert` AFTER the seed pass, so a plain * (non-`isSystem`, non-`preserveAudit`) import of a parent carrying * `task_count: 42` would lose the 42 to the strip and get no 0 from the seed * either — the seed already skipped that field precisely BECAUSE the caller @@ -998,16 +999,54 @@ export function stripReadonlyFields( * it runs before the payload is dispatched, which covers the SQL driver's * `supports.autonumber` path without the driver participating at all. * - * Same two guards as the update strip: only keys the CALLER supplied are - * candidates (a `beforeInsert` hook that computes the value survives), and the - * `preserveAudit` whitelist is honoured so a historical import may reinstate the - * legacy record numbers it is migrating. `isSystem` writes never reach here — - * the caller gates on that, exactly as it does for the update strip. + * Same guards as the update strip, and — since #6339 — the same THREE of them: + * the CALLER must have sent the key, the key must still hold THE CALLER'S OWN + * VALUE, and the `preserveAudit` whitelist is honoured so a historical import + * may reinstate the legacy record numbers it is migrating. `isSystem` writes + * never reach here — the caller gates on that, exactly as it does for the + * update strip. + * + * ### Why `supplied` carries VALUES, not just keys (#6339) + * + * The insert-side twin of #5591, found while measuring it, and wrong for the + * identical reason. This strip runs AFTER `beforeInsert` (`engine.insert` calls + * it on the post-hook rows), so "the caller named this key" and "this key still + * holds the caller's value" are different facts, and only the second licenses a + * `delete`. With a key SET the delete took whatever value was standing there — + * so a `beforeInsert` hook that re-issues or normalizes a record number lost its + * write to any caller that had ALSO submitted that key, and the record fell back + * to the sequence value. + * + * Measured (objectstack#6339, real ObjectQL + in-memory driver, one object + * `{ title: text, code: autonumber }` and one hook writing `ctx.input.data.code`): + * + * - caller omits `code` ⇒ committed `code` = the hook's value (hook write lives) + * - caller sends `code` ⇒ committed `code` = `"1"` (hook write dies) + * + * The two differ in nothing but whether the caller's payload happened to carry a + * same-named key — and the first outcome is the one {@link + * runtimeOwnedStripWarning} promises IN PROSE to every hook author: "A + * beforeInsert/beforeUpdate hook does NOT need either — hook-written keys are + * not caller-supplied." The key-set judgement made that sentence true only by + * accident, so this is the code being brought to its own documented contract. + * + * NOT a relaxation of #5503: a caller-seeded record number that no hook rewrote + * is still dropped, still warns with the same text, and still reports through + * `onFieldsDropped` / `strictReadonlyWrites`. What changed is exclusively the + * case where the value being deleted was never the caller's. + * + * KNOWN LIMIT, identical to the update side's and deliberately not papered over: + * the snapshot is SHALLOW, so a hook that mutates a caller-supplied object IN + * PLACE is indistinguishable from a hook that did nothing, and the field is + * still stripped. That fallback is the pre-#6339 behaviour, i.e. fail-safe; a + * hook meaning to own a runtime-owned column should ASSIGN to it. (An + * `autonumber` value is a scalar in every supported shape, so this limit is + * theoretical here in a way it is not for the update path's `json` columns.) */ export function stripRuntimeOwnedFields( objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, - suppliedKeys: ReadonlySet, + supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], options?: { preserveAudit?: boolean }, ): Record | undefined | null { @@ -1018,7 +1057,18 @@ export function stripRuntimeOwnedFields( for (const [name, def] of Object.entries(fields)) { if (!isRuntimeOwnedField(def)) continue; if (!(name in (result as Record))) continue; - if (!suppliedKeys.has(name)) continue; // hook/middleware stamp — keep + // Own-property, never `in`: a field name is `^[a-z_][a-z0-9_]*$`, which + // admits `constructor` / `valueOf` — inherited from `Object.prototype` on + // any plain snapshot, so `in` would call a hook stamp caller-supplied and + // strip it. + if (!Object.prototype.hasOwnProperty.call(supplied, name)) continue; // hook/middleware stamp — keep + // [#6339] ...and it must still BE the caller's value. A hook that overwrote + // this key wrote a PLATFORM value, and deleting that is what sent records + // to the database holding a sequence number the hook had just replaced. + // `Object.is`, not `===`: `===` reports NaN !== NaN, which would read a + // caller-forged NaN as "a hook rewrote it" and KEEP the forgery — the one + // input where the loose operator inverts the verdict. + if (!Object.is((result as Record)[name], supplied[name])) continue; if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data }; delete (result as Record)[name];