From eb6333bc4b831d1661a9e1bd6428bffcd300afdf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:23:18 +0000 Subject: [PATCH] fix(objectql): a multi update strips a non-id `data.id` from the SET payload (#6262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update(o, { id: { $in: ['a','b'] }, title: 'x' }, { multi: true })` has dispatched correctly since #5748 / PR #5919 — an operator object is not a primary key, so it stops shadowing the ladder and the declared bulk intent is honoured (`driver.updateMany`). What that fix did not do is clean the PAYLOAD. Measured on origin/main with a recording driver over the real engine: updateMany({ object: 'probe_task' }, { id: { $in: ['a','b'] }, title: 'x' }) i.e. the driver is asked to write a serialized operator object into the primary-key column of every matched row. Five backends would each answer that differently (the #5240 / #4434 family), and on the ones that accept it the matched rows lose their identity irreversibly. Reaching the multi branch AT ALL means `resolveEngineUpdateDispatch` returned `multi`, i.e. it found no scalar truthy id in EITHER source — so whatever sits in `data.id` there is a value the engine has already RULED is not a primary key. The strip is that same answer applied one layer on, not a second opinion: a value that is not the primary key does not get to sit in the primary-key column either. - Zero verdict change: `ENGINE_UPDATE_DISPATCH_CASES` is untouched and `operator object in data.id WITH multi:true` still expects 'multi'. Rejecting the call instead (#6262 route B) would reverse that just-landed case — a partial rollback of #5748's ruling A, which needs a fresh decision. - No reachable legitimate write is lost: a truthy scalar `data.id` outranks both `where` and `multi` and never reaches this branch, and N rows cannot share one primary key anyway. - The by-id path is unchanged and pinned as-is: `driver.update` takes the primary key in its own argument, so the key in the payload is redundant rather than damaging. - Falsy scalars keep the #5747 / #5748 dispatch semantics (still 'multi') and are stripped on the same argument — stripping operator objects while leaving `{ id: 0 }` in would be a second rule about one fact. The drop logs at warn, naming the consequence and both correct spellings. Deliberately not routed through `onFieldsDropped`: `DroppedFieldsEvent.reason` is a closed enum over the two read-only strips (#3407 / #3042), and widening that vocabulary is a `packages/spec` change with its own consumers. Fixes #6262 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../engine-update-multi-payload-id-strip.md | 23 ++ .../engine-update-multi-payload-id.test.ts | 227 ++++++++++++++++++ packages/objectql/src/engine.ts | 51 ++++ 3 files changed, 301 insertions(+) create mode 100644 .changeset/engine-update-multi-payload-id-strip.md create mode 100644 packages/objectql/src/engine-update-multi-payload-id.test.ts diff --git a/.changeset/engine-update-multi-payload-id-strip.md b/.changeset/engine-update-multi-payload-id-strip.md new file mode 100644 index 0000000000..28539d65e3 --- /dev/null +++ b/.changeset/engine-update-multi-payload-id-strip.md @@ -0,0 +1,23 @@ +--- +'@objectstack/objectql': patch +--- + +`multi: true` 更新的 SET 载荷不再携带 `id`——算子对象不会再被写进每一行的主键列 + +`update(o, { id: { $in: ['a','b'] }, title: 'x' }, { multi: true })` 的**派发**自 #5748 裁 A / PR #5919 起就是对的:算子对象不是主键,不再遮蔽派发阶梯,声明的 bulk intent 照做,调用落到 `driver.updateMany`。#5919 没有做、#5922 也按 PD #10 明确留在范围外的,是**载荷**那一半。实测(origin/main,记录型 driver 驱动真实引擎): + +``` +updateMany({ object: 'probe_task' }, { "id": { "$in": ["a","b"] }, "title": "x" }) + ^^^^^^^^^^^^^^^^^^^^^^^^ 这是 SET 子句 +``` + +即驱动被要求把一个序列化的算子对象写进**每一条命中行**的主键列。五个后端会对这件事各给一个答案(#5240 / #4434 家族),而在接受它的后端上,命中行的身份不可逆地丢失。 + +修法是**剥离**:走到 multi 分支本身就意味着 `resolveEngineUpdateDispatch` 答了 `multi`,即它在**两个** id 来源里都没找到真值标量 id——所以此刻 `data.id` 里的任何东西(算子对象、数组、`null`、假值标量)都是引擎**已经裁定不是主键**的值。同一个问题的同一个答案,只是多用在一层上:不是主键的东西,也就不该坐在主键列上。 + +- **零 verdict 变更**:`ENGINE_UPDATE_DISPATCH_CASES` 一行未动,`operator object in data.id WITH multi:true` 仍是 `'multi'`,`engine-update-dispatch.test.ts` 全绿。响亮拒绝(#6262 的 B 案)要反转这条刚落地的 case,属对 #5748 裁 A 的部分回退,需要新裁决,不在本次范围。 +- **无可达的合法写入被吞掉**:真值标量 `data.id` 压过 `where` 与 `multi`,根本到不了这个分支;而 N 行也不可能共用一个主键。 +- **单 id 路径零变化**:`driver.update(object, id, data, …)` 的主键走的是独立参数,载荷里的 `id` 只是冗余而非破坏,本次不动(已按现状钉死)。 +- **假值标量同判**:`{ id: 0 }` / `{ id: '' }` 的**判定语义**按 #5747 / #5748 原样不变(仍是 `multi`),载荷同样剥离——把算子对象剥掉却把假值标量留下,等于对同一个事实立第二条规则,正是 `engine-update-dispatch.ts` 这一族被抽出来防止的事。 + +被剥离时按 `warn` 记一条日志,点明后果与两种正确写法(单行按 id 更新 / 用 `where` 选行集)。刻意**不**走 `onFieldsDropped`:`DroppedFieldsEvent.reason` 是 `readonly` / `readonly_when` 两值的闭合枚举(#3407 / #3042),扩这个词表是 `packages/spec` 的改动、有 batch 与 REST 协议响应两处消费者,不该搭引擎修复的车。 diff --git a/packages/objectql/src/engine-update-multi-payload-id.test.ts b/packages/objectql/src/engine-update-multi-payload-id.test.ts new file mode 100644 index 0000000000..20199af099 --- /dev/null +++ b/packages/objectql/src/engine-update-multi-payload-id.test.ts @@ -0,0 +1,227 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// objectstack#6262 — a `multi: true` update must not hand the driver an `id` +// in the SET payload. +// +// ## The shape +// +// `update(o, { id: { $in: ['a','b'] }, title: 'x' }, { multi: true })` has +// dispatched correctly since objectstack#5748 / PR #5919: an operator object is +// not a primary key, so it stops shadowing the ladder and the declared bulk +// intent is honoured — `driver.updateMany`. What #5748 did NOT do is clean the +// PAYLOAD. The measured probe on `origin/main` (#6262's issue body): +// +// ``` +// updateMany( +// { object: 'probe_task' }, +// { id: { $in: ['a','b'] }, title: 'x' }, <-- the SET clause +// ) +// ``` +// +// i.e. the driver is asked to write a serialized operator object into the +// PRIMARY-KEY column of every matched row. Five backends would each answer that +// differently (the #5240 / #4434 family), and on the ones that accept it every +// matched row loses its identity. +// +// ## Why the fix is a strip and not a rejection +// +// Route B ("reject the whole call") would reverse a verdict +// `ENGINE_UPDATE_DISPATCH_CASES` states today — +// `operator object in data.id WITH multi:true` expects `'multi'` — i.e. a +// partial rollback of #5748's ruling A, which needs a fresh decision. Route A +// changes NO verdict: the dispatch already answered "this `data.id` is not a +// primary key", and the strip is nothing more than that same answer applied to +// the payload — a value the engine has ruled is not an id has no business +// sitting in the id column either. One question, one answer (#4550 / #4434). +// +// ## The rule, stated once +// +// Reaching the `multi` branch AT ALL means `resolveEngineUpdateDispatch` +// returned `{ kind: 'multi' }`, which means it found no scalar truthy id in +// EITHER source. So every `id` a payload can carry into this branch — an +// operator object, an array, `null`, a falsy scalar — is a value the dispatch +// has already ruled is not a primary key. There is no reachable shape where a +// bulk SET clause legitimately carries `id`: a truthy scalar `data.id` outranks +// both `where` and `multi` and never gets here (pinned below), and N rows +// cannot share one primary key anyway. Hence one rule with no exceptions, +// rather than a second rule for each shape. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { resolveEngineUpdateDispatch } from './engine-update-dispatch.js'; + +interface RecordedCall { + readonly fn: 'update' | 'updateMany'; + readonly id?: unknown; + readonly ast?: unknown; + /** A COPY — the engine may keep mutating its own payload after the call. */ + readonly data: Record; +} + +/** Records the exact SET payload each driver entry point received. */ +function makeRecordingDriver() { + const calls: RecordedCall[] = []; + const driver: any = { + name: 'recording', + version: '0.0.0', + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return []; }, + async findOne() { return null; }, + async create(_o: string, data: Record) { return { id: 'r1', ...data }; }, + async update(_o: string, id: string, data: Record) { + calls.push({ fn: 'update', id, data: { ...data } }); + return { id, ...data }; + }, + async updateMany(_o: string, ast: unknown, data: Record) { + calls.push({ fn: 'updateMany', ast, data: { ...data } }); + return 2; + }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return 0; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, calls }; +} + +async function makeEngine() { + const engine = new ObjectQL(); + const { driver, calls } = makeRecordingDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'task', + fields: { title: { type: 'text' }, tenant: { type: 'text' } }, + } as any); + return { engine, calls }; +} + +/** The one driver call the engine made, asserted to be the expected entry point. */ +async function observeWrite( + data: unknown, + options: unknown, + expectFn: 'update' | 'updateMany', +): Promise { + const { engine, calls } = await makeEngine(); + await engine.update('task', data as any, options as any); + expect(calls.map((c) => c.fn), 'driver entry points reached').toEqual([expectFn]); + return calls[0]; +} + +/** Own-property, never `in`: `Object.prototype` has no `id`, but say what we mean. */ +function hasIdKey(payload: Record): boolean { + return Object.prototype.hasOwnProperty.call(payload, 'id'); +} + +describe('#6262 — a multi update strips a non-id `data.id` from the SET payload', () => { + it('the PROBE shape: operator-object data.id + multi:true reaches updateMany with NO id in the payload', async () => { + const call = await observeWrite({ id: { $in: ['a', 'b'] }, title: 'x' }, { multi: true }, 'updateMany'); + // The regression itself: before the fix this payload was + // `{ id: { $in: ['a','b'] }, title: 'x' }` and the driver was asked to + // write the operator object into the primary-key column. + expect(hasIdKey(call.data), `SET payload was ${JSON.stringify(call.data)}`).toBe(false); + // ...and the strip takes ONLY `id` — the column the caller actually meant + // to write still lands, unchanged. + expect(call.data).toEqual({ title: 'x' }); + }); + + it('array data.id + multi:true — same strip, same surviving columns', async () => { + const call = await observeWrite({ id: ['a', 'b'], title: 'x' }, { multi: true }, 'updateMany'); + expect(hasIdKey(call.data)).toBe(false); + expect(call.data).toEqual({ title: 'x' }); + }); + + it('null data.id + multi:true — stripped, not written as a NULL primary key', async () => { + const call = await observeWrite({ id: null, title: 'x' }, { multi: true }, 'updateMany'); + expect(hasIdKey(call.data)).toBe(false); + expect(call.data).toEqual({ title: 'x' }); + }); + + it('a multi update that never carried an id is untouched', async () => { + const call = await observeWrite({ title: 'x' }, { where: { tenant: 't1' }, multi: true }, 'updateMany'); + expect(call.data).toEqual({ title: 'x' }); + // The row-scoping AST is what targets the rows, and it is unaffected. + expect(call.ast).toEqual({ object: 'task', where: { tenant: 't1' } }); + }); + + it('an $in over `where.id` still targets rows through the AST, with the payload unchanged', async () => { + const call = await observeWrite( + { title: 'x' }, + { where: { id: { $in: ['a', 'b'] } }, multi: true }, + 'updateMany', + ); + expect(call.data).toEqual({ title: 'x' }); + expect(call.ast).toEqual({ object: 'task', where: { id: { $in: ['a', 'b'] } } }); + }); + + it('does not mutate the payload object the CALLER handed in', async () => { + const { engine } = await makeEngine(); + const callerPayload: Record = { id: { $in: ['a', 'b'] }, title: 'x' }; + await engine.update('task', callerPayload as any, { multi: true } as any); + // The strip copies, like every other strip on this path. A caller that + // reuses its payload object (a loop over tenants) must see what it wrote. + expect(callerPayload).toEqual({ id: { $in: ['a', 'b'] }, title: 'x' }); + }); +}); + +describe('#6262 — the falsy scalars keep the #5747 / #5748 dispatch semantics', () => { + // These are NOT a new verdict. `0` and `''` are scalars, so they take the + // scalar branch of the id test and then fail its TRUTHINESS half — the engine + // branches on `if (hookContext.input.id)` and always has (the dispatch + // module's header point 3, and objectstack#5747 on the delete twin, whose + // option B — "make `{ id: 0 }` really delete by id" — was explicitly not + // taken). So the verdict here is `multi`, before this change and after it, + // and `ENGINE_UPDATE_DISPATCH_CASES` says so in its own row. + // + // What DOES change is the payload, on exactly the argument above: the + // dispatch has ruled this value is not a primary key, so writing it into the + // primary-key column of N rows is the same defect as the operator object, + // only quieter — a driver that accepts `id = 0` collapses every matched row + // onto one key instead of erroring. Leaving falsy scalars in while stripping + // operator objects would be a SECOND rule about the same fact, which is the + // shape #4550 / #4434 exist to prevent. + for (const falsy of [0, ''] as const) { + it(`data.id = ${JSON.stringify(falsy)} with multi:true still dispatches multi (verdict unchanged)`, async () => { + expect(resolveEngineUpdateDispatch({ id: falsy, title: 'x' }, { multi: true }).kind).toBe('multi'); + const call = await observeWrite({ id: falsy, title: 'x' }, { multi: true }, 'updateMany'); + expect(hasIdKey(call.data)).toBe(false); + expect(call.data).toEqual({ title: 'x' }); + }); + } +}); + +describe('#6262 — the by-id path is untouched', () => { + it('a scalar data.id outranks multi:true and reaches driver.update with the payload AS SENT', async () => { + const call = await observeWrite({ id: 'rec_1', title: 'x' }, { multi: true }, 'update'); + expect(call.id).toBe('rec_1'); + // The by-id branch has always handed the driver the payload including + // `id`, and #6262 is scoped to the multi branch: `driver.update` is given + // the primary key SEPARATELY, so the key in the payload is redundant, not + // damaging. Pinned so a future widening of the strip is a deliberate act. + expect(call.data).toEqual({ id: 'rec_1', title: 'x' }); + }); + + it('a scalar where.id reaches driver.update with the payload AS SENT', async () => { + const call = await observeWrite({ title: 'x' }, { where: { id: 'rec_1' } }, 'update'); + expect(call.id).toBe('rec_1'); + expect(call.data).toEqual({ title: 'x' }); + }); + + it('operator data.id BESIDE a scalar where.id: the where id wins, and the operator does not reach the payload column', async () => { + // #5748's headline shape — verdict `by-id`, bound id `rec_1`. The payload + // still carries the operator object here, because this is the by-id branch + // and the primary key travels in its own argument; the row's identity is + // never taken from the payload. What #6262 fixes is only the branch where + // the payload IS the SET clause. + const call = await observeWrite( + { id: { $in: ['a', 'b'] }, title: 'x' }, + { where: { id: 'rec_1' } }, + 'update', + ); + expect(call.id).toBe('rec_1'); + expect(call.data).toEqual({ id: { $in: ['a', 'b'] }, title: 'x' }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 125f0b1e5b..b39259f4b6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5963,6 +5963,57 @@ export class ObjectQL implements IObjectQLEngine { ); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); } else if (options?.multi && driver.updateMany) { + // [#6262] A bulk SET clause must not carry `id`. Reaching this + // branch AT ALL means `resolveEngineUpdateDispatch` returned + // `multi`, i.e. it found no scalar truthy id in EITHER source — + // so whatever sits in `data.id` here (an operator object, an + // array, `null`, a falsy scalar) is a value the engine has + // already RULED is not a primary key. Leaving it in the payload + // then asks the driver to write that ruled-not-an-id value into + // the primary-key column of every matched row: the measured + // probe was `updateMany({object}, { id: { $in: ['a','b'] }, + // title: 'x' })`, i.e. a serialized operator object as the new + // primary key of N rows. Five backends would each answer that + // differently (#5240 / #4434), and on the ones that accept it + // the matched rows lose their identity irreversibly. + // + // This is the SAME answer to the SAME question, applied one + // layer on — not a second opinion. #5748 / PR #5919 ruled that a + // non-scalar `data.id` is not an id and therefore stops + // shadowing the dispatch ladder; the declared bulk intent is + // honoured (`ENGINE_UPDATE_DISPATCH_CASES` says `'multi'`, and + // this change leaves every verdict in that set untouched). The + // strip is that ruling's other half: a value that is not the + // primary key does not get to sit in the primary-key column + // either. Rejecting the call instead (#6262's route B) would + // reverse a verdict the case-set states today, which is a fresh + // maintainer decision rather than this fix. + // + // No reachable shape loses a legitimate write: a truthy scalar + // `data.id` outranks both `where` and `multi` and never gets + // here, and N rows cannot share one primary key anyway. + // + // Deliberately NOT reported through `reportDroppedFields`: + // `DroppedFieldsEvent.reason` is a closed enum over the two + // READ-ONLY strips (`readonly` / `readonly_when`, #3407/#3042), + // and this drop is neither. Widening that vocabulary is a + // `packages/spec` change with its own consumers (batch + REST + // protocol responses), not a rider on an engine fix. The `warn` + // is the #4632 duty in the meantime: name the consequence and + // the remedy, since the caller is told the write succeeded. + const preIdMulti = hookContext.input.data as Record | null | undefined; + if (preIdMulti && typeof preIdMulti === 'object' && Object.prototype.hasOwnProperty.call(preIdMulti, 'id')) { + const { id: notAnId, ...withoutId } = preIdMulti; + hookContext.input.data = withoutId as any; + this.logger.warn( + `Bulk update on '${object}': dropped 'id' from the write payload. A multi:true update ` + + `targets rows through its predicate, and the engine has already ruled this value is not a ` + + `primary key (${JSON.stringify(notAnId) ?? String(notAnId)}) — writing it would have ` + + `overwritten the primary-key column of every matched row. To update ONE row by id, pass a ` + + `scalar id (\`update(object, { id, ...fields })\` or \`{ where: { id } }\`) instead of ` + + `options.multi; to SELECT rows by an id set, put it in \`where\` (\`{ where: { id: { $in: [...] } }, multi: true }\`).`, + ); + } await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });