fix(rest)!: POST /batch answers an update or delete op whose id names no row with the missing-id 400, not a 500

.changeset/24781-rest-batch-blank-id-400.md

@@ -0,0 +1,23 @@
+---
+'@objectstack/rest': minor
+---
+
+fix(rest)!: `POST /batch` answers an update or delete op whose id names no row with the missing-id `400`, not a `500` (#24781)
+
+Clause-②: no (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) A refusal at the cross-object batch door of request shapes that never wrote anything: every one of them already failed, as a 500 (or, for a delete addressed only by data.id, a 400 INVALID_FILTER), and now fails up front with the 400 a missing id already got. No metadata key, spelling, export, type or stored shape moves, so there is nothing for `objectstack migrate meta` to rewrite and no tombstone; the remedy is sending the record's id, stated below. -->
+
+**BREAKING** accept-set narrowing on one REST door, shipped as `minor` under the launch-window convention for accept-set narrowings. No request that succeeded before is refused now.
+
+The cross-object batch checked up front that an `update` / `delete` op names a row, but tested only for an absent or `null` id. An id the engine does not take as one reached the engine, and its `Update requires an ID…` / `Delete requires an ID…` throw answered as a server fault. A client's retry policy keyed on `5xx` then retried a request that can never succeed. The check now judges the id the op's write arm actually sends, with the engine's own by-id rule (a truthy string or number).
+
+- FROM: `update` with `"id": ""`, or with no `id` and a `data.id` of `""`, `0`, `true`, `false`, an object or an array, answered `500 DATABASE_ERROR`. An `"id": ""` beside a real `data.id` did too, because `id` is read first.
+- FROM: `delete` with `"id": ""` answered `500 INTERNAL_ERROR`.
+- FROM: `delete` with no `id` and the record's id in `data.id` answered `400 INVALID_FILTER`, with a message about a `where.id` the caller never wrote. The delete arm never read `data.id`; it is the create/update payload.
+- TO: each of these answers `400 VALIDATION_FAILED` with `Operation 'update' on 'OBJECT' requires an id` (or `'delete'`), the answer the same op with no id already got. The batch is refused before the transaction opens, and nothing is written.
+- Fix: send the record's id in the op's `id` (an update may carry it in `data.id` instead).
+
+This step runs before the per-object API-exposure check, as it did for a missing id. So for such an op, this `400` now also replaces whatever a later step used to answer first, such as the `404` / `405` for an API-disabled object or method.
+
+**Unchanged.** A missing or `null` id answers the same `400`. A real id updates and deletes as before, by `id` or (update only) by `data.id`. An update whose `data.id` is a `{ $ref }` to an earlier create still resolves. A whitespace id is an id the engine looks up: it answers `404 RECORD_NOT_FOUND`, like any unknown id. The per-object doors (`/data/:object/batch`, `updateMany`, `deleteMany`) already refused a blank id per row and are not changed.

packages/rest/src/batch-routes.ts

@@ -53,6 +53,9 @@ import type { DroppedFieldsEvent } from '@objectstack/spec/data';
 // The write-response rules (credential-class mask, `internal` omit) for
 // the cross-object batch's direct-engine update arm.
 import { omitInternalFieldsFromWriteResponse } from '@objectstack/core';
+// The engine's own by-id predicates, which `ObjectQL.update` / `.delete`
+// dispatch on — the cross-object batch's up-front id check asks them.
+import { resolveEngineDeleteDispatch, resolveEngineUpdateDispatch } from '@objectstack/metadata-core';
 import { handleRouteError } from './error-response.js';
 import type { ServerScopedDataRequest } from './data-action-routes.js';
 import {
@@ -81,6 +84,64 @@ export interface BatchRouteHost {
     loadObjectItems: RestServer['loadObjectItems'];
 }
 
+/** One operation of the cross-object batch, as the spec contract parses it. */
+type CrossObjectBatchOp = {
+    object: string;
+    action: 'create' | 'update' | 'delete';
+    id?: string;
+    data?: Record<string, any>;
+};
+
+/**
+ * A field value of the form `{ $ref: <earlier op index> }`: an intra-batch
+ * reference the cross-object batch resolves, inside the transaction, to that
+ * op's created id. One spelling for the resolver and for the up-front id check,
+ * so the check never judges a value the resolver would have replaced.
+ */
+function isBatchRef(value: unknown): value is { $ref: unknown } {
+    return Boolean(value) && typeof value === 'object' && '$ref' in (value as object);
+}
+
+/**
+ * The row an `update` / `delete` op of the cross-object batch addresses. The
+ * up-front refusal and both write arms read it HERE, so the refusal cannot pass
+ * an op that its arm then hands to the engine without an id.
+ *
+ * - `update`: `id`, else the payload's `data.id`. An `id` key that is present
+ *   wins even when it names no row: it is refused, and the payload's id does
+ *   not silently replace it.
+ * - `delete`: `id` alone. The delete arm addresses `where: { id }` and never
+ *   read `data.id`, which the spec declares as the create/update payload. The
+ *   up-front check used to accept `data.id` for a delete anyway, and the op
+ *   then reached the engine with no address at all.
+ */
+function batchOpRowAddress(op: CrossObjectBatchOp, data: Record<string, any> | undefined): unknown {
+    return op.action === 'delete' ? op.id : (op.id ?? data?.id);
+}
+
+/**
+ * Does this address name no row the engine could write? Such an op is refused
+ * up front with the answer a missing id gets. Before, it reached `ql.update` /
+ * `ql.delete`, whose `… requires an ID or options.multi=true` throw answered as
+ * a 500.
+ *
+ * Judged by the engine's OWN by-id predicates (`resolveEngineUpdateDispatch` /
+ * `resolveEngineDeleteDispatch`, the functions `ObjectQL.update` and
+ * `ObjectQL.delete` dispatch on), never by a mirrored `if`. A by-id address is
+ * a TRUTHY scalar. So `undefined`, `null`, `''`, `0`, a boolean, an object and
+ * an array name no row. A whitespace string does name one: the engine looks it
+ * up and answers 404 when no row has it. ⛔ This is not an id FORMAT check.
+ *
+ * A `{ $ref }` is not judged here. It is resolved inside the transaction, and
+ * a reference to no earlier create is refused there (`BATCH_UNRESOLVED_REF`).
+ */
+function namesNoRow(action: 'update' | 'delete', address: unknown): boolean {
+    if (isBatchRef(address)) return false;
+    return action === 'delete'
+        ? resolveEngineDeleteDispatch({ where: { id: address } }).kind !== 'by-id'
+        : resolveEngineUpdateDispatch({ id: address }, undefined).kind !== 'by-id';
+}
+
 /**
  * Register batch operation endpoints — `RestServer#registerBatchEndpoints`'
  * body, called by that method with the server as `this`. The method's own
@@ -185,7 +246,7 @@ export function registerBatchEndpoints(this: BatchRouteHost, basePath: string):
                     res.status(400).json({ error: 'Invalid batch request', code: 'VALIDATION_FAILED', issues: parsed.error?.issues });
                     return;
                 }
-                const ops: Array<{ object: string; action: 'create' | 'update' | 'delete'; id?: string; data?: Record<string, any> }> = parsed.data.operations;
+                const ops: CrossObjectBatchOp[] = parsed.data.operations;
                 // All-or-nothing by construction: refuse a request that asks for
                 // non-atomic semantics rather than silently applying atomically
                 // (honest contract). Per-object partial batches use the
@@ -201,9 +262,11 @@ export function registerBatchEndpoints(this: BatchRouteHost, basePath: string):
                 if (this.enforceBatchSize(res, ops.length, maxBatch)) return;
 
                 // update/delete need a target id — the schema can't express this
-                // conditionally, so surface it as a 400 up front.
+                // conditionally, so surface it as a 400 up front. "Missing" is
+                // the address the op's arm would hand the engine naming no row
+                // (`namesNoRow`): a blank id gets the answer an absent one does.
                 for (const op of ops) {
-                    if ((op.action === 'update' || op.action === 'delete') && op.id == null && op.data?.id == null) {
+                    if ((op.action === 'update' || op.action === 'delete') && namesNoRow(op.action, batchOpRowAddress(op, op.data))) {
                         res.status(400).json({ error: `Operation '${op.action}' on '${op.object}' requires an id`, code: 'VALIDATION_FAILED' });
                         return;
                     }
@@ -240,8 +303,8 @@ export function registerBatchEndpoints(this: BatchRouteHost, basePath: string):
                     if (!data || typeof data !== 'object') return data;
                     const result: any = Array.isArray(data) ? [] : {};
                     for (const [k, v] of Object.entries(data)) {
-                        if (v && typeof v === 'object' && '$ref' in (v as any)) {
-                            const idx = (v as any).$ref;
+                        if (isBatchRef(v)) {
+                            const idx = v.$ref;
                             const ref = typeof idx === 'number' ? out[idx] : undefined;
                             const refId = ref && (ref.id ?? ref._id);
                             if (refId == null) {
@@ -309,7 +372,7 @@ export function registerBatchEndpoints(this: BatchRouteHost, basePath: string):
                             // and `readonlyWhen` (#3042) on its own update path,
                             // and reports them through this listener.
                             const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); };
-                            const id = op.id ?? data?.id;
+                            const id = batchOpRowAddress(op, data);
                             const updated = await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped });
                             // [#7823] …but the RESPONSE half moved to the ingress
                             // (A-prime ruling, 2026-08-13): the engine keeps its
@@ -330,7 +393,7 @@ export function registerBatchEndpoints(this: BatchRouteHost, basePath: string):
                                 out.push(updated);
                             }
                         } else { // 'delete'
-                            out.push(await ql.delete(op.object, { where: { id: op.id }, context: trxCtx }));
+                            out.push(await ql.delete(op.object, { where: { id: batchOpRowAddress(op, data) }, context: trxCtx }));
                         }
                     }
                     return out;

packages/rest/src/rest-batch-blank-id.test.ts

@@ -0,0 +1,187 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `POST /api/v1/batch` — an `update` / `delete` op whose row address names no
+ * row is refused up front with the missing-id answer, on a real engine
+ * (`ObjectQL` + sqlite `SqlDriver`), the real protocol and the real
+ * `RestServer` route, with `objectQLProvider` wired so the cross-object door is
+ * reachable rather than answering 501.
+ *
+ * The door's up-front check read `op.id == null && op.data?.id == null`, so an
+ * address the engine does not take as an id — the empty string, `0`, a
+ * non-scalar — passed it, reached `ql.update` / `ql.delete`, and the engine's
+ * `Update requires an ID…` / `Delete requires an ID…` throw answered as a
+ * server fault. And the check accepted `data.id` for a `delete`, whose write
+ * arm never read it. Both are now judged once: the address each arm hands the
+ * engine (`batchOpRowAddress`), against the engine's own by-id predicate.
+ *
+ * Measured on `origin/main` a43405d59 with this harness, before the fix: every
+ * REFUSED row below answered `500` — `DATABASE_ERROR` on an update arm,
+ * `INTERNAL_ERROR` on a delete arm — and wrote nothing (the transaction rolled
+ * the batch back). They now answer `400 VALIDATION_FAILED` with the sentence a
+ * missing id gets, before the transaction opens.
+ *
+ * Controls: a missing id answers as before; a real id updates and deletes as
+ * before (by `id`, and an update by `data.id`); an update whose `data.id` is a
+ * `{ $ref }` to an earlier create still resolves; a whitespace id is an id the
+ * engine looks up, so it answers what an unknown id answers (`404`), not this
+ * refusal.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { ObjectQL } from '@objectstack/objectql';
+import { SqlDriver } from '@objectstack/driver-sql';
+import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
+import { RestServer } from './rest-server';
+
+const OBJ = {
+  name: 'blank_id_task', label: 'Task', systemFields: false,
+  fields: {
+    id: { name: 'id', type: 'text' as const, primaryKey: true },
+    title: { name: 'title', type: 'text' as const },
+  },
+};
+
+const liveEngines: ObjectQL[] = [];
+afterEach(async () => {
+  while (liveEngines.length) {
+    try { await liveEngines.pop()?.destroy(); } catch { /* noop */ }
+  }
+});
+
+function createMockServer() {
+  const noop = () => {};
+  return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
+}
+
+function makeRes() {
+  const res: any = {
+    write: () => true, end: () => {},
+    header: () => res,
+    status: (code: number) => { res._status = code; return res; },
+    json: (body: any) => { res._json = body; return res; },
+  };
+  return res;
+}
+
+async function boot() {
+  const engine = new ObjectQL();
+  liveEngines.push(engine);
+  const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true });
+  engine.registerDriver(driver, true);
+  await engine.init();
+  engine.registry.registerObject(OBJ as any);
+  await engine.syncSchemas();
+  await engine.insert('blank_id_task', { id: 'row-1', title: 'Kept' });
+
+  const protocol = new ObjectStackProtocolImplementation(engine as any);
+  const rest = new RestServer(
+    createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any,
+    undefined, undefined, undefined, undefined, // kernelManager, envRegistry, defaultEnvIdProvider, authServiceProvider
+    (async () => engine) as any,                // objectQLProvider (positional arg #8)
+  );
+  (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
+  rest.registerRoutes();
+  const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/batch');
+  expect(route, 'POST /api/v1/batch').toBeDefined();
+  const transaction = vi.spyOn(engine, 'transaction');
+  const batch = async (operations: unknown[], extra: Record<string, unknown> = {}) => {
+    const res = makeRes();
+    await route!.handler({ method: 'POST', params: {}, query: {}, headers: {}, body: { operations, ...extra } } as any, res);
+    return { status: res._status ?? 200, body: res._json };
+  };
+  const row = (id: string) => engine.findOne('blank_id_task', { where: { id } });
+  return { batch, row, transaction };
+}
+
+const O = 'blank_id_task';
+const missingId = (action: 'update' | 'delete') => ({
+  error: `Operation '${action}' on '${O}' requires an id`,
+  code: 'VALIDATION_FAILED',
+});
+
+/** Every op here names no row the engine could address; the arm is the op's own. */
+const REFUSED: ReadonlyArray<readonly [string, 'update' | 'delete', Record<string, unknown>]> = [
+  ['update, id ""', 'update', { id: '', data: { title: 'Changed' } }],
+  ['update, data.id ""', 'update', { data: { id: '', title: 'Changed' } }],
+  ['update, id "" beside a real data.id (the arm reads id first)', 'update', { id: '', data: { id: 'row-1', title: 'Changed' } }],
+  ['update, data.id 0', 'update', { data: { id: 0, title: 'Changed' } }],
+  ['update, data.id false', 'update', { data: { id: false, title: 'Changed' } }],
+  ['update, data.id true', 'update', { data: { id: true, title: 'Changed' } }],
+  ['update, data.id {}', 'update', { data: { id: {}, title: 'Changed' } }],
+  ['update, data.id []', 'update', { data: { id: [], title: 'Changed' } }],
+  ['delete, id ""', 'delete', { id: '' }],
+  ['delete, id "" beside a real data.id', 'delete', { id: '', data: { id: 'row-1' } }],
+  ['delete, a real id in data.id only (the delete arm never read data.id)', 'delete', { data: { id: 'row-1' } }],
+];
+
+describe('POST /api/v1/batch: an update or delete op that names no row answers the missing-id 400', () => {
+  let ctx: Awaited<ReturnType<typeof boot>>;
+  beforeEach(async () => { ctx = await boot(); });
+
+  it.each(REFUSED)('%s → 400 VALIDATION_FAILED, before the transaction opens, nothing written', async (_l, action, op) => {
+    const res = await ctx.batch([{ object: O, action, ...op }], { atomic: true });
+    expect({ status: res.status, body: res.body }).toEqual({ status: 400, body: missingId(action) });
+    expect(ctx.transaction).not.toHaveBeenCalled();
+    expect(await ctx.row('row-1')).toMatchObject({ id: 'row-1', title: 'Kept' });
+  });
+
+  it('refuses the whole batch up front: an earlier create in it is never written', async () => {
+    const res = await ctx.batch([
+      { object: O, action: 'create', data: { id: 'row-2', title: 'New' } },
+      { object: O, action: 'update', id: '', data: { title: 'Changed' } },
+    ]);
+    expect({ status: res.status, body: res.body }).toEqual({ status: 400, body: missingId('update') });
+    expect(ctx.transaction).not.toHaveBeenCalled();
+    expect(await ctx.row('row-2')).toBeFalsy();
+  });
+
+  // ── controls: answers that do not move ────────────────────────────────────
+
+  it.each([
+    ['update', { data: { title: 'Changed' } }],
+    ['delete', {}],
+  ] as const)('control: %s with no id at all answers the same 400', async (action, op) => {
+    const res = await ctx.batch([{ object: O, action, ...op }]);
+    expect({ status: res.status, body: res.body }).toEqual({ status: 400, body: missingId(action) });
+    expect(await ctx.row('row-1')).toMatchObject({ title: 'Kept' });
+  });
+
+  it('control: an update by id writes the row', async () => {
+    const res = await ctx.batch([{ object: O, action: 'update', id: 'row-1', data: { title: 'By id' } }]);
+    expect(res.status).toBe(200);
+    expect(await ctx.row('row-1')).toMatchObject({ title: 'By id' });
+  });
+
+  it('control: an update by data.id writes the row', async () => {
+    const res = await ctx.batch([{ object: O, action: 'update', data: { id: 'row-1', title: 'By data.id' } }]);
+    expect(res.status).toBe(200);
+    expect(await ctx.row('row-1')).toMatchObject({ title: 'By data.id' });
+  });
+
+  it('control: an update whose data.id is a $ref to an earlier create still resolves', async () => {
+    const res = await ctx.batch([
+      { object: O, action: 'create', data: { id: 'row-3', title: 'Created' } },
+      { object: O, action: 'update', data: { id: { $ref: 0 }, title: 'Then updated' } },
+    ]);
+    expect(res.status).toBe(200);
+    expect(await ctx.row('row-3')).toMatchObject({ title: 'Then updated' });
+  });
+
+  it('control: a delete by id deletes the row', async () => {
+    const res = await ctx.batch([{ object: O, action: 'delete', id: 'row-1' }]);
+    expect(res.status).toBe(200);
+    expect(await ctx.row('row-1')).toBeFalsy();
+  });
+
+  it.each([
+    ['an unknown id', 'nope'],
+    ['a whitespace id (an id the engine looks up)', '  '],
+  ])('control: update and delete with %s answer 404 RECORD_NOT_FOUND, not this refusal', async (_l, id) => {
+    for (const op of [{ action: 'update', id, data: { title: 'Changed' } }, { action: 'delete', id }]) {
+      const res = await ctx.batch([{ object: O, ...op }]);
+      expect([op.action, res.status, res.body?.code]).toEqual([op.action, 404, 'RECORD_NOT_FOUND']);
+    }
+    expect(await ctx.row('row-1')).toMatchObject({ title: 'Kept' });
+  });
+});