fix(objectql): multi update 的 SET 载荷剥掉非 id 的 data.id (#6262)

.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 协议响应两处消费者,不该搭引擎修复的车。

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<string, unknown>;
+}
+
+/** 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<string, unknown>) { return { id: 'r1', ...data }; },
+    async update(_o: string, id: string, data: Record<string, unknown>) {
+      calls.push({ fn: 'update', id, data: { ...data } });
+      return { id, ...data };
+    },
+    async updateMany(_o: string, ast: unknown, data: Record<string, unknown>) {
+      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<RecordedCall> {
+  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<string, unknown>): 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<string, unknown> = { 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' });
+  });
+});

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<string, unknown>, 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<string, unknown> | 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<string, unknown>, opCtx.context, hookContext.input.options);
                normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
                validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation });