diff --git a/.changeset/23491-patch-data-request-gate.md b/.changeset/23491-patch-data-request-gate.md new file mode 100644 index 00000000000..686fac2c406 --- /dev/null +++ b/.changeset/23491-patch-data-request-gate.md @@ -0,0 +1,27 @@ +--- +'@objectstack/rest': minor +'@objectstack/runtime': minor +--- + +fix(rest,runtime)!: both `/data` write doors validate the request as the caller sent it: a `PATCH` or `POST` create whose body is not a record, and a `PATCH` whose `expectedVersion` is a number, are refused `400 VALIDATION_FAILED` on both doors before the engine is asked + +`RestServer` validates `PATCH /data/:object/:id` against `UpdateDataRequestSchema` and `POST /data/:object` against `CreateDataRequestSchema`. The runtime dispatcher's `/data` domain, the only answer on a host that mounts just the `@objectstack/hono` catch-all, validated neither, so a body that is not a record reached the engine. On a `PATCH` it answered `500` with the engine's own error. On a `POST`, an array created one row per element and a number created an empty row, both answered `201`. `RestServer`'s own `PATCH` gate parsed a coerced copy of the request: it read a JSON `null` body as `{}`, and it read a numeric `expectedVersion` as its string. Both doors now validate the assembled request as the caller sent it, and they answer each case alike. + +Clause-②: no + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. + +What changes for you, wherever `@objectstack/rest` or the runtime dispatcher serves these routes: + +- `RestServer`, `PATCH /data/:object/:id` with a JSON `null` body: FROM `200`, an update that changed no field and still moved the record's `updated_at` (so every other holder of the old version saw a conflict on its next guarded save), TO `400 VALIDATION_FAILED` naming `data`, the record unchanged. The runtime dispatcher answers the same. +- The runtime dispatcher, `PATCH /data/:object/:id` with an array, a string, a number or `null` as the body: FROM `500` carrying the engine's own error text, nothing written, TO `400 VALIDATION_FAILED` naming `data`, before the engine is asked. `RestServer` already answered `400` for the first three. +- The runtime dispatcher, `POST /data/:object` with an array, a string or a number as the body: FROM `201` with one row created per array element, `201` with an empty row for a number, and the engine's own `400 INVALID_FIELD` for a string, TO `400 VALIDATION_FAILED` naming `data`, with no row created. `RestServer` already answered this. +- Both doors, `PATCH /data/:object/:id` with a body `expectedVersion` that is a number (`5`, `0`): FROM judged as its string (`'5'`) and answered `409 CONCURRENT_UPDATE`, TO `400 VALIDATION_FAILED` before the protocol is asked, whatever `If-Match` is sent beside it. The request schema declares the token a string, and a number is now one of the token shapes that can never carry a version. + +How it renders: `RestServer` answers `{ error, code: 'VALIDATION_FAILED', fields, object }`, the envelope its request gate already used. The runtime dispatcher returns its own `400 VALIDATION_FAILED` envelope, `fields` under `error.details`, instead of throwing, so a `@objectstack/hono` `createHonoApp` host renders it as `400`. + +Unchanged: a record body, `{}` included; a string `expectedVersion` (judged; `409` on a mismatch); `If-Match` and the `DELETE`'s `?expectedVersion`, which are strings by transport; and a JSON `null` body on a `POST` create, which both doors still read as an empty record and answer `201`. + +To send a patch or a row, send the fields as a JSON object. To guard a write, send the version you read (the record's `updated_at`) as a string. The `@objectstack/client` SDK's `data.update` sends the version as an `If-Match` header string, so an SDK caller is affected only when the `data` it passes is not an object, or carries a number `expectedVersion`. diff --git a/packages/rest/src/request-schema-gate.conformance.test.ts b/packages/rest/src/request-schema-gate.conformance.test.ts index e8b50e96506..bbbf3fb9f5b 100644 --- a/packages/rest/src/request-schema-gate.conformance.test.ts +++ b/packages/rest/src/request-schema-gate.conformance.test.ts @@ -131,7 +131,9 @@ const CASES: GateCase[] = [ { route: 'PATCH /api/v1/data/:object/:id', params: PARAMS_ID, - violating: ['garbage', 42, [{ name: 'x' }]], + // #23491 — `null` was read as an empty patch (`data ?? {}`) and answered + // 200, an update that changed no field and still moved the version. + violating: ['garbage', 42, [{ name: 'x' }], null], valid: { name: 'ACME (renamed)' }, spy: 'updateData', }, diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d69a2b87312..a22d641127c 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -1743,17 +1743,25 @@ function sendMetaItemAbsent(res: any): void { * - ABSENT: the spelling consulted is `undefined` or `null`. The body key or * the query name falls through to `If-Match`, and no `If-Match` either is * the legal unguarded write. - * - JUDGED: a string with something in it, or a finite number. The door - * forwards its string, and the protocol compares it (`409 - * CONCURRENT_UPDATE` on a mismatch; the quoted-empty tag `""` is the - * protocol's own `400`, `MalformedVersionTokenError`). + * - JUDGED: a string with something in it. The door forwards it, and the + * protocol compares it (`409 CONCURRENT_UPDATE` on a mismatch; the + * quoted-empty tag `""` is the protocol's own `400`, + * `MalformedVersionTokenError`). * - REFUSED `400 VALIDATION_FAILED`: anything else the caller put in a token - * position — an empty or blank string, a boolean, an array, an object. It - * can never carry a version, so it is the protocol's ruled answer to `""` - * (maintainer decision #20 ①: a token shape that can never carry a version - * is refused, neither read as "no token" nor answered `409`), given here - * because the protocol never sees these: it reads a blank string as no - * token. + * position — an empty or blank string, a number, a boolean, an array, an + * object. It can never carry a version, so it is the protocol's ruled + * answer to `""` (maintainer decision #20 ①: a token shape that can never + * carry a version is refused, neither read as "no token" nor answered + * `409`), given here because the protocol never sees these: it reads a + * blank string as no token. + * + * [#23517] A number is in the refused set, no longer judged as its string. + * `UpdateDataRequestSchema.expectedVersion` declares a string, and the door + * forwarded `String(5)`, so the request gate below only ever parsed the + * coerced copy and a token the contract never admits was answered `409` as if + * it were a stale version. Only a body token can be a number: `If-Match` and + * `?expectedVersion` are strings by transport, so the `DELETE` and every + * header token are unaffected. * * What it replaces is a truthiness test. The door forwarded a token only when * it was truthy, and `bodyVersion ?? If-Match` reads `If-Match` only when the @@ -1779,14 +1787,13 @@ function sendMetaItemAbsent(res: any): void { function versionTokenRefusal(token: unknown, spelling: string): string | undefined { if (token === undefined || token === null) return undefined; if (typeof token === 'string' && token.trim() !== '') return undefined; - if (typeof token === 'number' && Number.isFinite(token)) return undefined; const shape = typeof token === 'string' ? 'empty' : Array.isArray(token) ? 'an array' : typeof token === 'object' ? 'an object' : `the ${typeof token} \`${String(token)}\``; return `${spelling} is ${shape}, which can never carry a version, so it is refused rather than read as ` - + 'no token. Send the version you read (the record\'s `updated_at`), or omit both expectedVersion and ' - + 'If-Match to perform an unguarded write.'; + + 'no token. Send the version you read as a string (the record\'s `updated_at`), or omit both ' + + 'expectedVersion and If-Match to perform an unguarded write.'; } /** @@ -9173,12 +9180,29 @@ export class RestServer { // validated as the assembled `UpdateDataRequestSchema` // request so a non-record body 400s instead of reaching // the engine. + // + // [#23491] …the patch AS THE CALLER SENT IT. It was + // `data ?? {}`, so a JSON `null` body parsed as an empty + // patch and answered `200`: an update that changed no + // field and still moved the record's version. `null` is + // not the record the contract declares, so it is refused + // like an array, a string or a number, and the runtime + // dispatcher's twin (`domains/data.ts`) asks this gate + // over the same assembled request. + // + // [#23517] …and the token as the caller sent it, never + // its `String()`: this gate parsed `String(5)`, so a + // number `expectedVersion: z.string()` declares no room + // for was forwarded and judged `409`. + // `versionTokenRefusal` above already refuses it, so what + // reaches this gate and the forward below is a non-blank + // string. const { UpdateDataRequestSchema } = await import('@objectstack/spec/api'); const updateInput = { object: req.params.object, id: req.params.id, - data: data ?? {}, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), + data, + ...(expectedVersion != null ? { expectedVersion } : {}), }; const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput); if (!parsedUpdateOne.success) { @@ -9193,8 +9217,8 @@ export class RestServer { const updateRequest: ServerScopedDataRequest = { object: req.params.object, id: req.params.id, - data: data ?? {}, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), + data, + ...(expectedVersion != null ? { expectedVersion } : {}), ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }; diff --git a/packages/runtime/src/domains/data-update-expected-version.test.ts b/packages/runtime/src/domains/data-update-expected-version.test.ts index 2a8ae9866ec..26dd3571f2b 100644 --- a/packages/runtime/src/domains/data-update-expected-version.test.ts +++ b/packages/runtime/src/domains/data-update-expected-version.test.ts @@ -294,12 +294,12 @@ for (const [doorName, boot] of DOORS) { * row. What either door does with them is `RestServer`'s reading, not this * file's: a token is the body key when it is not nullish (else `If-Match`), * the key is stripped from the patch whatever its value, and a token is - * forwarded as its string unless it can never carry a version, which is - * refused (#23492, pinned in its own block below). + * forwarded unless it can never carry a version, which is refused (#23492, + * pinned in its own block below; a number among them since #23517, pinned in + * the block after it). */ const EDGE_SHAPES: ReadonlyArray PatchOptions]> = [ ['a stale token', () => ({ body: { name: AFTER, expectedVersion: STALE } })], - ['a number token', () => ({ body: { name: AFTER, expectedVersion: 20000101 } })], ['a null body token beside a stale If-Match', () => ({ body: { name: AFTER, expectedVersion: null }, headers: { 'If-Match': STALE } })], ['a null body token beside the current If-Match', (v) => ({ body: { name: AFTER, expectedVersion: null }, headers: { 'If-Match': v } })], ['an empty-string body token beside a stale If-Match', () => ({ body: { name: AFTER, expectedVersion: '' }, headers: { 'If-Match': STALE } })], @@ -346,7 +346,8 @@ describe('[#23462] the two doors answer each token shape with the same status, c * position that can never carry a version is refused with the protocol's own * answer to the quoted-empty tag `""`, `400 VALIDATION_FAILED`, before * `updateData` is asked; maintainer decision #20 ① rules that shape refused, - * neither read as "no token" nor answered `409`. + * neither read as "no token" nor answered `409`. A number joined this set + * with #23517 and has its own block below. */ const UNUSABLE: ReadonlyArray = [ ["a body expectedVersion of '' beside a stale If-Match", { body: { name: AFTER, expectedVersion: '' }, headers: { 'If-Match': STALE } }], @@ -391,10 +392,13 @@ for (const [doorName, boot] of DOORS) { expect(await door.name()).toBe(BEFORE); }, 60_000); - it('control: a body token of 0 is a number, judged as "0" (refused 409), never dropped', async () => { + it('control: a body token of 0 is never dropped: it is refused 400, so the current If-Match beside it is not read', async () => { + // [#23517] It was judged as '0' (409) until a number joined the + // refused set. Either way it is not read as "no token": had it + // been dropped, the current If-Match would have applied the write. const door = await boot(); - conflict(await door.patch({ body: { name: AFTER, expectedVersion: 0 }, headers: { 'If-Match': door.version } })); - expect(door.forwarded()).toEqual([{ expectedVersion: '0', data: { name: AFTER } }]); + refused(await door.patch({ body: { name: AFTER, expectedVersion: 0 }, headers: { 'If-Match': door.version } })); + expect(door.forwarded()).toEqual([]); expect(await door.name()).toBe(BEFORE); }, 60_000); }); @@ -411,6 +415,71 @@ describe('[#23492] the two doors refuse each token that can never carry a versio } }); +// ── A number token ──────────────────────────────────────────────────────────── + +/** + * [#23517] `UpdateDataRequestSchema.expectedVersion` declares a string. Both + * doors forwarded a number as `String(token)`, so `RestServer`'s request gate + * only ever parsed the coerced copy and the dispatcher had no gate: a body + * `expectedVersion: 5` was judged as `'5'` and answered `409 + * CONCURRENT_UPDATE` on both doors (measured on this rig before the fix, + * beside a stale and beside the current `If-Match`; the row unchanged). Now a + * number is refused `400 VALIDATION_FAILED` before `updateData` is asked, the + * answer maintainer decision #20 ① gives a token shape that can never carry a + * version. The `If-Match` beside it is never read: the body key is the token + * whenever it is not nullish. + */ +const NUMBER_TOKENS: ReadonlyArray = [['5', 5], ['0', 0]]; +const IF_MATCH_BESIDE: ReadonlyArray string]> = [ + ['a stale If-Match', () => STALE], + ['the current If-Match', (version) => version], +]; + +for (const [doorName, boot] of DOORS) { + describe(`[#23517] ${doorName}: PATCH /data/:object/:id refuses a number expectedVersion`, () => { + for (const [tokenName, token] of NUMBER_TOKENS) { + for (const [besideName, ifMatch] of IF_MATCH_BESIDE) { + it(`expectedVersion: ${tokenName} beside ${besideName}: 400 VALIDATION_FAILED, updateData never asked, the row unchanged`, async () => { + const door = await boot(); + refused(await door.patch({ body: { name: AFTER, expectedVersion: token }, headers: { 'If-Match': ifMatch(door.version) } })); + expect(door.forwarded()).toEqual([]); + expect(await door.name()).toBe(BEFORE); + }, 60_000); + } + } + + it('control: the same token as a string is judged, not refused (409 against a stale version)', async () => { + const door = await boot(); + conflict(await door.patch({ body: { name: AFTER, expectedVersion: '5' }, headers: { 'If-Match': door.version } })); + expect(door.forwarded()).toEqual([{ expectedVersion: '5', data: { name: AFTER } }]); + expect(await door.name()).toBe(BEFORE); + }, 60_000); + + it('control: If-Match alone is a string by transport, so the current one still updates', async () => { + const door = await boot(); + updated(await door.patch({ body: { name: AFTER }, headers: { 'If-Match': door.version } })); + expect(door.forwarded()).toEqual([{ expectedVersion: door.version, data: { name: AFTER } }]); + expect(await door.name()).toBe(AFTER); + }, 60_000); + }); +} + +describe('[#23517] the two doors refuse a number expectedVersion alike', () => { + for (const [tokenName, token] of NUMBER_TOKENS) { + for (const [besideName, ifMatch] of IF_MATCH_BESIDE) { + it(`expectedVersion: ${tokenName} beside ${besideName}: the same status, code and sentence`, async () => { + const answers: Answer[] = []; + for (const [, boot] of DOORS) { + const door = await boot(); + answers.push(await door.patch({ body: { name: AFTER, expectedVersion: token }, headers: { 'If-Match': ifMatch(door.version) } })); + } + const [rest, catchAllAnswer] = answers; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } + } +}); + // ── The dispatcher's fallback updater ───────────────────────────────────────── /** diff --git a/packages/runtime/src/domains/data-write-request-gate.test.ts b/packages/runtime/src/domains/data-write-request-gate.test.ts new file mode 100644 index 00000000000..7667fc84dab --- /dev/null +++ b/packages/runtime/src/domains/data-write-request-gate.test.ts @@ -0,0 +1,312 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23491] Both doors of the `/data` single-record writes validate the + * assembled protocol request before the engine is asked: `RestServer`'s + * registered routes and the runtime dispatcher's `/data` domain, the only + * answer on a host that mounts just `@objectstack/hono`'s `${prefix}/*` + * catch-all. + * + * ## What was wrong + * + * `RestServer` validates `PATCH /data/:object/:id` as `UpdateDataRequestSchema` + * and `POST /data/:object` as `CreateDataRequestSchema` (#3899), so a body + * that is not a record answers `400 VALIDATION_FAILED`. The dispatcher's two + * branches had no gate. Measured on this rig before the fix: + * + * - `PATCH` with an array, a string, a number or `null` answered `500 + * INTERNAL_ERROR` with the engine's own error text ("no such column: 0", + * "Cannot use 'in' operator …", "Cannot read properties of null …"), + * after `updateData` was asked; + * - `POST` with an array CREATED a row per element (`201`), a number created + * an empty row (`201`), and a string answered the engine's unknown-field + * `400 INVALID_FIELD`; + * - `RestServer`'s `PATCH` read a `null` body as `{}` (`data ?? {}`) and + * answered `200`: an update that changed no field and still moved the + * record's `updated_at`, so every other holder of the old version was + * turned stale by a request that wrote nothing. + * + * The `null` body now answers `400` on both `PATCH` doors: the declared type + * is a record, and one door reading it as `{}` was the consumer-side leniency + * the contract-first rule forbids. A `null` CREATE is the one shape left as it + * was: `RestServer`'s create reads it as `{}`, the dispatcher's gate asks the + * same assembled request, and the two doors answer it alike (pinned below as + * parity only). + * + * ## The rig + * + * The one `data-update-expected-version.test.ts` describes: each door gets + * its own REAL store (better-sqlite3 `:memory:`, the real `ObjectQL` and a + * real `ObjectStackProtocolImplementation`); `RestServer` is driven through + * its registered handler with the request `plugin-hono-server` builds, and + * the dispatcher through `HttpDispatcher.dispatch()` with the body parsed the + * way the catch-all parses it (`JSON.parse` of the request text, any failure + * `{}`). Every row asserts the store, not just the door's word. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from '@objectstack/rest'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const PREFIX = '/api/v1'; +const DATA = `${PREFIX}/data`; +/** `registry.registerObject` requires a package id (see the sibling real-stack tests). */ +const TEST_PACKAGE_ID = 'objectstack-test'; +const CALLER = { userId: 'u_member', isSystem: false }; + +const TASK = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' }, + name: { name: 'name', type: 'text' as const, label: 'Name' }, + }, +}; + +const BEFORE = 'first'; +const AFTER = 'second'; + +const clone = (v: T): T => (v === undefined ? v : JSON.parse(JSON.stringify(v))); + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** What the store holds after a request: `r1`'s name and version, and the row count. */ +interface StoreState { name: unknown; version: unknown; rows: number } + +/** One real store with one `task` row, `r1`, named {@link BEFORE}. */ +async function bootStore() { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + engine.registry.registerObject(TASK as any, TEST_PACKAGE_ID); + await engine.syncSchemas(); + const protocol = new ObjectStackProtocolImplementation(engine as any); + await protocol.createData({ object: 'task', data: { id: 'r1', name: BEFORE } } as any); + const updateData = vi.spyOn(protocol, 'updateData'); + const createData = vi.spyOn(protocol, 'createData'); + const state = async (): Promise => { + const row: any = await engine.findOne('task', { where: { id: 'r1' } }); + const rows = ((await engine.find('task', {})) as unknown[]).length; + const v = row?.updated_at; + return { name: row?.name, version: v instanceof Date ? v.toISOString() : v, rows }; + }; + return { protocol, updateData, createData, state }; +} + +// ── The two doors ───────────────────────────────────────────────────────────── + +interface Answer { status: number; code?: string; fields?: string[] } +interface Door { + patch(body: unknown): Promise; + create(body: unknown): Promise; + state(): Promise; + updateCalls(): number; + createCalls(): number; +} + +/** The refusal's `code` in either door's envelope: nested under `error`, or beside a string `error`. */ +const codeOf = (body: any): string | undefined => + body?.error && typeof body.error === 'object' ? body.error.code : body?.code; +/** The field names a refusal carries: `RestServer`'s top-level `fields`, the dispatcher's `error.details.fields`. */ +const fieldsOf = (body: any): string[] | undefined => { + const fields = Array.isArray(body?.fields) ? body.fields : body?.error?.details?.fields; + return Array.isArray(fields) ? fields.map((f: any) => f?.field).sort() : undefined; +}; + +function makeRes() { + const res: any = { statusCode: 200, body: undefined, headers: {} as Record }; + res.status = (code: number) => { res.statusCode = code; return res; }; + res.json = (body: any) => { res.body = body; return res; }; + res.send = () => res; + res.end = () => res; + res.header = (k: string, v: string) => { res.headers[k] = v; return res; }; + res.setHeader = () => {}; + res.write = () => true; + return res; +} + +async function bootRestServer(): Promise { + const { protocol, updateData, createData, state } = await bootStore(); + const noop = () => {}; + const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; + const rest: any = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any); + rest.resolveExecCtx = async () => clone(CALLER); + rest.registerRoutes(); + const routeOf = (method: string, path: string) => { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} is not registered`); + return route; + }; + const update = routeOf('PATCH', `${DATA}/:object/:id`); + const create = routeOf('POST', `${DATA}/:object`); + const drive = async (route: any, path: string, params: Record, body: unknown): Promise => { + const res = makeRes(); + // `plugin-hono-server` hands a JSON body over as `c.req.json()` parsed it: `null` stays `null`. + await route.handler({ method: route.method, path, params, headers: { 'content-type': 'application/json' }, query: {}, body: clone(body) }, res); + return { status: res.statusCode, code: codeOf(res.body), fields: fieldsOf(res.body) }; + }; + return { + state, + updateCalls: () => updateData.mock.calls.length, + createCalls: () => createData.mock.calls.length, + patch: (body) => drive(update, `${DATA}/task/r1`, { object: 'task', id: 'r1' }, body), + create: (body) => drive(create, `${DATA}/task`, { object: 'task' }, body), + }; +} + +/** + * What `createHonoApp`'s `${prefix}/*` catch-all hands `dispatch()` for a + * write: the path below the prefix, the JSON body parsed from the request's + * text (any failure is `{}`, the adapter's `readDispatchBody`), and the raw + * Fetch `Request` as `context.request`. + */ +async function catchAll(dispatcher: HttpDispatcher, method: string, path: string, body: unknown): Promise { + const raw = new Request(`http://localhost${PREFIX}${path}`, { + method, + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); + let parsed: unknown; + try { + parsed = JSON.parse(await raw.clone().text()); + } catch { + parsed = {}; + } + try { + const res: any = await dispatcher.dispatch(method, path, parsed, {}, { request: raw } as any, PREFIX); + const out = res.response?.body; + return { status: res.response?.status ?? 0, code: codeOf(out), fields: fieldsOf(out) }; + } catch (e) { + // A throw out of `dispatch()` is rendered by the host's exit; read it + // through the one rule the dispatcher's exits answer a throw with. + const thrown = resolveThrownHttpError(e); + return { status: thrown.status, code: thrown.code }; + } +} + +async function bootCatchAll(): Promise { + const { protocol, updateData, createData, state } = await bootStore(); + const services: Record = { protocol }; + const get = (n: string) => services[n] ?? null; + const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) }; + const dispatcher = new HttpDispatcher(kernel); + // The seam `dispatch()` resolves identity through (the sibling + // dispatch-level pins supply the principal the same way). + (dispatcher as any).timedResolveExecutionContext = async () => clone(CALLER); + return { + state, + updateCalls: () => updateData.mock.calls.length, + createCalls: () => createData.mock.calls.length, + patch: (body) => catchAll(dispatcher, 'PATCH', '/data/task/r1', body), + create: (body) => catchAll(dispatcher, 'POST', '/data/task', body), + }; +} + +const DOORS: ReadonlyArray Promise]> = [ + ['RestServer', bootRestServer], + ['the @objectstack/hono catch-all', bootCatchAll], +]; + +/** The JSON values a body can be that are not the record both request schemas declare for `data`. */ +const NOT_A_RECORD: ReadonlyArray = [ + ['an array', [{ name: AFTER }]], + ['a string', 'garbage'], + ['a number', 42], + ['null', null], +]; + +/** `400 VALIDATION_FAILED`, the refusal naming `data`: the request schema answered, not the engine. */ +const refusedOnData = (a: Answer) => + expect({ status: a.status, code: a.code, fields: a.fields }, JSON.stringify(a)) + .toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: ['data'] }); + +// ── PATCH /data/:object/:id ─────────────────────────────────────────────────── + +for (const [doorName, boot] of DOORS) { + describe(`[#23491] ${doorName}: PATCH /data/:object/:id refuses a body that is not a record`, () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: 400 VALIDATION_FAILED on data, updateData never asked, the row and its version unchanged`, async () => { + const door = await boot(); + const before = await door.state(); + refusedOnData(await door.patch(body)); + expect(door.updateCalls()).toBe(0); + expect(await door.state()).toEqual(before); + }, 60_000); + } + + it('control: a record body updates the row (200)', async () => { + const door = await boot(); + const answer = await door.patch({ name: AFTER }); + expect(answer.status, JSON.stringify(answer)).toBe(200); + expect(door.updateCalls()).toBe(1); + expect((await door.state()).name).toBe(AFTER); + }, 60_000); + }); +} + +describe('[#23491] the two PATCH doors refuse each non-record body alike', () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: the same status, code and fields`, async () => { + const answers: Answer[] = []; + for (const [, boot] of DOORS) answers.push(await (await boot()).patch(body)); + const [rest, catchAllAnswer] = answers; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } +}); + +// ── POST /data/:object ──────────────────────────────────────────────────────── + +/** The create's non-record bodies `RestServer`'s assembled request refuses; `null` is read as `{}` by both doors. */ +const CREATE_REFUSED = NOT_A_RECORD.filter(([, body]) => body !== null); + +for (const [doorName, boot] of DOORS) { + describe(`[#23491] ${doorName}: POST /data/:object refuses a body that is not a record`, () => { + for (const [shape, body] of CREATE_REFUSED) { + it(`${shape}: 400 VALIDATION_FAILED on data, createData never asked, no row created`, async () => { + const door = await boot(); + const before = await door.state(); + refusedOnData(await door.create(body)); + expect(door.createCalls()).toBe(0); + expect(await door.state()).toEqual(before); + }, 60_000); + } + + it('control: a record body creates one row (201)', async () => { + const door = await boot(); + const before = await door.state(); + const answer = await door.create({ name: AFTER }); + expect(answer.status, JSON.stringify(answer)).toBe(201); + expect((await door.state()).rows).toBe(before.rows + 1); + }, 60_000); + }); +} + +describe('[#23491] the two POST doors answer each non-record body alike', () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: the same status, code, fields and row count`, async () => { + const seen: Array = []; + for (const [, boot] of DOORS) { + const door = await boot(); + const answer = await door.create(body); + seen.push({ ...answer, rows: (await door.state()).rows }); + } + const [rest, catchAllAnswer] = seen; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } +}); diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 8804b5772d2..b2eaf6bfffa 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -36,7 +36,11 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. import { assertFilterParamSuppliedOnce, repeatedQueryParamMessage } from '@objectstack/rest'; // [#23492] The dispatcher's one constructor for a `400 VALIDATION_FAILED` // answer, which a refused version token returns in this transport's envelope. -import { validationFailure, VALIDATION_FAILED_STATUS } from '../validation-failure.js'; +// [#23491] …and the one Zod-issue → `fields[]` mapping, which a write whose +// assembled protocol request does not parse returns the same way. +import { validationFailure, fieldsFromZodIssues, VALIDATION_FAILED_STATUS } from '../validation-failure.js'; +// [#23491] The request contracts `RestServer`'s twin routes validate (#3899). +import { CreateDataRequestSchema, UpdateDataRequestSchema } from '@objectstack/spec/api'; /** * [#23265] The request's `If-Match`, from either header shape a host hands @@ -57,12 +61,18 @@ function requestIfMatch(headers: unknown): unknown { * [#23492] What this domain's `PATCH` and `DELETE` do with the * optimistic-concurrency token they read, before `callData` is asked: absent * (`undefined` or `null`, which falls through to `If-Match`), judged (a string - * with something in it, or a finite number, forwarded as its string), or - * refused `400 VALIDATION_FAILED` (an empty or blank string, a boolean, an - * array, an object: a token shape that can never carry a version, maintainer - * decision #20 ①). It replaces a truthiness test that let `expectedVersion: ''` - * or `false`, and `?expectedVersion=`, hide a stale `If-Match` and run the - * write unguarded. + * with something in it, forwarded as it is), or refused `400 + * VALIDATION_FAILED` (an empty or blank string, a number, a boolean, an array, + * an object: a token shape that can never carry a version, maintainer decision + * #20 ①). It replaces a truthiness test that let `expectedVersion: ''` or + * `false`, and `?expectedVersion=`, hide a stale `If-Match` and run the write + * unguarded. + * + * [#23517] A number is refused, no longer judged as its string: + * `UpdateDataRequestSchema.expectedVersion` declares a string, and forwarding + * `String(5)` answered `409` for a token the contract never admits. Only a + * body token can be a number; `If-Match` and `?expectedVersion` are strings by + * transport. * * `RestServer`'s twin routes ask the same function, statement for statement * (`versionTokenRefusal` in `packages/rest/src/rest-server.ts`, whose docblock @@ -74,14 +84,39 @@ function requestIfMatch(headers: unknown): unknown { function versionTokenRefusal(token: unknown, spelling: string): string | undefined { if (token === undefined || token === null) return undefined; if (typeof token === 'string' && token.trim() !== '') return undefined; - if (typeof token === 'number' && Number.isFinite(token)) return undefined; const shape = typeof token === 'string' ? 'empty' : Array.isArray(token) ? 'an array' : typeof token === 'object' ? 'an object' : `the ${typeof token} \`${String(token)}\``; return `${spelling} is ${shape}, which can never carry a version, so it is refused rather than read as ` - + 'no token. Send the version you read (the record\'s `updated_at`), or omit both expectedVersion and ' - + 'If-Match to perform an unguarded write.'; + + 'no token. Send the version you read as a string (the record\'s `updated_at`), or omit both ' + + 'expectedVersion and If-Match to perform an unguarded write.'; +} + +/** + * [#23491] This domain's answer to a `PATCH` or `POST` create whose assembled + * protocol request (`UpdateDataRequestSchema` / `CreateDataRequestSchema`) + * does not parse: `400 VALIDATION_FAILED`, `details.fields[]` naming each + * failing member, before `callData` is asked. `RestServer`'s twin routes gate + * the same assembled request (#3899); this domain had no gate, so a body that + * is not a record (an array, a string, a number, `null`) reached the engine + * as the patch or the row, and answered `500` with the engine's own error, or + * created rows. RETURNED, not thrown, for the reason `versionTokenRefusal`'s + * refusal is: a throw out of `dispatch()` reaches a `createHonoApp` host's + * catch-all, which renders only some declared statuses. + */ +function writeRequestRefusal( + deps: DomainHandlerDeps, + write: 'update' | 'create', + issues: Array<{ path: Array; code: string; message: string }>, + input: unknown, +): HttpDispatcherResult { + const fields = fieldsFromZodIssues(issues, input); + const message = `Invalid ${write} request: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`; + return { + handled: true, + response: deps.errorFromThrown(validationFailure(message, fields), VALIDATION_FAILED_STATUS), + }; } export function createDataDomain(deps: DomainHandlerDeps): DomainRoute { @@ -231,13 +266,33 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m response: deps.errorFromThrown(validationFailure(versionRefusal, []), VALIDATION_FAILED_STATUS), }; } - // Spec: returns UpdateDataResponse = { object, id, record } - const result = await actionExec.callData(deps, _context, 'update', { + // [#23491] The assembled request is validated as + // `UpdateDataRequestSchema`, the gate `RestServer`'s twin has + // asked since #3899, over the patch AS THE CALLER SENT IT (the + // token stripped, nothing defaulted): a body that is not a record + // is refused `400` here, before the engine is asked. It used to + // reach `updateData` as the patch and answer `500` with the + // engine's own error. A JSON `null` is refused like the rest: the + // twin used to read it as `{}` and answer `200`, an update that + // changed no field and still moved the record's version; both + // doors now refuse it. + // + // [#23517] …and the token as the caller sent it too, never its + // `String()`: the gate parsed `String(5)`, so a number the schema + // declares no room for was forwarded and judged `409`. + // `versionTokenRefusal` above already refuses it, so what reaches + // this gate and the forward below is a non-blank string. + const updateInput = { object: objectName, id, data, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), - }, _context.dataDriver, _context.environmentId, _context.executionContext); + ...(expectedVersion != null ? { expectedVersion } : {}), + }; + const parsedUpdate = UpdateDataRequestSchema.safeParse(updateInput); + if (!parsedUpdate.success) return writeRequestRefusal(deps, 'update', parsedUpdate.error.issues, updateInput); + // Spec: returns UpdateDataResponse = { object, id, record } + const result = await actionExec.callData(deps, _context, 'update', updateInput, + _context.dataDriver, _context.environmentId, _context.executionContext); return { handled: true, response: deps.success(result) }; } @@ -327,8 +382,21 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // POST /data/:object (Create) if (m === 'POST') { + // [#23491] The `PATCH` above's gate, on the create: the assembled + // request is validated as `CreateDataRequestSchema` before the + // engine is asked. This branch had no gate, so an array body + // created one row per element (`201`), a number created an empty + // row (`201`), and a string answered the engine's own unknown-field + // `400`; `RestServer`'s twin refused all three `400 + // VALIDATION_FAILED` (#3899). The assembled request is the twin's, + // statement for statement, `null` read as `{}` included: both doors + // answer a JSON `null` create alike (an empty row), and that + // leniency is retired on both at once or not at all. + const createInput = { object: objectName, data: body ?? {} }; + const parsedCreate = CreateDataRequestSchema.safeParse(createInput); + if (!parsedCreate.success) return writeRequestRefusal(deps, 'create', parsedCreate.error.issues, createInput); // Spec: returns CreateDataResponse = { object, id, record } - const result = await actionExec.callData(deps, _context, 'create', { object: objectName, data: body }, _context.dataDriver, _context.environmentId, _context.executionContext); + const result = await actionExec.callData(deps, _context, 'create', createInput, _context.dataDriver, _context.environmentId, _context.executionContext); const res = deps.success(result); res.status = 201; return { handled: true, response: res };