From b38fa3e17c2afa3b110b09a73b7fa096c9a114dc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:00:12 +0000 Subject: [PATCH] fix(metadata-core): truthy-test the scalar where.id in the delete dispatch (#5747) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../engine-delete-dispatch-falsy-scalar-id.md | 20 +++++ .../src/engine-delete-dispatch.ts | 76 ++++++++++++++++--- .../src/engine-delete-dispatch.test.ts | 39 ++++++++++ 3 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 .changeset/engine-delete-dispatch-falsy-scalar-id.md diff --git a/.changeset/engine-delete-dispatch-falsy-scalar-id.md b/.changeset/engine-delete-dispatch-falsy-scalar-id.md new file mode 100644 index 0000000000..e0a575a20c --- /dev/null +++ b/.changeset/engine-delete-dispatch-falsy-scalar-id.md @@ -0,0 +1,20 @@ +--- +'@objectstack/metadata-core': patch +--- + +`resolveEngineDeleteDispatch` 末尾改真值测试:假值标量 `where.id` 不再答 `by-id` + +`engine-delete-dispatch.ts` 的自我描述是「what does `ObjectQLEngine.delete` do with this call」的**唯一**答案,其测试文件头把赌注写得很明白:一份漂移的共享判定比没有判定更糟——每个钉在它上面的假引擎都会自信地、一致地错,而门禁照样报绿。这条性质此前在**假值标量 id** 上不成立,实测(origin/main,记录型 driver 驱动真实引擎): + +| 调用 | 真实 `ObjectQL.delete` | 判定(修改前) | +|:---|:---|:---| +| `{ where: { id: 0 } }` | `reject` | `by-id` | +| `{ where: { id: '' } }` | `reject` | `by-id` | +| `{ where: { id: 0 }, multi: true }` | `multi` | `by-id` | +| `{ where: { id: '' }, multi: true }` | `multi` | `by-id` | + +原因是两侧问了不同的问题:判定读 `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`)是可达形状,不是猎奇。 + +本次改的是**判定,不是引擎**。`resolveEngineDeleteDispatch` 是对 `ObjectQL.delete` 的描述,错的是描述:`delete(o, { where: { id: 0 } })` 改动前抛错,改动后照样抛错,**生产者行为零变化**,`engine.ts` 一字未动。反向做法(让 `{ id: 0 }` 变成真的按 id 删)是改生产者行为,已作为 #5747 的 B 方案明确不取。 + +同时给 `ENGINE_DELETE_DISPATCH_CASES` 补上 `{ id: 0 }` / `{ id: '' }` 的有/无 `multi` 四例——此前这套逐例对照**结构上够不到**这个输入(#4868 家族:一次逐例跑不可能反驳一个没人列出来的输入),这才是判定能悄悄漂移一年的原因。`scalarDeleteId` 保持值忠实(`{ where: { id: 0 } }` 仍返回 `0`),真值测试只加在判定这一层,与 update 侧孪生模块 `scalarUpdateId` 的分法一致。 diff --git a/packages/metadata-core/src/engine-delete-dispatch.ts b/packages/metadata-core/src/engine-delete-dispatch.ts index d26fd4410c..606726d298 100644 --- a/packages/metadata-core/src/engine-delete-dispatch.ts +++ b/packages/metadata-core/src/engine-delete-dispatch.ts @@ -65,21 +65,45 @@ * call identify a single row by primary key?* * * - `options.where.id` is a **scalar** (`string` / `number` / `bigint`, not - * `null`) → `by-id`: routes to `driver.delete`, runs cascade-delete and the - * by-id RLS pre-image check. + * `null`) **and truthy** → `by-id`: routes to `driver.delete`, runs + * cascade-delete and the by-id RLS pre-image check. * - otherwise, `options.multi` is truthy → `multi`: routes to * `driver.deleteMany` with the middleware-composed AST. * - otherwise → **`reject`**. The call names neither one row nor a bulk * intent, and the engine throws rather than guessing. * - * The scalar test is load-bearing and is the half a hand-written double most - * often drops: `where: { id: { $in: [...] } }` is a *multi-row predicate*, not - * an id. Treating it as an id would bind the operator object literally into - * `driver.delete(object, {$in: […]})` **and** skip both the row-scoping AST - * seeding (#2982) and the by-id pre-image check. So it is `reject` unless the - * caller also said `multi`. + * Two halves of that first line are load-bearing, and a hand-written double + * drops one or the other — which is the whole argument for importing this + * instead of copying it: + * + * 1. **The scalar test.** `where: { id: { $in: [...] } }` is a *multi-row + * predicate*, not an id. Treating it as an id would bind the operator + * object literally into `driver.delete(object, {$in: […]})` **and** skip + * both the row-scoping AST seeding (#2982) and the by-id pre-image check. + * So it is `reject` unless the caller also said `multi`. + * 2. **Truthiness, not `!== undefined`.** The engine branches on + * `if (hookContext.input.id)`, so a falsy scalar id — `where: { id: 0 }`, + * `where: { id: '' }` — does **not** take the by-id route; it falls through + * to `multi`/`reject` like any other non-identifying call. Byte-for-byte + * the rule the twin states as its own point 3, and until objectstack#5747 + * this module read `!== undefined` and answered `by-id` for both — the one + * input on which a double pinned to `assertEngineDeleteDispatch` was still + * *looser than the producer* (it accepted `delete(o, { where: { id: '' } })` + * while a running server answers `ENGINE_DELETE_REJECT_MESSAGE`), which is + * the #4434 shape this module exists to remove. `where: { id: '' }` is a + * reachable spelling, not a curiosity: an empty path segment or an unfilled + * form field passed straight through to `where.id` produces it. + * + * Note this **changed the predicate, never the engine**. `resolveEngine…` + * is a description of `ObjectQL.delete`, and it was the description that + * was wrong; `delete(o, { where: { id: 0 } })` threw before this change and + * throws after it. Realigning the engine to the old description instead — + * making `{ id: 0 }` a real by-id delete — would have been a change to the + * producer's behaviour, and was rejected as such (objectstack#5747 option + * B, deliberately not taken). * * @see ObjectQL.delete in `packages/objectql/src/engine.ts` — the only production caller. + * @see engine-update-dispatch.ts — the twin; its point 3 is this module's point 2. * @see packages/objectql/src/engine-delete-dispatch.ts — the re-export shim that keeps * objectql's original import path (and its public API) working. * @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 /** What `ObjectQLEngine.delete` will do with a given options bag. */ export type EngineDeleteDispatch = - /** A scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */ + /** A TRUTHY scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */ | { readonly kind: 'by-id'; readonly id: string | number | bigint } /** No single id but `options.multi` — `driver.deleteMany` with the composed AST. */ | { readonly kind: 'multi' } @@ -108,12 +132,21 @@ export interface EngineDeleteDispatchInput { } /** - * Extract the SCALAR `where.id`, or `undefined` when the call does not name one - * row by primary key. + * Extract the SCALAR `where.id`, or `undefined` when `where` carries no scalar + * there at all. * * `null`, `undefined`, arrays, and operator objects (`{ $in: [...] }`, * `{ $ne: … }`) all yield `undefined` — they are predicates over many rows, not * a primary key. + * + * This answers only "is that VALUE a scalar?", which is **not** the whole + * by-id question: the engine additionally requires the id to be TRUTHY, so + * `scalarDeleteId({ where: { id: 0 } })` is `0` while the call itself + * dispatches `multi`/`reject` (objectstack#5747). Kept value-faithful on + * purpose — narrowing it to "truthy scalar" would make the extractor and its + * name disagree, and a caller asking what the `where` holds would have to + * reach for a second spelling. Use {@link resolveEngineDeleteDispatch} for the + * verdict about a CALL; its twin `scalarUpdateId` splits the same way. */ export function scalarDeleteId( options?: EngineDeleteDispatchInput | null, @@ -140,7 +173,11 @@ export function resolveEngineDeleteDispatch( options?: EngineDeleteDispatchInput | null, ): EngineDeleteDispatch { const id = scalarDeleteId(options); - if (id !== undefined) return { kind: 'by-id', id }; + // The engine branches on `if (hookContext.input.id)` — truthiness, not + // `!== undefined`, so a falsy scalar id (`0`, `''`) is not an identifying + // call and falls down the same ladder as a non-scalar one. See header + // point 2, and the twin's point 3 (objectstack#5747 / objectstack#5748). + if (id) return { kind: 'by-id', id }; if (options?.multi) return { kind: 'multi' }; return { kind: 'reject', message: ENGINE_DELETE_REJECT_MESSAGE }; } @@ -192,12 +229,27 @@ export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] = { what: 'multi with a predicate', options: { where: { rule_id: 'r1' }, multi: true }, expect: 'multi' }, { what: 'multi with no predicate at all', options: { multi: true }, expect: 'multi' }, { what: 'multi alongside an $in id set', options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' }, + // ── The FALSY scalars (objectstack#5747). `0` and `''` are scalars, so + // `scalarDeleteId` returns them — but the engine's `if (input.id)` is a + // truthiness test, so neither identifies a row. With a declared bulk + // intent they are honoured as `multi` (the caller's `where` still rides + // onto the AST, so the predicate is `id = 0` / `id = ''`, not "every + // row"); without one they are `reject`, below. Same pair the twin pins + // on the update side. + { what: 'falsy scalar where.id (0) with multi:true', options: { where: { id: 0 }, multi: true }, expect: 'multi' }, + { what: "falsy scalar where.id ('') with multi:true", options: { where: { id: '' }, multi: true }, expect: 'multi' }, // ── The rejects. Everything below is what #4434 shipped against a fake that // accepted it, and what a running server answers 500 to. { what: 'predicate on a non-id column, no multi', options: { where: { rule_id: 'r1' } }, expect: 'reject' }, { what: '$in over ids, no multi (an operator object is NOT an id)', options: { where: { id: { $in: ['a', 'b'] } } }, expect: 'reject' }, { what: 'array id, no multi', options: { where: { id: ['a', 'b'] } }, expect: 'reject' }, { what: 'null id, no multi', options: { where: { id: null } }, expect: 'reject' }, + // The two shapes objectstack#5747 was filed for: a fake pinned to + // `assertEngineDeleteDispatch` ACCEPTED both until this case-set could + // reach them (#4868 family — a per-case parity run cannot contradict an + // input nobody listed). + { what: 'falsy scalar where.id (0), no multi', options: { where: { id: 0 } }, expect: 'reject' }, + { what: "falsy scalar where.id (''), no multi — the empty path segment / unfilled form field", options: { where: { id: '' } }, expect: 'reject' }, { what: 'empty where, no multi', options: { where: {} }, expect: 'reject' }, { what: 'no options at all', options: undefined, expect: 'reject' }, { what: 'multi explicitly false with a predicate', options: { where: { rule_id: 'r1' }, multi: false }, expect: 'reject' }, diff --git a/packages/objectql/src/engine-delete-dispatch.test.ts b/packages/objectql/src/engine-delete-dispatch.test.ts index 3f3ecd3584..3887646b6b 100644 --- a/packages/objectql/src/engine-delete-dispatch.test.ts +++ b/packages/objectql/src/engine-delete-dispatch.test.ts @@ -103,4 +103,43 @@ describe('engine delete dispatch — the shared predicate IS the engine (#4550)' expect(scalarDeleteId({ where: {} })).toBeUndefined(); expect(scalarDeleteId(undefined)).toBeUndefined(); }); + + // ── objectstack#5747. The twin's `branches on TRUTHINESS` test, on this side. + it('branches on TRUTHINESS, so a falsy scalar where.id does not identify a row', () => { + expect(resolveEngineDeleteDispatch({ where: { id: 0 } }).kind).toBe('reject'); + expect(resolveEngineDeleteDispatch({ where: { id: '' } }).kind).toBe('reject'); + expect(resolveEngineDeleteDispatch({ where: { id: 0 }, multi: true }).kind).toBe('multi'); + expect(resolveEngineDeleteDispatch({ where: { id: '' }, multi: true }).kind).toBe('multi'); + // …while `scalarDeleteId` still reports the raw scalar it found. The two + // answer different questions and only `resolveEngineDeleteDispatch` + // answers the engine's. Same split as `scalarUpdateId` on the twin. + expect(scalarDeleteId({ where: { id: 0 } })).toBe(0); + expect(scalarDeleteId({ where: { id: '' } })).toBe(''); + }); + + it('a fake pinned to assertEngineDeleteDispatch now refuses the falsy scalar ids too (#5747)', () => { + // This is the whole point of the issue: before #5747 both of these + // RETURNED `{ kind: 'by-id', id: 0 }` / `{ kind: 'by-id', id: '' }`, so + // every double pinned to this line accepted a call the real server + // answers `ENGINE_DELETE_REJECT_MESSAGE` to — looser than the producer on + // exactly one input, which is the #4434 shape. + expect(() => assertEngineDeleteDispatch({ where: { id: 0 } })).toThrow(ENGINE_DELETE_REJECT_MESSAGE); + expect(() => assertEngineDeleteDispatch({ where: { id: '' } })).toThrow(ENGINE_DELETE_REJECT_MESSAGE); + expect(assertEngineDeleteDispatch({ where: { id: 0 }, multi: true })).toEqual({ kind: 'multi' }); + }); + + it('a falsy-id `multi` delete is still SCOPED by the caller where, not a whole-table purge', () => { + // `multi` is the honest verdict for `{ where: { id: 0 }, multi: true }`, + // and the reason it is safe is that the caller's own predicate still + // rides onto the #2982 AST — the bulk delete filters on `id = 0`, it does + // not match every row. Asserted against the REAL engine because that is + // the only place the claim is true or false. + return (async () => { + const { engine, calls } = await makeEngine(); + await engine.delete('task', { where: { id: 0 }, multi: true } as any); + expect(calls).toHaveLength(1); + expect(calls[0].fn).toBe('deleteMany'); + expect(calls[0].arg).toMatchObject({ object: 'task', where: { id: 0 } }); + })(); + }); });