Skip to content

Commit 50dfe9c

Browse files
baozhoutaoclaude
andauthored
fix(metadata-core): truthy-test the scalar where.id in the delete dispatch (#5747) (#5999)
`resolveEngineDeleteDispatch` answered `by-id` for a falsy scalar `where.id` (`0`, `''`) while `ObjectQL.delete` — which branches on `if (hookContext.input.id)` — routes the same call to multi/reject. Measured against the real engine with a recording driver: {where:{id:0}} engine=reject predicate=by-id {where:{id:''}} engine=reject predicate=by-id {where:{id:0},multi:true} engine=multi predicate=by-id {where:{id:''},multi:true} engine=multi predicate=by-id So every fake engine pinned to `assertEngineDeleteDispatch` ACCEPTED `delete(o, { where: { id: '' } })` while a running server throws — the #4434 shape this module exists to remove, on the one input the shared case-set could not reach (#4868 family). This changes the PREDICATE, never the engine: the predicate is a description of `ObjectQL.delete` and the description was wrong. `engine.ts` is untouched and `delete(o, {where:{id:0}})` throws before and after. Option B (make `{id:0}` a real by-id delete) would have changed producer behaviour and is deliberately not taken. `ENGINE_DELETE_DISPATCH_CASES` gains the four falsy shapes, so the real-engine parity run now reaches them. `scalarDeleteId` stays value-faithful — it answers "is that VALUE a scalar", the truthiness test belongs to the dispatch — matching how `scalarUpdateId` and the update twin already split it (#5748). Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We Co-authored-by: Claude <noreply@anthropic.com>
1 parent 44106d9 commit 50dfe9c

3 files changed

Lines changed: 123 additions & 12 deletions

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/metadata-core': patch
3+
---
4+
5+
`resolveEngineDeleteDispatch` 末尾改真值测试:假值标量 `where.id` 不再答 `by-id`
6+
7+
`engine-delete-dispatch.ts` 的自我描述是「what does `ObjectQLEngine.delete` do with this call」的**唯一**答案,其测试文件头把赌注写得很明白:一份漂移的共享判定比没有判定更糟——每个钉在它上面的假引擎都会自信地、一致地错,而门禁照样报绿。这条性质此前在**假值标量 id** 上不成立,实测(origin/main,记录型 driver 驱动真实引擎):
8+
9+
| 调用 | 真实 `ObjectQL.delete` | 判定(修改前) |
10+
|:---|:---|:---|
11+
| `{ where: { id: 0 } }` | `reject` | `by-id` |
12+
| `{ where: { id: '' } }` | `reject` | `by-id` |
13+
| `{ where: { id: 0 }, multi: true }` | `multi` | `by-id` |
14+
| `{ where: { id: '' }, multi: true }` | `multi` | `by-id` |
15+
16+
原因是两侧问了不同的问题:判定读 `scalarDeleteId(...) !== undefined`,而 `engine.ts` 把判定结果落进 `id` 之后按 `if (hookContext.input.id)` 分支——**真值**测试,`0` / `''` 落到 multi/reject 阶梯。于是按 `assertEngineDeleteDispatch(options)` 钉死的替身会**接受** `delete(o, { where: { id: '' } })`,而真服务器抛 `Delete requires an ID or options.multi=true`:pinned 替身在这一个输入上仍比生产者宽松,正是本模块存在的理由(#4434 形状)。`id: ''`(路径段为空 / 表单字段未填直传 `where.id`)是可达形状,不是猎奇。
17+
18+
本次改的是**判定,不是引擎**。`resolveEngineDeleteDispatch` 是对 `ObjectQL.delete` 的描述,错的是描述:`delete(o, { where: { id: 0 } })` 改动前抛错,改动后照样抛错,**生产者行为零变化**,`engine.ts` 一字未动。反向做法(让 `{ id: 0 }` 变成真的按 id 删)是改生产者行为,已作为 #5747 的 B 方案明确不取。
19+
20+
同时给 `ENGINE_DELETE_DISPATCH_CASES` 补上 `{ id: 0 }` / `{ id: '' }` 的有/无 `multi` 四例——此前这套逐例对照**结构上够不到**这个输入(#4868 家族:一次逐例跑不可能反驳一个没人列出来的输入),这才是判定能悄悄漂移一年的原因。`scalarDeleteId` 保持值忠实(`{ where: { id: 0 } }` 仍返回 `0`),真值测试只加在判定这一层,与 update 侧孪生模块 `scalarUpdateId` 的分法一致。

‎packages/metadata-core/src/engine-delete-dispatch.ts‎

Lines changed: 64 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,45 @@
6565
* call identify a single row by primary key?*
6666
*
6767
* - `options.where.id` is a **scalar** (`string` / `number` / `bigint`, not
68-
* `null`) → `by-id`: routes to `driver.delete`, runs cascade-delete and the
69-
* by-id RLS pre-image check.
68+
* `null`) **and truthy** → `by-id`: routes to `driver.delete`, runs
69+
* cascade-delete and the by-id RLS pre-image check.
7070
* - otherwise, `options.multi` is truthy → `multi`: routes to
7171
* `driver.deleteMany` with the middleware-composed AST.
7272
* - otherwise → **`reject`**. The call names neither one row nor a bulk
7373
* intent, and the engine throws rather than guessing.
7474
*
75-
* The scalar test is load-bearing and is the half a hand-written double most
76-
* often drops: `where: { id: { $in: [...] } }` is a *multi-row predicate*, not
77-
* an id. Treating it as an id would bind the operator object literally into
78-
* `driver.delete(object, {$in: […]})` **and** skip both the row-scoping AST
79-
* seeding (#2982) and the by-id pre-image check. So it is `reject` unless the
80-
* caller also said `multi`.
75+
* Two halves of that first line are load-bearing, and a hand-written double
76+
* drops one or the other — which is the whole argument for importing this
77+
* instead of copying it:
78+
*
79+
* 1. **The scalar test.** `where: { id: { $in: [...] } }` is a *multi-row
80+
* predicate*, not an id. Treating it as an id would bind the operator
81+
* object literally into `driver.delete(object, {$in: […]})` **and** skip
82+
* both the row-scoping AST seeding (#2982) and the by-id pre-image check.
83+
* So it is `reject` unless the caller also said `multi`.
84+
* 2. **Truthiness, not `!== undefined`.** The engine branches on
85+
* `if (hookContext.input.id)`, so a falsy scalar id — `where: { id: 0 }`,
86+
* `where: { id: '' }` — does **not** take the by-id route; it falls through
87+
* to `multi`/`reject` like any other non-identifying call. Byte-for-byte
88+
* the rule the twin states as its own point 3, and until objectstack#5747
89+
* this module read `!== undefined` and answered `by-id` for both — the one
90+
* input on which a double pinned to `assertEngineDeleteDispatch` was still
91+
* *looser than the producer* (it accepted `delete(o, { where: { id: '' } })`
92+
* while a running server answers `ENGINE_DELETE_REJECT_MESSAGE`), which is
93+
* the #4434 shape this module exists to remove. `where: { id: '' }` is a
94+
* reachable spelling, not a curiosity: an empty path segment or an unfilled
95+
* form field passed straight through to `where.id` produces it.
96+
*
97+
* Note this **changed the predicate, never the engine**. `resolveEngine…`
98+
* is a description of `ObjectQL.delete`, and it was the description that
99+
* was wrong; `delete(o, { where: { id: 0 } })` threw before this change and
100+
* throws after it. Realigning the engine to the old description instead —
101+
* making `{ id: 0 }` a real by-id delete — would have been a change to the
102+
* producer's behaviour, and was rejected as such (objectstack#5747 option
103+
* B, deliberately not taken).
81104
*
82105
* @see ObjectQL.delete in `packages/objectql/src/engine.ts` — the only production caller.
106+
* @see engine-update-dispatch.ts — the twin; its point 3 is this module's point 2.
83107
* @see packages/objectql/src/engine-delete-dispatch.ts — the re-export shim that keeps
84108
* objectql's original import path (and its public API) working.
85109
* @see packages/objectql/src/engine-delete-dispatch.test.ts — the test that drives the
@@ -93,7 +117,7 @@ export const ENGINE_DELETE_REJECT_MESSAGE = 'Delete requires an ID or options.mu
93117

94118
/** What `ObjectQLEngine.delete` will do with a given options bag. */
95119
export type EngineDeleteDispatch =
96-
/** A scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */
120+
/** A TRUTHY scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */
97121
| { readonly kind: 'by-id'; readonly id: string | number | bigint }
98122
/** No single id but `options.multi` — `driver.deleteMany` with the composed AST. */
99123
| { readonly kind: 'multi' }
@@ -108,12 +132,21 @@ export interface EngineDeleteDispatchInput {
108132
}
109133

110134
/**
111-
* Extract the SCALAR `where.id`, or `undefined` when the call does not name one
112-
* row by primary key.
135+
* Extract the SCALAR `where.id`, or `undefined` when `where` carries no scalar
136+
* there at all.
113137
*
114138
* `null`, `undefined`, arrays, and operator objects (`{ $in: [...] }`,
115139
* `{ $ne: … }`) all yield `undefined` — they are predicates over many rows, not
116140
* a primary key.
141+
*
142+
* This answers only "is that VALUE a scalar?", which is **not** the whole
143+
* by-id question: the engine additionally requires the id to be TRUTHY, so
144+
* `scalarDeleteId({ where: { id: 0 } })` is `0` while the call itself
145+
* dispatches `multi`/`reject` (objectstack#5747). Kept value-faithful on
146+
* purpose — narrowing it to "truthy scalar" would make the extractor and its
147+
* name disagree, and a caller asking what the `where` holds would have to
148+
* reach for a second spelling. Use {@link resolveEngineDeleteDispatch} for the
149+
* verdict about a CALL; its twin `scalarUpdateId` splits the same way.
117150
*/
118151
export function scalarDeleteId(
119152
options?: EngineDeleteDispatchInput | null,
@@ -140,7 +173,11 @@ export function resolveEngineDeleteDispatch(
140173
options?: EngineDeleteDispatchInput | null,
141174
): EngineDeleteDispatch {
142175
const id = scalarDeleteId(options);
143-
if (id !== undefined) return { kind: 'by-id', id };
176+
// The engine branches on `if (hookContext.input.id)` — truthiness, not
177+
// `!== undefined`, so a falsy scalar id (`0`, `''`) is not an identifying
178+
// call and falls down the same ladder as a non-scalar one. See header
179+
// point 2, and the twin's point 3 (objectstack#5747 / objectstack#5748).
180+
if (id) return { kind: 'by-id', id };
144181
if (options?.multi) return { kind: 'multi' };
145182
return { kind: 'reject', message: ENGINE_DELETE_REJECT_MESSAGE };
146183
}
@@ -192,12 +229,27 @@ export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] =
192229
{ what: 'multi with a predicate', options: { where: { rule_id: 'r1' }, multi: true }, expect: 'multi' },
193230
{ what: 'multi with no predicate at all', options: { multi: true }, expect: 'multi' },
194231
{ what: 'multi alongside an $in id set', options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' },
232+
// ── The FALSY scalars (objectstack#5747). `0` and `''` are scalars, so
233+
// `scalarDeleteId` returns them — but the engine's `if (input.id)` is a
234+
// truthiness test, so neither identifies a row. With a declared bulk
235+
// intent they are honoured as `multi` (the caller's `where` still rides
236+
// onto the AST, so the predicate is `id = 0` / `id = ''`, not "every
237+
// row"); without one they are `reject`, below. Same pair the twin pins
238+
// on the update side.
239+
{ what: 'falsy scalar where.id (0) with multi:true', options: { where: { id: 0 }, multi: true }, expect: 'multi' },
240+
{ what: "falsy scalar where.id ('') with multi:true", options: { where: { id: '' }, multi: true }, expect: 'multi' },
195241
// ── The rejects. Everything below is what #4434 shipped against a fake that
196242
// accepted it, and what a running server answers 500 to.
197243
{ what: 'predicate on a non-id column, no multi', options: { where: { rule_id: 'r1' } }, expect: 'reject' },
198244
{ what: '$in over ids, no multi (an operator object is NOT an id)', options: { where: { id: { $in: ['a', 'b'] } } }, expect: 'reject' },
199245
{ what: 'array id, no multi', options: { where: { id: ['a', 'b'] } }, expect: 'reject' },
200246
{ what: 'null id, no multi', options: { where: { id: null } }, expect: 'reject' },
247+
// The two shapes objectstack#5747 was filed for: a fake pinned to
248+
// `assertEngineDeleteDispatch` ACCEPTED both until this case-set could
249+
// reach them (#4868 family — a per-case parity run cannot contradict an
250+
// input nobody listed).
251+
{ what: 'falsy scalar where.id (0), no multi', options: { where: { id: 0 } }, expect: 'reject' },
252+
{ what: "falsy scalar where.id (''), no multi — the empty path segment / unfilled form field", options: { where: { id: '' } }, expect: 'reject' },
201253
{ what: 'empty where, no multi', options: { where: {} }, expect: 'reject' },
202254
{ what: 'no options at all', options: undefined, expect: 'reject' },
203255
{ what: 'multi explicitly false with a predicate', options: { where: { rule_id: 'r1' }, multi: false }, expect: 'reject' },

‎packages/objectql/src/engine-delete-dispatch.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,43 @@ describe('engine delete dispatch — the shared predicate IS the engine (#4550)'
103103
expect(scalarDeleteId({ where: {} })).toBeUndefined();
104104
expect(scalarDeleteId(undefined)).toBeUndefined();
105105
});
106+
107+
// ── objectstack#5747. The twin's `branches on TRUTHINESS` test, on this side.
108+
it('branches on TRUTHINESS, so a falsy scalar where.id does not identify a row', () => {
109+
expect(resolveEngineDeleteDispatch({ where: { id: 0 } }).kind).toBe('reject');
110+
expect(resolveEngineDeleteDispatch({ where: { id: '' } }).kind).toBe('reject');
111+
expect(resolveEngineDeleteDispatch({ where: { id: 0 }, multi: true }).kind).toBe('multi');
112+
expect(resolveEngineDeleteDispatch({ where: { id: '' }, multi: true }).kind).toBe('multi');
113+
// …while `scalarDeleteId` still reports the raw scalar it found. The two
114+
// answer different questions and only `resolveEngineDeleteDispatch`
115+
// answers the engine's. Same split as `scalarUpdateId` on the twin.
116+
expect(scalarDeleteId({ where: { id: 0 } })).toBe(0);
117+
expect(scalarDeleteId({ where: { id: '' } })).toBe('');
118+
});
119+
120+
it('a fake pinned to assertEngineDeleteDispatch now refuses the falsy scalar ids too (#5747)', () => {
121+
// This is the whole point of the issue: before #5747 both of these
122+
// RETURNED `{ kind: 'by-id', id: 0 }` / `{ kind: 'by-id', id: '' }`, so
123+
// every double pinned to this line accepted a call the real server
124+
// answers `ENGINE_DELETE_REJECT_MESSAGE` to — looser than the producer on
125+
// exactly one input, which is the #4434 shape.
126+
expect(() => assertEngineDeleteDispatch({ where: { id: 0 } })).toThrow(ENGINE_DELETE_REJECT_MESSAGE);
127+
expect(() => assertEngineDeleteDispatch({ where: { id: '' } })).toThrow(ENGINE_DELETE_REJECT_MESSAGE);
128+
expect(assertEngineDeleteDispatch({ where: { id: 0 }, multi: true })).toEqual({ kind: 'multi' });
129+
});
130+
131+
it('a falsy-id `multi` delete is still SCOPED by the caller where, not a whole-table purge', () => {
132+
// `multi` is the honest verdict for `{ where: { id: 0 }, multi: true }`,
133+
// and the reason it is safe is that the caller's own predicate still
134+
// rides onto the #2982 AST — the bulk delete filters on `id = 0`, it does
135+
// not match every row. Asserted against the REAL engine because that is
136+
// the only place the claim is true or false.
137+
return (async () => {
138+
const { engine, calls } = await makeEngine();
139+
await engine.delete('task', { where: { id: 0 }, multi: true } as any);
140+
expect(calls).toHaveLength(1);
141+
expect(calls[0].fn).toBe('deleteMany');
142+
expect(calls[0].arg).toMatchObject({ object: 'task', where: { id: 0 } });
143+
})();
144+
});
106145
});

0 commit comments

Comments
 (0)