|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | +// |
| 3 | +// objectstack#6435 — the BY-ID half of #6262. A single-row update must not hand |
| 4 | +// the driver a SET payload whose `id` the dispatch has already ruled is not a |
| 5 | +// primary key. |
| 6 | +// |
| 7 | +// ## The shape |
| 8 | +// |
| 9 | +// `update(o, { id: { $in: ['a','b'] }, title: 'x' }, { where: { id: 'rec_1' } })` |
| 10 | +// dispatches correctly and has since #5748 / PR #5919: the operator object is |
| 11 | +// not an id, so it stops shadowing the ladder, the decision falls through to |
| 12 | +// `where.id`, and `rec_1` is bound. `ENGINE_UPDATE_DISPATCH_CASES` states that |
| 13 | +// row verbatim (`expect: 'by-id'`, `expectId: 'rec_1'`). What #5748 did NOT do |
| 14 | +// — and what PR #6433 fixed for the `multi` arm only — is clean the PAYLOAD. |
| 15 | +// Measured on `origin/main` with a recording driver, this branch: |
| 16 | +// |
| 17 | +// ``` |
| 18 | +// driver.update('task', 'rec_1', { id: { $in: ['a','b'] }, title: 'x' }) |
| 19 | +// ^^^^^^^^^^^^^^^^^^^^^^ the SET clause |
| 20 | +// ``` |
| 21 | +// |
| 22 | +// `packages/drivers/driver-sql/src/sql-driver.ts`'s `update()` formats the |
| 23 | +// WHOLE payload (`applyWriteColumnMap(formatInput(object, data))`; `id` is on |
| 24 | +// no skip list), so the row lands as |
| 25 | +// `UPDATE task SET id = '{"$in":["a","b"]}', title = 'x' WHERE id = 'rec_1'` — |
| 26 | +// rec_1's identity overwritten with a serialized operator object, irreversibly, |
| 27 | +// on any backend that accepts the write. |
| 28 | +// |
| 29 | +// ## Why a strip, and why exactly this wide |
| 30 | +// |
| 31 | +// Route A, the same answer #6262 gave one arm over: a value the engine has |
| 32 | +// ALREADY RULED is not a primary key does not get to sit in the primary-key |
| 33 | +// column either. It changes NO verdict — every case in |
| 34 | +// `ENGINE_UPDATE_DISPATCH_CASES` still resolves as it did, which is why every |
| 35 | +// test below asserts the dispatch alongside the payload |
| 36 | +// (`assertEngineUpdateDispatch`, the producer's own decision). |
| 37 | +// |
| 38 | +// A TRUTHY SCALAR `data.id` is deliberately left alone. There the payload's |
| 39 | +// `id` IS the bound key (a scalar `data.id` outranks `where` and `multi` |
| 40 | +// alike), so the write is `SET id = 'rec_1' WHERE id = 'rec_1'` — a same-value |
| 41 | +// no-op, redundant rather than damaging, and long-standing behaviour. Widening |
| 42 | +// the strip over it is a separate decision and is NOT taken here; the contrast |
| 43 | +// pins below record that it stayed put. |
| 44 | +// |
| 45 | +// Route B (reject a non-scalar `data.id` outright) would reverse the |
| 46 | +// `expect: 'by-id'` verdict the case-set states today — a partial rollback of |
| 47 | +// #5748's ruling A, i.e. a maintainer decision, not this fix. Route C |
| 48 | +// (per-driver skip lists) is the #5240 / #4434 shape of five backends giving |
| 49 | +// one question five answers. |
| 50 | +// |
| 51 | +// ## The membership rule, asked and never re-derived |
| 52 | +// |
| 53 | +// "Is this payload `id` a primary key?" is asked by CALLING the dispatch on the |
| 54 | +// payload with no options — `resolveEngineUpdateDispatch(data, undefined)` is |
| 55 | +// `by-id` exactly when `data.id` is a truthy scalar. The scalar test itself |
| 56 | +// (`asScalarId`) is deliberately unexported: "adding a third public spelling of |
| 57 | +// the same question is how a rule with one definition grows a second one" |
| 58 | +// (`engine-update-dispatch.ts`). So the strip set is defined by the producer, |
| 59 | +// not mirrored beside it, and the table below is a MEASUREMENT of that set |
| 60 | +// rather than a second statement of it. |
| 61 | + |
| 62 | +import { describe, it, expect } from 'vitest'; |
| 63 | +import type { EngineUpdateOptions } from '@objectstack/spec/data'; |
| 64 | +import { ObjectQL } from './engine.js'; |
| 65 | +import { assertEngineUpdateDispatch } from './engine-update-dispatch.js'; |
| 66 | + |
| 67 | +interface RecordedCall { |
| 68 | + readonly fn: 'update' | 'updateMany'; |
| 69 | + readonly id?: unknown; |
| 70 | + readonly ast?: unknown; |
| 71 | + /** A COPY — the engine may keep mutating its own payload after the call. */ |
| 72 | + readonly data: Record<string, unknown>; |
| 73 | +} |
| 74 | + |
| 75 | +/** |
| 76 | + * Records the exact SET payload each driver entry point received. |
| 77 | + * |
| 78 | + * Same double as `engine-update-multi-payload-id.test.ts` uses, on purpose: |
| 79 | + * the two halves of one defect are measured with one instrument. |
| 80 | + */ |
| 81 | +function makeRecordingDriver() { |
| 82 | + const calls: RecordedCall[] = []; |
| 83 | + const driver: any = { |
| 84 | + name: 'recording', |
| 85 | + version: '0.0.0', |
| 86 | + supports: {}, |
| 87 | + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, |
| 88 | + async find() { return []; }, |
| 89 | + async findOne() { return null; }, |
| 90 | + async create(_o: string, data: Record<string, unknown>) { return { id: 'r1', ...data }; }, |
| 91 | + async update(_o: string, id: string, data: Record<string, unknown>) { |
| 92 | + calls.push({ fn: 'update', id, data: { ...data } }); |
| 93 | + return { id, ...data }; |
| 94 | + }, |
| 95 | + async updateMany(_o: string, ast: unknown, data: Record<string, unknown>) { |
| 96 | + calls.push({ fn: 'updateMany', ast, data: { ...data } }); |
| 97 | + return 2; |
| 98 | + }, |
| 99 | + async delete() { return true; }, |
| 100 | + async deleteMany() { return 0; }, |
| 101 | + async count() { return 0; }, |
| 102 | + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, |
| 103 | + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, |
| 104 | + async commit() {}, async rollback() {}, |
| 105 | + }; |
| 106 | + return { driver, calls }; |
| 107 | +} |
| 108 | + |
| 109 | +async function makeEngine() { |
| 110 | + const engine = new ObjectQL(); |
| 111 | + const { driver, calls } = makeRecordingDriver(); |
| 112 | + engine.registerDriver(driver, true); |
| 113 | + await engine.init(); |
| 114 | + engine.registry.registerObject({ |
| 115 | + name: 'task', |
| 116 | + fields: { title: { type: 'text' }, tenant: { type: 'text' } }, |
| 117 | + } as any); |
| 118 | + return { engine, calls }; |
| 119 | +} |
| 120 | + |
| 121 | +/** |
| 122 | + * Drive the REAL engine and return the one driver call it made — after |
| 123 | + * asserting, through the producer's own decision function, that this call |
| 124 | + * dispatches where the test says it does. Both halves matter: the whole point |
| 125 | + * of #6435 is that the DISPATCH was already right and only the PAYLOAD was not, |
| 126 | + * so a test that only looked at the payload could not tell a fixed strip from a |
| 127 | + * silently re-routed call. |
| 128 | + */ |
| 129 | +async function observeWrite( |
| 130 | + data: Record<string, unknown>, |
| 131 | + options: EngineUpdateOptions | undefined, |
| 132 | + expect_: { fn: 'update' | 'updateMany'; boundId?: unknown }, |
| 133 | +): Promise<RecordedCall> { |
| 134 | + const dispatch = assertEngineUpdateDispatch(data, options); |
| 135 | + expect(dispatch.kind, 'dispatch verdict').toBe(expect_.fn === 'update' ? 'by-id' : 'multi'); |
| 136 | + if (dispatch.kind === 'by-id') expect(dispatch.id, 'bound primary key').toEqual(expect_.boundId); |
| 137 | + |
| 138 | + const { engine, calls } = await makeEngine(); |
| 139 | + await engine.update('task', data, options); |
| 140 | + expect(calls.map((c) => c.fn), 'driver entry points reached').toEqual([expect_.fn]); |
| 141 | + if (expect_.fn === 'update') expect(calls[0].id, 'id argument').toEqual(expect_.boundId); |
| 142 | + return calls[0]; |
| 143 | +} |
| 144 | + |
| 145 | +/** Own-property, never `in`: `Object.prototype` has no `id`, but say what we mean. */ |
| 146 | +function hasIdKey(payload: Record<string, unknown>): boolean { |
| 147 | + return Object.prototype.hasOwnProperty.call(payload, 'id'); |
| 148 | +} |
| 149 | + |
| 150 | +describe('#6435 — a by-id update strips a ruled-not-an-id `data.id` from the SET payload', () => { |
| 151 | + it('the PROBE shape: operator-object data.id + scalar where.id — dispatch still binds rec_1, the payload no longer carries the operator', async () => { |
| 152 | + const call = await observeWrite( |
| 153 | + { id: { $in: ['a', 'b'] }, title: 'x' }, |
| 154 | + { where: { id: 'rec_1' } }, |
| 155 | + { fn: 'update', boundId: 'rec_1' }, |
| 156 | + ); |
| 157 | + // The regression itself: before the fix this payload was |
| 158 | + // `{ id: { $in: ['a','b'] }, title: 'x' }` and driver-sql wrote the |
| 159 | + // serialized operator object into rec_1's primary-key column. |
| 160 | + expect(hasIdKey(call.data), `SET payload was ${JSON.stringify(call.data)}`).toBe(false); |
| 161 | + // ...and the strip takes ONLY `id` — the column the caller actually meant |
| 162 | + // to write still lands, unchanged. |
| 163 | + expect(call.data).toEqual({ title: 'x' }); |
| 164 | + }); |
| 165 | + |
| 166 | + it('array data.id + scalar where.id — same strip, same surviving columns', async () => { |
| 167 | + const call = await observeWrite( |
| 168 | + { id: ['a', 'b'], title: 'x' }, |
| 169 | + { where: { id: 'rec_1' } }, |
| 170 | + { fn: 'update', boundId: 'rec_1' }, |
| 171 | + ); |
| 172 | + expect(hasIdKey(call.data)).toBe(false); |
| 173 | + expect(call.data).toEqual({ title: 'x' }); |
| 174 | + }); |
| 175 | + |
| 176 | + it('null data.id + scalar where.id — stripped, not written as a NULL primary key', async () => { |
| 177 | + // The entry point #6435's body names as the more common one, and the |
| 178 | + // dispatch ladder puts it squarely in the strip set: `asScalarId(null)` is |
| 179 | + // `undefined`, so the decision falls through to `where.id` and binds THAT — |
| 180 | + // the payload's `null` is a ruled-not-an-id value like any other. |
| 181 | + // |
| 182 | + // Reachability, read from source rather than asserted: the REST PATCH route |
| 183 | + // (`packages/rest/src/rest-server.ts`, `PATCH /data/:object/:id`) strips |
| 184 | + // only `expectedVersion` from the body, and `UpdateDataRequestSchema` |
| 185 | + // types `data` as `z.record(z.string(), z.unknown())`, which admits `null`; |
| 186 | + // the protocol's `updateData` (`packages/metadata-protocol/src/protocol.ts`) |
| 187 | + // then calls `engine.update(object, request.data, { where: { id } })` with |
| 188 | + // the body VERBATIM. So a client that GETs a row, edits two fields and PUTs |
| 189 | + // the whole document back — with `id` serialized as `null` — lands exactly |
| 190 | + // here. That read is a static one; no end-to-end HTTP repro was run. |
| 191 | + const call = await observeWrite( |
| 192 | + { id: null, title: 'x' }, |
| 193 | + { where: { id: 'rec_1' } }, |
| 194 | + { fn: 'update', boundId: 'rec_1' }, |
| 195 | + ); |
| 196 | + expect(hasIdKey(call.data)).toBe(false); |
| 197 | + expect(call.data).toEqual({ title: 'x' }); |
| 198 | + }); |
| 199 | + |
| 200 | + it('a by-id update that never carried an id is untouched', async () => { |
| 201 | + const call = await observeWrite( |
| 202 | + { title: 'x' }, |
| 203 | + { where: { id: 'rec_1' } }, |
| 204 | + { fn: 'update', boundId: 'rec_1' }, |
| 205 | + ); |
| 206 | + expect(call.data).toEqual({ title: 'x' }); |
| 207 | + }); |
| 208 | + |
| 209 | + it('does not mutate the payload object the CALLER handed in', async () => { |
| 210 | + // The strip copies, like every other strip on this path (#5591 / #6343). |
| 211 | + // A caller that reuses its payload object across a loop must see what it |
| 212 | + // wrote — and `Object.is` says so too: the engine's payload is a DIFFERENT |
| 213 | + // object, not the caller's with a key deleted out from under it. |
| 214 | + const { engine } = await makeEngine(); |
| 215 | + const callerPayload: Record<string, unknown> = { id: { $in: ['a', 'b'] }, title: 'x' }; |
| 216 | + const snapshot = callerPayload; |
| 217 | + await engine.update('task', callerPayload, { where: { id: 'rec_1' } }); |
| 218 | + expect(Object.is(callerPayload, snapshot), 'caller kept its own object').toBe(true); |
| 219 | + expect(callerPayload).toEqual({ id: { $in: ['a', 'b'] }, title: 'x' }); |
| 220 | + }); |
| 221 | +}); |
| 222 | + |
| 223 | +describe('#6435 — the falsy scalars keep the #5747 / #5748 dispatch semantics', () => { |
| 224 | + // NOT a new verdict. `0` and `''` are scalars, so they take the scalar branch |
| 225 | + // of the id test and then fail its TRUTHINESS half — the engine branches on |
| 226 | + // `if (hookContext.input.id)` and always has (the dispatch module's header |
| 227 | + // point 3). So `data: { id: 0 }` beside `where: { id: 'rec_1' }` falls |
| 228 | + // through to `where` and binds `rec_1`, before this change and after it. |
| 229 | + // |
| 230 | + // What DOES change is the payload, on the same argument as the operator |
| 231 | + // object: the dispatch has ruled this value is not the primary key of the row |
| 232 | + // being written, so `SET id = 0` would replace rec_1's key with `0`. Leaving |
| 233 | + // falsy scalars in while stripping operator objects would be a SECOND rule |
| 234 | + // about one fact — the shape #4550 / #4434 exist to prevent, and the same |
| 235 | + // call PR #6433 made for the multi arm. |
| 236 | + for (const falsy of [0, ''] as const) { |
| 237 | + it(`data.id = ${JSON.stringify(falsy)} beside a scalar where.id still binds rec_1 (verdict unchanged) and is stripped`, async () => { |
| 238 | + const call = await observeWrite( |
| 239 | + { id: falsy, title: 'x' }, |
| 240 | + { where: { id: 'rec_1' } }, |
| 241 | + { fn: 'update', boundId: 'rec_1' }, |
| 242 | + ); |
| 243 | + expect(hasIdKey(call.data)).toBe(false); |
| 244 | + expect(call.data).toEqual({ title: 'x' }); |
| 245 | + }); |
| 246 | + } |
| 247 | +}); |
| 248 | + |
| 249 | +describe('#6435 — the contrast pins: what this change deliberately does NOT touch', () => { |
| 250 | + it('a TRUTHY SCALAR data.id is still handed to the driver as sent', async () => { |
| 251 | + // Route A's boundary, pinned. Here the payload's `id` IS the bound key, so |
| 252 | + // the write is a same-value no-op rather than an identity rewrite. If a |
| 253 | + // later change decides to strip this too, THIS is the assertion that must |
| 254 | + // flip — deliberately, with its own reasoning. |
| 255 | + const call = await observeWrite( |
| 256 | + { id: 'rec_1', title: 'x' }, |
| 257 | + { where: { id: 'rec_1' } }, |
| 258 | + { fn: 'update', boundId: 'rec_1' }, |
| 259 | + ); |
| 260 | + expect(call.data).toEqual({ id: 'rec_1', title: 'x' }); |
| 261 | + }); |
| 262 | + |
| 263 | + it('a truthy scalar data.id that DISAGREES with where.id still wins, payload as sent', async () => { |
| 264 | + // `ENGINE_UPDATE_DISPATCH_CASES`: "a SCALAR data.id still wins over a |
| 265 | + // scalar where.id" ⇒ bound id `rec_1`, not `rec_2`. Unchanged here. |
| 266 | + const call = await observeWrite( |
| 267 | + { id: 'rec_1', title: 'x' }, |
| 268 | + { where: { id: 'rec_2' } }, |
| 269 | + { fn: 'update', boundId: 'rec_1' }, |
| 270 | + ); |
| 271 | + expect(call.data).toEqual({ id: 'rec_1', title: 'x' }); |
| 272 | + }); |
| 273 | + |
| 274 | + it('the MULTI arm is exactly as PR #6433 left it', async () => { |
| 275 | + // The sibling half, re-measured from this file so a regression in either |
| 276 | + // arm shows up next to the other. #6433's own suite is the primary pin. |
| 277 | + const call = await observeWrite( |
| 278 | + { id: { $in: ['a', 'b'] }, title: 'x' }, |
| 279 | + { multi: true }, |
| 280 | + { fn: 'updateMany' }, |
| 281 | + ); |
| 282 | + expect(hasIdKey(call.data)).toBe(false); |
| 283 | + expect(call.data).toEqual({ title: 'x' }); |
| 284 | + expect(call.ast).toEqual({ object: 'task' }); |
| 285 | + }); |
| 286 | + |
| 287 | + it('a multi update selecting rows by an id SET keeps its predicate and its payload', async () => { |
| 288 | + const call = await observeWrite( |
| 289 | + { title: 'x' }, |
| 290 | + { where: { id: { $in: ['a', 'b'] } }, multi: true }, |
| 291 | + { fn: 'updateMany' }, |
| 292 | + ); |
| 293 | + expect(call.data).toEqual({ title: 'x' }); |
| 294 | + expect(call.ast).toEqual({ object: 'task', where: { id: { $in: ['a', 'b'] } } }); |
| 295 | + }); |
| 296 | +}); |
0 commit comments