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. 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..64f091a3ae8 --- /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 } }); + 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' }); + }); +});