|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | +// |
| 3 | +// objectstack#6262 — a `multi: true` update must not hand the driver an `id` |
| 4 | +// in the SET payload. |
| 5 | +// |
| 6 | +// ## The shape |
| 7 | +// |
| 8 | +// `update(o, { id: { $in: ['a','b'] }, title: 'x' }, { multi: true })` has |
| 9 | +// dispatched correctly since objectstack#5748 / PR #5919: an operator object is |
| 10 | +// not a primary key, so it stops shadowing the ladder and the declared bulk |
| 11 | +// intent is honoured — `driver.updateMany`. What #5748 did NOT do is clean the |
| 12 | +// PAYLOAD. The measured probe on `origin/main` (#6262's issue body): |
| 13 | +// |
| 14 | +// ``` |
| 15 | +// updateMany( |
| 16 | +// { object: 'probe_task' }, |
| 17 | +// { id: { $in: ['a','b'] }, title: 'x' }, <-- the SET clause |
| 18 | +// ) |
| 19 | +// ``` |
| 20 | +// |
| 21 | +// i.e. the driver is asked to write a serialized operator object into the |
| 22 | +// PRIMARY-KEY column of every matched row. Five backends would each answer that |
| 23 | +// differently (the #5240 / #4434 family), and on the ones that accept it every |
| 24 | +// matched row loses its identity. |
| 25 | +// |
| 26 | +// ## Why the fix is a strip and not a rejection |
| 27 | +// |
| 28 | +// Route B ("reject the whole call") would reverse a verdict |
| 29 | +// `ENGINE_UPDATE_DISPATCH_CASES` states today — |
| 30 | +// `operator object in data.id WITH multi:true` expects `'multi'` — i.e. a |
| 31 | +// partial rollback of #5748's ruling A, which needs a fresh decision. Route A |
| 32 | +// changes NO verdict: the dispatch already answered "this `data.id` is not a |
| 33 | +// primary key", and the strip is nothing more than that same answer applied to |
| 34 | +// the payload — a value the engine has ruled is not an id has no business |
| 35 | +// sitting in the id column either. One question, one answer (#4550 / #4434). |
| 36 | +// |
| 37 | +// ## The rule, stated once |
| 38 | +// |
| 39 | +// Reaching the `multi` branch AT ALL means `resolveEngineUpdateDispatch` |
| 40 | +// returned `{ kind: 'multi' }`, which means it found no scalar truthy id in |
| 41 | +// EITHER source. So every `id` a payload can carry into this branch — an |
| 42 | +// operator object, an array, `null`, a falsy scalar — is a value the dispatch |
| 43 | +// has already ruled is not a primary key. There is no reachable shape where a |
| 44 | +// bulk SET clause legitimately carries `id`: a truthy scalar `data.id` outranks |
| 45 | +// both `where` and `multi` and never gets here (pinned below), and N rows |
| 46 | +// cannot share one primary key anyway. Hence one rule with no exceptions, |
| 47 | +// rather than a second rule for each shape. |
| 48 | + |
| 49 | +import { describe, it, expect } from 'vitest'; |
| 50 | +import { ObjectQL } from './engine.js'; |
| 51 | +import { resolveEngineUpdateDispatch } from './engine-update-dispatch.js'; |
| 52 | + |
| 53 | +interface RecordedCall { |
| 54 | + readonly fn: 'update' | 'updateMany'; |
| 55 | + readonly id?: unknown; |
| 56 | + readonly ast?: unknown; |
| 57 | + /** A COPY — the engine may keep mutating its own payload after the call. */ |
| 58 | + readonly data: Record<string, unknown>; |
| 59 | +} |
| 60 | + |
| 61 | +/** Records the exact SET payload each driver entry point received. */ |
| 62 | +function makeRecordingDriver() { |
| 63 | + const calls: RecordedCall[] = []; |
| 64 | + const driver: any = { |
| 65 | + name: 'recording', |
| 66 | + version: '0.0.0', |
| 67 | + supports: {}, |
| 68 | + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, |
| 69 | + async find() { return []; }, |
| 70 | + async findOne() { return null; }, |
| 71 | + async create(_o: string, data: Record<string, unknown>) { return { id: 'r1', ...data }; }, |
| 72 | + async update(_o: string, id: string, data: Record<string, unknown>) { |
| 73 | + calls.push({ fn: 'update', id, data: { ...data } }); |
| 74 | + return { id, ...data }; |
| 75 | + }, |
| 76 | + async updateMany(_o: string, ast: unknown, data: Record<string, unknown>) { |
| 77 | + calls.push({ fn: 'updateMany', ast, data: { ...data } }); |
| 78 | + return 2; |
| 79 | + }, |
| 80 | + async delete() { return true; }, |
| 81 | + async deleteMany() { return 0; }, |
| 82 | + async count() { return 0; }, |
| 83 | + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, |
| 84 | + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, |
| 85 | + async commit() {}, async rollback() {}, |
| 86 | + }; |
| 87 | + return { driver, calls }; |
| 88 | +} |
| 89 | + |
| 90 | +async function makeEngine() { |
| 91 | + const engine = new ObjectQL(); |
| 92 | + const { driver, calls } = makeRecordingDriver(); |
| 93 | + engine.registerDriver(driver, true); |
| 94 | + await engine.init(); |
| 95 | + engine.registry.registerObject({ |
| 96 | + name: 'task', |
| 97 | + fields: { title: { type: 'text' }, tenant: { type: 'text' } }, |
| 98 | + } as any); |
| 99 | + return { engine, calls }; |
| 100 | +} |
| 101 | + |
| 102 | +/** The one driver call the engine made, asserted to be the expected entry point. */ |
| 103 | +async function observeWrite( |
| 104 | + data: unknown, |
| 105 | + options: unknown, |
| 106 | + expectFn: 'update' | 'updateMany', |
| 107 | +): Promise<RecordedCall> { |
| 108 | + const { engine, calls } = await makeEngine(); |
| 109 | + await engine.update('task', data as any, options as any); |
| 110 | + expect(calls.map((c) => c.fn), 'driver entry points reached').toEqual([expectFn]); |
| 111 | + return calls[0]; |
| 112 | +} |
| 113 | + |
| 114 | +/** Own-property, never `in`: `Object.prototype` has no `id`, but say what we mean. */ |
| 115 | +function hasIdKey(payload: Record<string, unknown>): boolean { |
| 116 | + return Object.prototype.hasOwnProperty.call(payload, 'id'); |
| 117 | +} |
| 118 | + |
| 119 | +describe('#6262 — a multi update strips a non-id `data.id` from the SET payload', () => { |
| 120 | + it('the PROBE shape: operator-object data.id + multi:true reaches updateMany with NO id in the payload', async () => { |
| 121 | + const call = await observeWrite({ id: { $in: ['a', 'b'] }, title: 'x' }, { multi: true }, 'updateMany'); |
| 122 | + // The regression itself: before the fix this payload was |
| 123 | + // `{ id: { $in: ['a','b'] }, title: 'x' }` and the driver was asked to |
| 124 | + // write the operator object into the primary-key column. |
| 125 | + expect(hasIdKey(call.data), `SET payload was ${JSON.stringify(call.data)}`).toBe(false); |
| 126 | + // ...and the strip takes ONLY `id` — the column the caller actually meant |
| 127 | + // to write still lands, unchanged. |
| 128 | + expect(call.data).toEqual({ title: 'x' }); |
| 129 | + }); |
| 130 | + |
| 131 | + it('array data.id + multi:true — same strip, same surviving columns', async () => { |
| 132 | + const call = await observeWrite({ id: ['a', 'b'], title: 'x' }, { multi: true }, 'updateMany'); |
| 133 | + expect(hasIdKey(call.data)).toBe(false); |
| 134 | + expect(call.data).toEqual({ title: 'x' }); |
| 135 | + }); |
| 136 | + |
| 137 | + it('null data.id + multi:true — stripped, not written as a NULL primary key', async () => { |
| 138 | + const call = await observeWrite({ id: null, title: 'x' }, { multi: true }, 'updateMany'); |
| 139 | + expect(hasIdKey(call.data)).toBe(false); |
| 140 | + expect(call.data).toEqual({ title: 'x' }); |
| 141 | + }); |
| 142 | + |
| 143 | + it('a multi update that never carried an id is untouched', async () => { |
| 144 | + const call = await observeWrite({ title: 'x' }, { where: { tenant: 't1' }, multi: true }, 'updateMany'); |
| 145 | + expect(call.data).toEqual({ title: 'x' }); |
| 146 | + // The row-scoping AST is what targets the rows, and it is unaffected. |
| 147 | + expect(call.ast).toEqual({ object: 'task', where: { tenant: 't1' } }); |
| 148 | + }); |
| 149 | + |
| 150 | + it('an $in over `where.id` still targets rows through the AST, with the payload unchanged', async () => { |
| 151 | + const call = await observeWrite( |
| 152 | + { title: 'x' }, |
| 153 | + { where: { id: { $in: ['a', 'b'] } }, multi: true }, |
| 154 | + 'updateMany', |
| 155 | + ); |
| 156 | + expect(call.data).toEqual({ title: 'x' }); |
| 157 | + expect(call.ast).toEqual({ object: 'task', where: { id: { $in: ['a', 'b'] } } }); |
| 158 | + }); |
| 159 | + |
| 160 | + it('does not mutate the payload object the CALLER handed in', async () => { |
| 161 | + const { engine } = await makeEngine(); |
| 162 | + const callerPayload: Record<string, unknown> = { id: { $in: ['a', 'b'] }, title: 'x' }; |
| 163 | + await engine.update('task', callerPayload as any, { multi: true } as any); |
| 164 | + // The strip copies, like every other strip on this path. A caller that |
| 165 | + // reuses its payload object (a loop over tenants) must see what it wrote. |
| 166 | + expect(callerPayload).toEqual({ id: { $in: ['a', 'b'] }, title: 'x' }); |
| 167 | + }); |
| 168 | +}); |
| 169 | + |
| 170 | +describe('#6262 — the falsy scalars keep the #5747 / #5748 dispatch semantics', () => { |
| 171 | + // These are NOT a new verdict. `0` and `''` are scalars, so they take the |
| 172 | + // scalar branch of the id test and then fail its TRUTHINESS half — the engine |
| 173 | + // branches on `if (hookContext.input.id)` and always has (the dispatch |
| 174 | + // module's header point 3, and objectstack#5747 on the delete twin, whose |
| 175 | + // option B — "make `{ id: 0 }` really delete by id" — was explicitly not |
| 176 | + // taken). So the verdict here is `multi`, before this change and after it, |
| 177 | + // and `ENGINE_UPDATE_DISPATCH_CASES` says so in its own row. |
| 178 | + // |
| 179 | + // What DOES change is the payload, on exactly the argument above: the |
| 180 | + // dispatch has ruled this value is not a primary key, so writing it into the |
| 181 | + // primary-key column of N rows is the same defect as the operator object, |
| 182 | + // only quieter — a driver that accepts `id = 0` collapses every matched row |
| 183 | + // onto one key instead of erroring. Leaving falsy scalars in while stripping |
| 184 | + // operator objects would be a SECOND rule about the same fact, which is the |
| 185 | + // shape #4550 / #4434 exist to prevent. |
| 186 | + for (const falsy of [0, ''] as const) { |
| 187 | + it(`data.id = ${JSON.stringify(falsy)} with multi:true still dispatches multi (verdict unchanged)`, async () => { |
| 188 | + expect(resolveEngineUpdateDispatch({ id: falsy, title: 'x' }, { multi: true }).kind).toBe('multi'); |
| 189 | + const call = await observeWrite({ id: falsy, title: 'x' }, { multi: true }, 'updateMany'); |
| 190 | + expect(hasIdKey(call.data)).toBe(false); |
| 191 | + expect(call.data).toEqual({ title: 'x' }); |
| 192 | + }); |
| 193 | + } |
| 194 | +}); |
| 195 | + |
| 196 | +describe('#6262 — the by-id path is untouched', () => { |
| 197 | + it('a scalar data.id outranks multi:true and reaches driver.update with the payload AS SENT', async () => { |
| 198 | + const call = await observeWrite({ id: 'rec_1', title: 'x' }, { multi: true }, 'update'); |
| 199 | + expect(call.id).toBe('rec_1'); |
| 200 | + // The by-id branch has always handed the driver the payload including |
| 201 | + // `id`, and #6262 is scoped to the multi branch: `driver.update` is given |
| 202 | + // the primary key SEPARATELY, so the key in the payload is redundant, not |
| 203 | + // damaging. Pinned so a future widening of the strip is a deliberate act. |
| 204 | + expect(call.data).toEqual({ id: 'rec_1', title: 'x' }); |
| 205 | + }); |
| 206 | + |
| 207 | + it('a scalar where.id reaches driver.update with the payload AS SENT', async () => { |
| 208 | + const call = await observeWrite({ title: 'x' }, { where: { id: 'rec_1' } }, 'update'); |
| 209 | + expect(call.id).toBe('rec_1'); |
| 210 | + expect(call.data).toEqual({ title: 'x' }); |
| 211 | + }); |
| 212 | + |
| 213 | + it('operator data.id BESIDE a scalar where.id: the where id wins, and the operator does not reach the payload column', async () => { |
| 214 | + // #5748's headline shape — verdict `by-id`, bound id `rec_1`. The payload |
| 215 | + // still carries the operator object here, because this is the by-id branch |
| 216 | + // and the primary key travels in its own argument; the row's identity is |
| 217 | + // never taken from the payload. What #6262 fixes is only the branch where |
| 218 | + // the payload IS the SET clause. |
| 219 | + const call = await observeWrite( |
| 220 | + { id: { $in: ['a', 'b'] }, title: 'x' }, |
| 221 | + { where: { id: 'rec_1' } }, |
| 222 | + 'update', |
| 223 | + ); |
| 224 | + expect(call.id).toBe('rec_1'); |
| 225 | + expect(call.data).toEqual({ id: { $in: ['a', 'b'] }, title: 'x' }); |
| 226 | + }); |
| 227 | +}); |
0 commit comments