Skip to content

Commit 946a131

Browse files
baozhoutaoclaude
andauthored
fix(metadata-core,objectql): scalar-test data.id in the update dispatch, so a payload operator object is not bound as a primary key (#5748) (#5919)
`ObjectQL.update(object, data, options)` asked "does this call name one row?" of two places under two different rules: `options.where.id` went through a scalar test (an operator object / array / `null` is a multi-row predicate, not an id -- #4434 / #4550), while `data.id` was taken VERBATIM whenever truthy, ahead of both `where` and `options.multi`. So the same operator object was a predicate in `where` and a primary key in the payload: `update(o, { id: { $in: ['a','b'] }, title: 'x' }, { multi: true })` dispatched `by-id` with `{$in: [...]}` bound into `driver.update`'s primary-key position, and the caller's explicit `multi: true` was swallowed with no diagnostic -- declared != enforced, one layer under the `multi` intent key #5393 had just added to the flow `update_record` node. `data.id` now goes through the SAME scalar test as `where.id`, defined once as `asScalarId` and reached by both halves, so a non-scalar payload id names no row and stops shadowing the ladder below it: it falls through to `where.id`, then `multi`, then `reject`. A scalar `data.id` is untouched -- it still outranks `where` and `multi`, which is the common, legal `update(o, { id, ...fields })` spelling every in-repo caller uses. Ruled by the maintainer (2026-08-06) as option A over option B's "reject any non-scalar `data.id`". B's objection -- that an operator object in the payload is most likely a typo the author meant for `where`, and that A would promote it into a real bulk write -- is answered by the ladder rather than by a second error message: with no declared `multi`, a non-scalar `data.id` is the existing loud reject and nothing reaches the driver. That is pinned as its own test. `ENGINE_UPDATE_DISPATCH_CASES` gains an optional `expectId`, because the verdict alone cannot separate "picked an id" from "picked the RIGHT id": an operator `data.id` beside a scalar `where.id` is `by-id` before and after, and only the bound value says which source won. Reverse-verified: restoring the verbatim `data.id` read turns 11 of these red against the real engine. Fixes #5748 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We Co-authored-by: Claude <noreply@anthropic.com>
1 parent 61dc08e commit 946a131

4 files changed

Lines changed: 354 additions & 139 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/metadata-core": patch
3+
"@objectstack/objectql": patch
4+
---
5+
6+
fix(metadata-core,objectql): `ObjectQL.update``data.id` 同过标量测试,不再把载荷里的算子对象当主键 (#5748)
7+
8+
`ObjectQL.update(object, data, options)` 用两处取主键,而这两处此前用的是**两套规则**:
9+
10+
- `options.where.id`**标量测试** —— `{ id: { $in: [...] } }` / `{ id: [...] }` /
11+
`{ id: null }` 是多行谓词,不算 id(#4434 / #4550);
12+
- `data.id` **不做任何测试**,只要为真就原样当主键,并且先于 `where`、也先于
13+
`options.multi`
14+
15+
于是同一个算子对象,写在 `where.id` 里被正确识别为谓词,写在 `data.id` 里却被
16+
当成主键绑进 `driver.update(object, id, …)` 的主键位置,**显式声明的
17+
`multi: true` 被无声忽略**。后果不是数据被覆盖,而是静默失灵或难读的驱动错误:
18+
SQLite 侧报参数绑定错误,别的驱动可能只匹配零行 —— 两种都不会告诉调用方
19+
「你的 `multi` 被忽略了」。这是 declared ≠ enforced 的一种,#5393 刚给 flow 的
20+
`update_record` 补上的 `multi` 批量意图键正是被这条更早的规则盖掉的。
21+
22+
现在 `data.id``where.id` **共用同一个标量测试**(判定在
23+
`packages/metadata-core/src/engine-update-dispatch.ts` 定义一次,`engine.ts`
24+
全部 fake engine 经 `resolveEngineUpdateDispatch` /
25+
`assertEngineUpdateDispatch` 复用同一份)。非标量 `data.id` 不算 id,因此不再
26+
盖住任何东西:判定按 `where.id``multi``reject` 的原有阶梯继续往下走。
27+
28+
**行为矩阵(FROM → TO)。标量 `data.id` 的按 id 写法完全不受影响。**
29+
30+
| 调用 | FROM | TO |
31+
|:---|:---|:---|
32+
| `update(o, { id: 'rec_1', …f })` | by-id `'rec_1'` | **不变** |
33+
| `update(o, { id: 'rec_1', …f }, { multi: true })` | by-id `'rec_1'` | **不变**(标量 `data.id` 仍先于 `multi`) |
34+
| `update(o, { id: 'rec_1', …f }, { where: { id: 'rec_2' } })` | by-id `'rec_1'` | **不变**(标量 `data.id` 仍先于 `where`) |
35+
| `update(o, { id: 0, …f }, { multi: true })` | multi | **不变**(真值判定,`0` 不标识行) |
36+
| `update(o, { id: { $in: [...] }, …f }, { multi: true })` | by-id,算子对象被绑进主键位 | **multi** —— 声明的批量意图被执行 |
37+
| `update(o, { id: ['a','b'], …f }, { multi: true })` | by-id,数组被绑进主键位 | **multi** |
38+
| `update(o, { id: { $in: [...] }, …f })`(**** `multi`) | by-id,算子对象被绑进主键位 | **reject**,消息不变:`Update requires an ID or options.multi=true` |
39+
| `update(o, { id: { $in: [...] }, …f }, { multi: false })` | 同上 | **reject** |
40+
| `update(o, { id: { $in: [...] }, …f }, { where: { id: 'rec_1' } })` | by-id,绑的是**算子对象** | by-id,绑的是 **`'rec_1'`** |
41+
42+
最后一格是这次修复里唯一「判定不变、绑定值变了」的一格 —— 前后都是 `by-id`,
43+
变的是哪一个 id 源胜出。`ENGINE_UPDATE_DISPATCH_CASES` 因此新增可选的
44+
`expectId`,把落进主键位的值本身也钉住,避免用例因为「什么都没产出」而绿。
45+
46+
**「无 `multi` 的非标量 `data.id`」被明确定成响亮拒绝**,不会静默升级成一次真的
47+
批量写 —— 这是裁决(维护者 2026-08-06)对方案 B 那条顾虑的处置:把算子对象写进
48+
载荷大概率是写错了位置,那就报错,而不是替作者决定他想批量写。
49+
50+
无 API 变更:导出符号、类型与 `ENGINE_UPDATE_REJECT_MESSAGE` 的文案均不变。

packages/metadata-core/src/engine-update-dispatch.ts

Lines changed: 96 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@
4545
* this call identify a single row by primary key?* — asked of two places, in
4646
* this order:
4747
*
48-
* - **`data.id`, taken verbatim when truthy** → `by-id`: routes to
49-
* `driver.update(object, id, data, …)`.
50-
* - otherwise, `options.where.id` is a **scalar** (`string` / `number` /
51-
* `bigint`, not `null`) **and truthy** → `by-id`, same route.
48+
* - **`data.id`** is a **scalar** (`string` / `number` / `bigint`, not `null`)
49+
* **and truthy** → `by-id`: routes to `driver.update(object, id, data, …)`.
50+
* - otherwise, `options.where.id` is a **scalar** **and truthy** → `by-id`,
51+
* same route.
5252
* - otherwise, `options.multi` is truthy → `multi`: routes to
5353
* `driver.updateMany` with the middleware-composed AST (#2982).
5454
* - otherwise → **`reject`**. The call names neither one row nor a bulk
@@ -57,23 +57,31 @@
5757
* Three things about that list are load-bearing and easy to get wrong when
5858
* copying it by hand — which is the whole argument for importing it instead:
5959
*
60-
* 1. **The `where.id` scalar test.** `{ id: { $in: [...] } }` / `{ id: [...] }`
61-
* / `{ id: null }` are predicates over many rows. Treating one as an id
62-
* would bind the operator object literally into `driver.update(object,
63-
* {$in: […]}, …)` **and** skip the #2982 row-scoping AST seeding. So they
64-
* are `reject` unless the caller also said `multi`.
65-
* 2. **`data.id` is NOT scalar-tested.** `ObjectQL.update` reads `data.id`
66-
* first and uses it as-is whenever it is truthy, so an operator object
67-
* parked there wins over everything below it — including an explicit
68-
* `multi: true`. This module reports that verdict rather than quietly
69-
* improving on it: a predicate that is *better* than the producer is a
70-
* second opinion, which is exactly what #4550 removed. (The asymmetry
71-
* itself is filed as objectstack#5748; when it is fixed, it is fixed **here
72-
* and in `engine.ts` together**, which is now one edit instead of two.)
60+
* 1. **The scalar test, on BOTH id sources.** `{ id: { $in: [...] } }` /
61+
* `{ id: [...] }` / `{ id: null }` are predicates over many rows. Treating
62+
* one as an id would bind the operator object literally into
63+
* `driver.update(object, {$in: […]}, …)` **and** skip the #2982 row-scoping
64+
* AST seeding. So they are `reject` unless the caller also said `multi` —
65+
* and that holds wherever the non-scalar sits, `options.where.id` or the
66+
* payload's `data.id`.
67+
* 2. **`data.id` outranks `where.id`, but only when it IS an id.** The payload
68+
* is read first, so a scalar `data.id` still wins over `where` and over an
69+
* explicit `multi: true`; `update(o, { id: 'rec_1', … }, { multi: true })`
70+
* is one by-id write, unchanged. What a non-scalar `data.id` no longer does
71+
* is *outrank* anything: it is not an id, so the decision falls through to
72+
* `where.id`, then to `multi`, then to `reject` — exactly the ladder a
73+
* non-scalar `where.id` falls down. Until objectstack#5748 the payload half
74+
* took `data.id` verbatim whenever it was truthy, which made the same
75+
* operator object an id in `data` and a predicate in `where` — two rules
76+
* for one primary key inside one method, with the payload one binding
77+
* `{$in: […]}` into the primary-key position and swallowing a declared
78+
* `multi: true` with no diagnostic. Now there is one rule, defined once
79+
* below and reached by both halves.
7380
* 3. **Truthiness, not `!== undefined`.** The engine branches on
7481
* `if (hookContext.input.id)`, so a falsy scalar id — `where: { id: 0 }`,
75-
* `where: { id: '' }` — does **not** take the by-id route; it falls through
76-
* to `multi`/`reject` like any other non-identifying call.
82+
* `data: { id: 0 }`, `where: { id: '' }` — does **not** take the by-id
83+
* route; it falls through to `multi`/`reject` like any other
84+
* non-identifying call.
7785
*
7886
* ## What the predicate deliberately does NOT model
7987
*
@@ -99,7 +107,7 @@ export const ENGINE_UPDATE_REJECT_MESSAGE = 'Update requires an ID or options.mu
99107

100108
/** What `ObjectQLEngine.update` will do with a given `(data, options)` pair. */
101109
export type EngineUpdateDispatch =
102-
/** A truthy `data.id`, or a truthy scalar `where.id` — `driver.update`. */
110+
/** A truthy scalar `data.id`, or a truthy scalar `where.id` — `driver.update`. */
103111
| { readonly kind: 'by-id'; readonly id: unknown }
104112
/** No single id but `options.multi` — `driver.updateMany` with the composed AST. */
105113
| { readonly kind: 'multi' }
@@ -119,6 +127,29 @@ export interface EngineUpdateDispatchData {
119127
readonly [k: string]: unknown;
120128
}
121129

130+
/**
131+
* "Is this VALUE a primary key, or a predicate over many rows?" — the one
132+
* scalar test, so that the two places an update can carry an id
133+
* (`options.where.id` and `data.id`) cannot answer it differently.
134+
*
135+
* `null`, `undefined`, arrays and operator objects (`{ $in: [...] }`,
136+
* `{ $ne: … }`) all yield `undefined`: they select a SET, and binding one into
137+
* the primary-key position of `driver.update(object, id, …)` is the #4434 /
138+
* #4550 failure the whole family exists to prevent.
139+
*
140+
* Not exported: callers want a verdict about a CALL, which is
141+
* {@link resolveEngineUpdateDispatch}, or about the `where` half, which is
142+
* {@link scalarUpdateId}. Adding a third public spelling of the same question
143+
* is how a rule with one definition grows a second one.
144+
*/
145+
function asScalarId(value: unknown): string | number | bigint | undefined {
146+
const t = typeof value;
147+
if (value !== null && (t === 'string' || t === 'number' || t === 'bigint')) {
148+
return value as string | number | bigint;
149+
}
150+
return undefined;
151+
}
152+
122153
/**
123154
* Extract the SCALAR `where.id`, or `undefined` when the call's `where` does
124155
* not name one row by primary key.
@@ -127,8 +158,8 @@ export interface EngineUpdateDispatchData {
127158
* arrays and operator objects (`{ $in: [...] }`, `{ $ne: … }`) all yield
128159
* `undefined`, because they are predicates over many rows.
129160
*
130-
* Note this covers only the `where` half of the update decision; `data.id`
131-
* outranks it and is taken verbatim (see the module header, point 2). Use
161+
* Note this covers only the `where` half of the update decision; a scalar
162+
* `data.id` outranks it (see the module header, point 2). Use
132163
* {@link resolveEngineUpdateDispatch} for the whole answer.
133164
*/
134165
export function scalarUpdateId(
@@ -137,12 +168,7 @@ export function scalarUpdateId(
137168
const where = options?.where;
138169
if (!where || typeof where !== 'object') return undefined;
139170
if (!('id' in (where as Record<string, unknown>))) return undefined;
140-
const whereId = (where as Record<string, unknown>).id;
141-
const t = typeof whereId;
142-
if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) {
143-
return whereId as string | number | bigint;
144-
}
145-
return undefined;
171+
return asScalarId((where as Record<string, unknown>).id);
146172
}
147173

148174
/**
@@ -163,9 +189,13 @@ export function resolveEngineUpdateDispatch(
163189
data: EngineUpdateDispatchData,
164190
options?: EngineUpdateDispatchInput | null,
165191
): EngineUpdateDispatch {
166-
// `let id = data.id; if (!id && <scalar where.id>) id = whereId;` — the
167-
// producer's own two lines, in the producer's own order.
168-
let id: unknown = data.id;
192+
// The payload is still read FIRST and still outranks `where` — but it only
193+
// outranks with an actual id. `data.id` goes through the same scalar test as
194+
// `where.id` (objectstack#5748), so an operator object / array / `null`
195+
// parked in the payload is not an id and does not shadow the ladder below
196+
// it. `data.id` stays UNGUARDED so a missing payload is the producer's
197+
// `TypeError`, not a kinder verdict.
198+
let id: unknown = asScalarId(data.id);
169199
if (!id) {
170200
const fromWhere = scalarUpdateId(options);
171201
if (fromWhere !== undefined) id = fromWhere;
@@ -208,8 +238,8 @@ export function assertEngineUpdateDispatch(
208238
*
209239
* Every case names a call shape and the verdict the **real engine** gives it.
210240
* A double proved against these is proved against the producer, including the
211-
* shapes that look like an id and are not — and the one that does not look
212-
* like an id and is (`data.id`).
241+
* shapes that look like an id and are not — in `where` and, since
242+
* objectstack#5748, in the payload too.
213243
*/
214244
export interface EngineUpdateDispatchCase {
215245
/** What the shape is, in the words a failure message should use. */
@@ -220,21 +250,44 @@ export interface EngineUpdateDispatchCase {
220250
readonly options: EngineUpdateDispatchInput | undefined;
221251
/** The verdict the engine gives it. */
222252
readonly expect: EngineUpdateDispatch['kind'];
253+
/**
254+
* For a `by-id` case, the value that must land in the PRIMARY-KEY position
255+
* of `driver.update(object, id, …)`.
256+
*
257+
* `expect` alone cannot separate "picked the right id" from "picked an id":
258+
* a payload carrying `{ id: { $in: […] } }` beside a scalar `where.id`
259+
* dispatches `by-id` under both the old rule and the new one, and only the
260+
* bound value says which id source won — the operator object (#5748's bug)
261+
* or the scalar (#5748's fix). Optional: omit it when the case's id source
262+
* is unambiguous.
263+
*/
264+
readonly expectId?: unknown;
223265
}
224266

225267
export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] = [
226268
// ── by-id via `where`.
227269
{ what: 'scalar string where.id', data: { title: 'x' }, options: { where: { id: 'rec_1' } }, expect: 'by-id' },
228270
{ what: 'scalar number where.id', data: { title: 'x' }, options: { where: { id: 42 } }, expect: 'by-id' },
229271
{ what: 'scalar where.id alongside other predicates', data: { title: 'x' }, options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'by-id' },
230-
// ── by-id via the PAYLOAD, which outranks `where` and `multi` alike.
231-
{ what: 'id carried in the data payload, no where at all', data: { id: 'rec_1', title: 'x' }, options: undefined, expect: 'by-id' },
232-
{ what: 'data.id wins over an explicit multi:true', data: { id: 'rec_1', title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'by-id' },
272+
// ── by-id via the PAYLOAD. A SCALAR `data.id` still outranks `where` and
273+
// `multi` alike — that is the common, legal `update(o, { id, …fields })`
274+
// spelling and objectstack#5748 left it exactly as it was.
275+
{ what: 'id carried in the data payload, no where at all', data: { id: 'rec_1', title: 'x' }, options: undefined, expect: 'by-id', expectId: 'rec_1' },
276+
{ what: 'a SCALAR data.id still wins over an explicit multi:true', data: { id: 'rec_1', title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'by-id', expectId: 'rec_1' },
277+
{ what: 'a SCALAR data.id still wins over a scalar where.id', data: { id: 'rec_1', title: 'x' }, options: { where: { id: 'rec_2' } }, expect: 'by-id', expectId: 'rec_1' },
278+
// ── The payload's scalar test (objectstack#5748). A non-scalar `data.id`
279+
// names no row, so it stops shadowing everything under it: the decision
280+
// falls through to `where.id`, then `multi`, then `reject`. Before #5748
281+
// each of these dispatched `by-id` with the operator object itself bound
282+
// into `driver.update`'s primary-key position.
283+
{ what: 'operator object in data.id, scalar where.id — the WHERE id wins, the operator is not one', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: { where: { id: 'rec_1' } }, expect: 'by-id', expectId: 'rec_1' },
233284
// ── multi.
234285
{ what: 'multi with a predicate', data: { title: 'x' }, options: { where: { tenant: 't1' }, multi: true }, expect: 'multi' },
235286
{ what: 'multi with no predicate at all', data: { title: 'x' }, options: { multi: true }, expect: 'multi' },
236287
{ what: 'multi alongside an $in id set', data: { title: 'x' }, options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' },
237288
{ what: 'multi with a FALSY data.id (0 does not identify a row)', data: { id: 0, title: 'x' }, options: { multi: true }, expect: 'multi' },
289+
{ what: 'operator object in data.id WITH multi:true — the declared bulk intent is honoured (#5748)', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: { multi: true }, expect: 'multi' },
290+
{ what: 'array data.id with multi:true', data: { id: ['a', 'b'], title: 'x' }, options: { multi: true }, expect: 'multi' },
238291
// ── The rejects. Every one of these is a call a fake that mirrors the rule
239292
// by hand tends to accept, and a running server answers 500 to.
240293
{ what: 'predicate on a non-id column, no multi', data: { title: 'x' }, options: { where: { tenant: 't1' } }, expect: 'reject' },
@@ -245,4 +298,11 @@ export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] =
245298
{ what: 'empty where, no multi', data: { title: 'x' }, options: { where: {} }, expect: 'reject' },
246299
{ what: 'no options at all', data: { title: 'x' }, options: undefined, expect: 'reject' },
247300
{ what: 'multi explicitly false with a predicate', data: { title: 'x' }, options: { where: { tenant: 't1' }, multi: false }, expect: 'reject' },
301+
// ── The typo shape #5748's B option was worried about, pinned LOUD: an
302+
// operator object in the payload with NO declared bulk intent is a
303+
// rejection, never a silent promotion to a bulk write.
304+
{ what: 'operator object in data.id, NO multi — rejected, NOT silently promoted to a bulk write (#5748)', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: undefined, expect: 'reject' },
305+
{ what: 'operator object in data.id, multi explicitly false', data: { id: { $in: ['a', 'b'] }, title: 'x' }, options: { multi: false }, expect: 'reject' },
306+
{ what: 'array data.id, no multi', data: { id: ['a', 'b'], title: 'x' }, options: undefined, expect: 'reject' },
307+
{ what: 'null data.id, no multi', data: { id: null, title: 'x' }, options: undefined, expect: 'reject' },
248308
];

0 commit comments

Comments
 (0)