fix(metadata-core): delete 判定末尾改真值测试,假值标量 where.id 不再答 by-id (#5747)

.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` 的分法一致。

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' },

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 } });
+    })();
+  });
 });