|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | +// |
| 3 | +// #6339 — the runtime-owned strip on the INSERT path must delete the value the |
| 4 | +// CALLER SUBMITTED, never whatever value happens to sit on the key at the moment |
| 5 | +// the strip runs. The insert-side twin of #5591 (update path), found while |
| 6 | +// measuring that one, and wrong for the identical reason. |
| 7 | +// |
| 8 | +// `stripRuntimeOwnedFields` runs AFTER `beforeInsert` — `engine.insert` hands it |
| 9 | +// the post-hook rows — but decided what to delete from a snapshot of the |
| 10 | +// caller's KEY NAMES. Those are different facts the instant a hook writes to a |
| 11 | +// runtime-owned column, and `delete result[name]` took whatever was standing |
| 12 | +// there. Measured on `origin/main` (one object `{ title: text, code: autonumber |
| 13 | +// }`, one hook assigning `ctx.input.data.code`): |
| 14 | +// |
| 15 | +// caller omits `code` ⇒ committed `code` = the hook's value (hook write lives) |
| 16 | +// caller sends `code` ⇒ committed `code` = "1" (hook write dies) |
| 17 | +// |
| 18 | +// The two calls differ in nothing but whether the caller's payload happened to |
| 19 | +// carry a same-named key — and the first outcome is what |
| 20 | +// `runtimeOwnedStripWarning()` promises IN PROSE to every hook author: |
| 21 | +// |
| 22 | +// "A beforeInsert/beforeUpdate hook does NOT need either — hook-written keys |
| 23 | +// are not caller-supplied." |
| 24 | +// |
| 25 | +// So the second is the code contradicting its own documented contract, not a |
| 26 | +// deliberate policy. The user-visible shape is the whole-record POST: read a |
| 27 | +// template, edit fields, submit everything back — the payload necessarily echoes |
| 28 | +// the record-number column it just read, and a hook that re-issues or normalizes |
| 29 | +// that number silently loses its write to the sequence. |
| 30 | +// |
| 31 | +// What this suite is NOT: a relaxation of #5503. A caller-seeded record number |
| 32 | +// that no hook overwrote is still stripped, still warns, and still reports |
| 33 | +// through `onFieldsDropped` / `strictReadonlyWrites` — pinned here next to the |
| 34 | +// fix so the two verdicts are read together. |
| 35 | + |
| 36 | +import { describe, it, expect, beforeEach } from 'vitest'; |
| 37 | +import { ObjectQL } from './engine.js'; |
| 38 | + |
| 39 | +function makeDriver() { |
| 40 | + const stores = new Map<string, Map<string, any>>(); |
| 41 | + const storeFor = (o: string) => { |
| 42 | + let s = stores.get(o); |
| 43 | + if (!s) { s = new Map(); stores.set(o, s); } |
| 44 | + return s; |
| 45 | + }; |
| 46 | + let n = 0; |
| 47 | + const driver: any = { |
| 48 | + // `supports: {}` — no native autonumber, so the ENGINE issues the sequence |
| 49 | + // value in `applyAutonumbers`. That is the path the fallback shows up on: |
| 50 | + // the strip deletes the hook's value, the field is then empty, and the |
| 51 | + // sequence fills it. |
| 52 | + name: 'memory', version: '0.0.0', supports: {}, |
| 53 | + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, |
| 54 | + async find(object: string) { return Array.from(storeFor(object).values()); }, |
| 55 | + async findOne() { return null; }, |
| 56 | + async create(object: string, data: Record<string, unknown>) { |
| 57 | + n += 1; |
| 58 | + const id = (data.id as string) ?? `r_${n}`; |
| 59 | + const row = { ...data, id }; |
| 60 | + storeFor(object).set(id, row); |
| 61 | + return row; |
| 62 | + }, |
| 63 | + async update() { return null; }, |
| 64 | + async updateMany() { return 0; }, |
| 65 | + async delete(object: string, id: string) { return storeFor(object).delete(id); }, |
| 66 | + async count() { return 0; }, |
| 67 | + async bulkCreate(object: string, rows: Record<string, unknown>[]) { |
| 68 | + return Promise.all(rows.map((r) => this.create(object, r, undefined))); |
| 69 | + }, |
| 70 | + async bulkUpdate() { return []; }, async bulkDelete() {}, |
| 71 | + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, |
| 72 | + async commit() {}, async rollback() {}, |
| 73 | + }; |
| 74 | + return { driver, storeFor }; |
| 75 | +} |
| 76 | + |
| 77 | +describe('insert strip acts on CALLER-submitted values (#6339)', () => { |
| 78 | + let engine: ObjectQL; |
| 79 | + let warns: string[]; |
| 80 | + /** Every `ctx.input.data` the hook saw, in call order. */ |
| 81 | + let hookSaw: Array<Record<string, unknown>>; |
| 82 | + |
| 83 | + beforeEach(async () => { |
| 84 | + warns = []; |
| 85 | + hookSaw = []; |
| 86 | + const logger: any = { |
| 87 | + warn: (m: string) => warns.push(String(m)), |
| 88 | + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, |
| 89 | + child() { return logger; }, |
| 90 | + }; |
| 91 | + engine = new ObjectQL({ logger }); |
| 92 | + engine.registerDriver(makeDriver().driver, true); |
| 93 | + await engine.init(); |
| 94 | + |
| 95 | + engine.registry.registerObject({ |
| 96 | + name: 'probe_num2', |
| 97 | + fields: { title: { type: 'text' }, code: { type: 'autonumber' } }, |
| 98 | + } as any); |
| 99 | + |
| 100 | + // The reported hook shape: a `beforeInsert` that OWNS the record number — |
| 101 | + // it re-issues or normalizes it rather than letting the sequence decide. |
| 102 | + // `code_source` is a plain text column recording that the hook ran, so a |
| 103 | + // test can tell "the hook did not fire" from "the hook fired and lost". |
| 104 | + engine.registerHook('beforeInsert', async (ctx: any) => { |
| 105 | + hookSaw.push({ ...(ctx.input.data as Record<string, unknown>) }); |
| 106 | + if (ctx.input.data.title === 'no-hook') return; |
| 107 | + ctx.input.data.code = `HOOK-${String(ctx.input.data.title)}`; |
| 108 | + }, { object: 'probe_num2', priority: 50 }); |
| 109 | + }); |
| 110 | + |
| 111 | + it('A (control, must not regress): a code the hook ADDS lands', async () => { |
| 112 | + // The face that already worked before #6339, and the one |
| 113 | + // `runtimeOwnedStripWarning` describes. The fix is worthless if it moved |
| 114 | + // this one, so it is pinned first. |
| 115 | + const row: any = await engine.insert('probe_num2', { title: 'A' }); |
| 116 | + expect(row.code).toBe('HOOK-A'); |
| 117 | + expect(warns).toEqual([]); |
| 118 | + }); |
| 119 | + |
| 120 | + it('B (THE REPORT): a code the hook OVERWROTE lands, even though the caller sent the key', async () => { |
| 121 | + // Identical to A except that the caller's payload also carries `code`. On |
| 122 | + // `origin/main` this committed "1" — the sequence value, because the strip |
| 123 | + // deleted the hook's write. Stated as the value it must NOT be, then as the |
| 124 | + // value it must be. |
| 125 | + const row: any = await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); |
| 126 | + expect(row.code).not.toBe('1'); |
| 127 | + expect(row.code).not.toBe('CALLER-FORGED'); |
| 128 | + expect(row.code).toBe('HOOK-B'); |
| 129 | + }); |
| 130 | + |
| 131 | + it('A and B now agree — the accident was the difference between them', async () => { |
| 132 | + // The proof the old behaviour was never deliberate: the same hook, the same |
| 133 | + // object, the same transition; only the caller's key set differed, and only |
| 134 | + // one of the two hook writes survived. |
| 135 | + const a: any = await engine.insert('probe_num2', { title: 'X' }); |
| 136 | + const b: any = await engine.insert('probe_num2', { title: 'X', code: 'CALLER-FORGED' }); |
| 137 | + expect(a.code).toBe(b.code); |
| 138 | + expect(a.code).toBe('HOOK-X'); |
| 139 | + }); |
| 140 | + |
| 141 | + it('#5503 UNCHANGED: a caller seed that NO hook overwrote is still stripped, with the same warning', async () => { |
| 142 | + // `title: 'no-hook'` makes the hook return without writing, so the caller's |
| 143 | + // value is the value on the key — and it goes, exactly as before. The |
| 144 | + // sequence issues the number instead. |
| 145 | + const row: any = await engine.insert('probe_num2', { title: 'no-hook', code: 'CALLER-FORGED' }); |
| 146 | + expect(row.code).not.toBe('CALLER-FORGED'); |
| 147 | + expect(row.code).toBe('1'); |
| 148 | + expect(warns).toHaveLength(1); |
| 149 | + // The contract of the text, not its wording (#5503's own pin discipline). |
| 150 | + expect(warns[0]).toContain("Field 'code' on 'probe_num2'"); |
| 151 | + expect(warns[0]).toContain('runtime-owned'); |
| 152 | + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); |
| 153 | + expect(warns[0]).toContain('hook-written keys are not caller-supplied'); |
| 154 | + }); |
| 155 | + |
| 156 | + it('a hook-overwritten code produces NO warning — the log would otherwise lie', async () => { |
| 157 | + // `runtimeOwnedStripWarning` says "the caller-supplied value was DROPPED and |
| 158 | + // the write is being COMMITTED WITHOUT IT". After the fix the column IS |
| 159 | + // committed, with the hook's value, so warning here would report a drop |
| 160 | + // that did not happen. |
| 161 | + await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); |
| 162 | + expect(warns).toEqual([]); |
| 163 | + }); |
| 164 | + |
| 165 | + it('P3: the caller-value snapshot is NOT the object the hook mutates in place', async () => { |
| 166 | + // The insert-path detail the report flagged as needing measurement, pinned |
| 167 | + // as an invariant rather than left as a coincidence. `suppliedPerRow` is now |
| 168 | + // an explicit shallow COPY of `opCtx.data`, taken ahead of the hooks — so a |
| 169 | + // hook writing `ctx.input.data.code = …` (in place, the ordinary spelling) |
| 170 | + // cannot rewrite the record of what the caller sent. |
| 171 | + // |
| 172 | + // Measured direction: on `origin/main` these were ALREADY distinct objects, |
| 173 | + // because `applyFieldDefaults` returns `{ ...record }` — but it hands the |
| 174 | + // SAME reference back on its `!fields` early return, and |
| 175 | + // `initializeSummaryFields` copies only when it seeds. The copy makes the |
| 176 | + // separation a property of the insert path itself. |
| 177 | + const payload: Record<string, unknown> = { title: 'B', code: 'CALLER-FORGED' }; |
| 178 | + const row: any = await engine.insert('probe_num2', payload); |
| 179 | + |
| 180 | + // The hook mutated a different object than the caller's... |
| 181 | + expect(hookSaw[0]).not.toBe(payload); |
| 182 | + // ...the caller's payload is unchanged by the write... |
| 183 | + expect(payload).toEqual({ title: 'B', code: 'CALLER-FORGED' }); |
| 184 | + // ...and the strip judged against 'CALLER-FORGED', not against the hook's |
| 185 | + // value, which is why the hook's value survived. |
| 186 | + expect(row.code).toBe('HOOK-B'); |
| 187 | + }); |
| 188 | + |
| 189 | + it('a hook that REPLACES ctx.input.data wholesale is judged the same way', async () => { |
| 190 | + // The other spelling a hook may use. `rows[i]` becomes an object the caller |
| 191 | + // never touched, so no key on it holds the caller's value and nothing is |
| 192 | + // stripped — including the record number the hook chose. |
| 193 | + engine.registerHook('beforeInsert', async (ctx: any) => { |
| 194 | + ctx.input.data = { ...(ctx.input.data as Record<string, unknown>), code: 'REPLACED-1' }; |
| 195 | + }, { object: 'probe_num2', priority: 90 }); |
| 196 | + const row: any = await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); |
| 197 | + expect(row.code).toBe('REPLACED-1'); |
| 198 | + expect(warns).toEqual([]); |
| 199 | + }); |
| 200 | + |
| 201 | + it('BULK: one batch, mixed rows — each row is judged on its own values', async () => { |
| 202 | + // The batch path runs the strip per row off one snapshot array, so a row |
| 203 | + // whose hook overwrote the key and a row whose hook did not must come out |
| 204 | + // differently in the SAME call. This is the shape a per-call flag or a |
| 205 | + // shared snapshot would get wrong. |
| 206 | + const rows: any = await engine.insert('probe_num2', [ |
| 207 | + { title: 'r1' }, // hook ADDS ⇒ HOOK-r1 |
| 208 | + { title: 'r2', code: 'CALLER-FORGED' }, // hook OVERWRITES ⇒ HOOK-r2 |
| 209 | + { title: 'no-hook', code: 'CALLER-FORGED' }, // no hook write ⇒ stripped, sequence |
| 210 | + { title: 'no-hook' }, // nothing at all ⇒ sequence |
| 211 | + ]); |
| 212 | + expect(rows[0].code).toBe('HOOK-r1'); |
| 213 | + expect(rows[1].code).toBe('HOOK-r2'); |
| 214 | + expect(rows[2].code).toBe('1'); |
| 215 | + expect(rows[3].code).toBe('2'); |
| 216 | + // Exactly one row was stripped, so exactly one warning. |
| 217 | + expect(warns).toHaveLength(1); |
| 218 | + expect(warns[0]).toContain("Field 'code'"); |
| 219 | + }); |
| 220 | + |
| 221 | + it('BULK: a caller-supplied value is never read from the WRONG row', async () => { |
| 222 | + // Off-by-one insurance for the per-row snapshot: row 0 supplies the value |
| 223 | + // row 1's hook happens to produce, and vice versa. A snapshot indexed wrong |
| 224 | + // would strip one of them. |
| 225 | + const rows: any = await engine.insert('probe_num2', [ |
| 226 | + { title: 'p', code: 'HOOK-q' }, |
| 227 | + { title: 'q', code: 'HOOK-p' }, |
| 228 | + ]); |
| 229 | + expect(rows[0].code).toBe('HOOK-p'); |
| 230 | + expect(rows[1].code).toBe('HOOK-q'); |
| 231 | + expect(warns).toEqual([]); |
| 232 | + }); |
| 233 | + |
| 234 | + it('onFieldsDropped: silent for a hook-overwritten code, fires for a real drop', async () => { |
| 235 | + // `DroppedFieldsEvent` is contracted as "dropped, and the write completed |
| 236 | + // WITHOUT them" (#3407). A committed column is not a drop. |
| 237 | + const kept: unknown[] = []; |
| 238 | + await engine.insert( |
| 239 | + 'probe_num2', |
| 240 | + { title: 'B', code: 'CALLER-FORGED' }, |
| 241 | + { onFieldsDropped: (e) => kept.push(e) }, |
| 242 | + ); |
| 243 | + expect(kept).toEqual([]); |
| 244 | + |
| 245 | + const dropped: unknown[] = []; |
| 246 | + await engine.insert( |
| 247 | + 'probe_num2', |
| 248 | + { title: 'no-hook', code: 'CALLER-FORGED' }, |
| 249 | + { onFieldsDropped: (e) => dropped.push(e) }, |
| 250 | + ); |
| 251 | + expect(dropped).toEqual([{ object: 'probe_num2', fields: ['code'], reason: 'readonly' }]); |
| 252 | + }); |
| 253 | + |
| 254 | + it('strictReadonlyWrites refuses the real forge and admits the hook write', async () => { |
| 255 | + // #5126 refuses rather than committing without the stripped column. A |
| 256 | + // hook-overwritten key is not stripped, so there is nothing to refuse — the |
| 257 | + // strict caller's contract is about columns that would be MISSING. |
| 258 | + await expect(engine.insert( |
| 259 | + 'probe_num2', |
| 260 | + { title: 'no-hook', code: 'CALLER-FORGED' }, |
| 261 | + { strictReadonlyWrites: true }, |
| 262 | + )).rejects.toThrow(); |
| 263 | + |
| 264 | + const row: any = await engine.insert( |
| 265 | + 'probe_num2', |
| 266 | + { title: 'B', code: 'CALLER-FORGED' }, |
| 267 | + { strictReadonlyWrites: true }, |
| 268 | + ); |
| 269 | + expect(row.code).toBe('HOOK-B'); |
| 270 | + }); |
| 271 | + |
| 272 | + it('an isSystem caller keeps its own seeded record number', async () => { |
| 273 | + // The whole pass is skipped for a trusted writer; the hook still runs, and |
| 274 | + // still wins, because it assigns last. |
| 275 | + const row: any = await engine.insert( |
| 276 | + 'probe_num2', |
| 277 | + { title: 'no-hook', code: 'SEED-9' }, |
| 278 | + { context: { isSystem: true } }, |
| 279 | + ); |
| 280 | + expect(row.code).toBe('SEED-9'); |
| 281 | + }); |
| 282 | + |
| 283 | + it('a preserveAudit historical import still reinstates a legacy record number', async () => { |
| 284 | + const row: any = await engine.insert( |
| 285 | + 'probe_num2', |
| 286 | + { title: 'no-hook', code: 'LEGACY-7' }, |
| 287 | + { context: { preserveAudit: true } }, |
| 288 | + ); |
| 289 | + expect(row.code).toBe('LEGACY-7'); |
| 290 | + expect(warns).toEqual([]); |
| 291 | + }); |
| 292 | + |
| 293 | + it('the hook can still SEE the caller-submitted record number', async () => { |
| 294 | + // Why the fix compares values instead of stripping ahead of the hooks: a |
| 295 | + // `beforeInsert` guard that reports on what the caller submitted reads |
| 296 | + // `ctx.input.data`. Stripping first would empty that out and silently |
| 297 | + // degrade every such diagnostic. |
| 298 | + await engine.insert('probe_num2', { title: 'B', code: 'CALLER-FORGED' }); |
| 299 | + expect(hookSaw[0]).toEqual({ title: 'B', code: 'CALLER-FORGED' }); |
| 300 | + }); |
| 301 | +}); |
0 commit comments