From 027961d3bf81015f5122e02810a3ac331382c2ac Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 07:15:24 +0000 Subject: [PATCH 1/3] fix(rest): POST /batch refuses a blank update/delete id with the missing-id 400 The cross-object batch's up-front id 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 boolean, an object or an array in data.id) passed it and the engine's "requires an ID" throw answered 500. A delete naming its id only in data.id passed it too, though the delete arm never read data.id. The check now judges the address each arm hands the engine (batchOpRowAddress), with the engine's own by-id predicates, and both arms read that same address. Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude --- packages/rest/src/batch-routes.ts | 77 +++++++- packages/rest/src/rest-batch-blank-id.test.ts | 187 ++++++++++++++++++ 2 files changed, 257 insertions(+), 7 deletions(-) create mode 100644 packages/rest/src/rest-batch-blank-id.test.ts diff --git a/packages/rest/src/batch-routes.ts b/packages/rest/src/batch-routes.ts index 8634e23bc36..c2ad0e6dac7 100644 --- a/packages/rest/src/batch-routes.ts +++ b/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; +}; + +/** + * A field value of the form `{ $ref: }`: 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 | 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 }> = 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; diff --git a/packages/rest/src/rest-batch-blank-id.test.ts b/packages/rest/src/rest-batch-blank-id.test.ts new file mode 100644 index 00000000000..15e48948f04 --- /dev/null +++ b/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 = {}) => { + 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 } } as any); + 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]> = [ + ['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>; + 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' }); + }); +}); From 3c1d02dc6bdd8a5b22ed9e3684c4b81f26a79f8f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 07:17:44 +0000 Subject: [PATCH 2/3] chore(changeset): @objectstack/rest minor for the batch blank-id narrowing Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude --- .changeset/24781-rest-batch-blank-id-400.md | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .changeset/24781-rest-batch-blank-id-400.md diff --git a/.changeset/24781-rest-batch-blank-id-400.md b/.changeset/24781-rest-batch-blank-id-400.md new file mode 100644 index 00000000000..ca4482a4c16 --- /dev/null +++ b/.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) + + + +**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. From 7ee83bccbbe0d035656ec01e9ba92b0e01f13537 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 07:45:50 +0000 Subject: [PATCH 3/3] test(rest): type the blank-id pin's read-back query options Claude-Session: https://claude.ai/code/session_01LmjU2x1faLZ39DVTFyVca6 Co-authored-by: Claude --- packages/rest/src/rest-batch-blank-id.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rest/src/rest-batch-blank-id.test.ts b/packages/rest/src/rest-batch-blank-id.test.ts index 15e48948f04..64f091a3ae8 100644 --- a/packages/rest/src/rest-batch-blank-id.test.ts +++ b/packages/rest/src/rest-batch-blank-id.test.ts @@ -90,7 +90,7 @@ async function boot() { 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 } } as any); + const row = (id: string) => engine.findOne('blank_id_task', { where: { id } }); return { batch, row, transaction }; }