diff --git a/.changeset/23462-dispatcher-patch-expected-version.md b/.changeset/23462-dispatcher-patch-expected-version.md new file mode 100644 index 00000000000..72caebefcb4 --- /dev/null +++ b/.changeset/23462-dispatcher-patch-expected-version.md @@ -0,0 +1,21 @@ +--- +'@objectstack/runtime': minor +--- + +fix(runtime)!: the runtime dispatcher's `PATCH /data/:object/:id` honours a body `expectedVersion` and `If-Match` as its `RestServer` twin does, so an update carrying a stale version is refused instead of applied + +`RestServer`'s `PATCH /data/:object/:id` reads an optimistic-concurrency token from the body's `expectedVersion`, else from the `If-Match` header. It strips `expectedVersion` out of the patch and hands the token to the protocol's `updateData`, which refuses an update whose record has moved past it. The runtime dispatcher's `/data` domain read neither and handed the whole body over as the patch, and its shared `callData` update arm forwarded no token. So on a host whose `/data` routes the dispatcher serves (a `createHonoApp` host), an update carrying a stale `If-Match` wrote the record and answered `200`, the same answer an update carrying no token gets. A body `expectedVersion` reached the engine as a field and was refused as an unknown one. The domain now reads the token the way `RestServer` does, and `callData` forwards it to `updateData`. + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. + +What changes for you, wherever the runtime dispatcher serves `PATCH /data/:object/:id`. Where `@objectstack/rest` is mounted it serves this route itself and already answered each of these, so the standalone server's answers do not move. + +- FROM an update with a stale `If-Match` that wrote the record and answered `200`, TO a refusal with the record unchanged: the protocol's `409 CONCURRENT_UPDATE`, thrown out of `dispatch()` for the host's exit to render, as every other refusal `updateData` throws is. A `@objectstack/hono` `createHonoApp` host still renders that refusal as `500` (the record is unchanged): its catch-all reads only an error's `statusCode`, and rendering the declared `409` is a separate fix. +- FROM a body `expectedVersion` that reached the engine as a field and was refused as unknown (`Unknown field 'expectedVersion'`, nothing written), TO the update's version token. It is stripped from the patch and never written as a field. The update is applied when the token names the current version and refused as above when it is stale. A body `expectedVersion` is read before `If-Match`, in `RestServer`'s order. +- FROM an update carrying a token on a host whose protocol has no `updateData` (the ObjectQL fallback), which updated unguarded, TO `501 NOT_IMPLEMENTED`, before anything is read or written. An update with no token reaches the fallback as before. + +To update over a version you read, send that version (the record's `updated_at`) as the body's `expectedVersion` or as `If-Match`. An update with no token is unchanged. The `@objectstack/client` SDK's `data.update(object, id, data)` sends `data` as the body, so an SDK caller whose `data` carries `expectedVersion` now gets, through the dispatcher, the guard it asked for. diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index f5614a8ba11..de6cf91945e 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -364,8 +364,34 @@ export async function callData(deps: ActionExecutionDeps, } if (action === 'update') { + // [#23462] `expectedVersion` rides through to `updateData`, whose + // version check refuses a stale update. This arm forwarded only + // `{ object, id, data }`, so a token the `/data` domain read was + // dropped here a second time. Present only when a caller sent one: + // every other caller hands `{ object, id, data }` and reaches + // `updateData` as before. The `delete` arm below does the same. + const expectedVersion = params?.expectedVersion; if (protocol && typeof protocol.updateData === 'function') { - return await protocol.updateData({ object: params.object, id: params.id, data: params.data, ...(scopeId ? { environmentId: scopeId } : {}), context: executionContext }); + return await protocol.updateData({ + object: params.object, + id: params.id, + data: params.data, + ...(expectedVersion !== undefined ? { expectedVersion } : {}), + ...(scopeId ? { environmentId: scopeId } : {}), + context: executionContext, + }); + } + // [#23462] The fallback below updates by id and nothing else, so it + // cannot judge a version. A caller who sent one is refused `501` + // before anything is read, never updated unguarded: that is the very + // drop the arm above just closed. The `delete` arm's fallback refuses + // it the same way. + if (expectedVersion !== undefined) { + throw { + statusCode: 501, + message: 'This host has no protocol updateData, so it cannot honour expectedVersion or If-Match on an ' + + 'update. Send the update without them, or update through a host whose protocol implements updateData.', + }; } if (ql && params.id && typeof ql.update === 'function') { let all = await ql.find(params.object, findOpts({ where: { id: params.id }, limit: 1 })); diff --git a/packages/runtime/src/domains/data-update-expected-version.test.ts b/packages/runtime/src/domains/data-update-expected-version.test.ts new file mode 100644 index 00000000000..8d0339f727b --- /dev/null +++ b/packages/runtime/src/domains/data-update-expected-version.test.ts @@ -0,0 +1,373 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23462] Both doors of `PATCH /data/:object/:id` honour the caller's + * optimistic-concurrency token alike: `RestServer`'s route and the runtime + * dispatcher's `/data` domain, the only answer on a host that mounts just + * `@objectstack/hono`'s `${prefix}/*` catch-all. The `DELETE` position of the + * same family is pinned in `data-delete-expected-version.test.ts` (#23265); + * this file is its `PATCH` sibling, on the same rig. + * + * ## What was wrong + * + * `RestServer` reads the body's `expectedVersion`, else `If-Match`, strips the + * key out of the patch, and hands the token to `updateData`, which refuses an + * update whose version moved (`409 CONCURRENT_UPDATE`). The dispatcher's + * `PATCH` branch read neither and handed the whole body over as the patch, and + * `callData`'s update arm forwarded no token. So through the catch-all a stale + * `If-Match` update was APPLIED with a `200`, and a body `expectedVersion` + * reached the engine as a field (refused as an unknown field, `500`). These + * rows hold the two doors to the same answer over the same cases, and read the + * store, not the door's word, after every request. + * + * ## The rig + * + * Each door gets its own REAL store: better-sqlite3 `:memory:`, the real + * `ObjectQL` engine and a real `ObjectStackProtocolImplementation`, so a + * conflict is the protocol's own version check, not a double's. + * + * - `RestServer`: its registered `PATCH` handler, called with the request + * the `plugin-hono-server` adapter builds (a lowercased header record and + * the parsed JSON body), the auth boundary (`resolveExecCtx`) stubbed; + * - the catch-all: {@link catchAll} repeats the catch-all's statements over a + * real Fetch `Request` into the real `HttpDispatcher.dispatch()`: the body + * is parsed from a clone of the request's text (an unparsable body is + * `{}`), and the raw `Request` rides as `context.request`, which is where an + * `If-Match` is read. The identity step is the one stubbed seam. + * + * ## How a refusal is read + * + * The conflict is not returned by `dispatch()`: `/data` has no local catch, so + * it throws the protocol's `ConcurrentUpdateError` and the host's exit renders + * it. Its status and code are read here through `resolveThrownHttpError` + * (`@objectstack/types`), the one rule the dispatcher's exits answer a throw + * with, and set beside what `RestServer` answered through `mapDataError`. The + * parity asserted is status, `code` and the store, never the envelope's bytes. + */ + +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' }, + }, +}; + +/** `r1`'s name before any request; every applied patch below writes {@link AFTER}. */ +const BEFORE = 'first'; +const AFTER = 'second'; + +/** A well-formed instant no row in these stores carries: a token read before a write that moved it. */ +const STALE = '2000-01-01T00:00:00.000Z'; + +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 */ } + } +}); + +/** One real store with one `task` row, `r1`, named {@link BEFORE}, whose current version is `version`. */ +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 row: any = await engine.findOne('task', { where: { id: 'r1' } }); + const version = row?.updated_at instanceof Date ? row.updated_at.toISOString() : String(row?.updated_at ?? ''); + if (!version) throw new Error('the store minted no updated_at, so no version token exists to pin'); + const updateData = vi.spyOn(protocol, 'updateData'); + /** `r1`'s `name` in the store now. */ + const name = async () => ((await engine.findOne('task', { where: { id: 'r1' } })) as any)?.name; + return { protocol, version, name, updateData }; +} + +// ── The two doors ───────────────────────────────────────────────────────────── + +interface Answer { status: number; code?: string; message?: string } +interface PatchOptions { headers?: Record; body?: unknown } +/** What one `updateData` call received. */ +interface Forwarded { expectedVersion: string | undefined; data: unknown } +interface Door { + version: string; + patch(opts?: PatchOptions): Promise; + name(): Promise; + forwarded(): Forwarded[]; +} + +/** 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; +const messageOf = (body: any): string | undefined => + body?.error && typeof body.error === 'object' ? body.error.message : body?.error; + +const forwardedBy = (updateData: { mock: { calls: any[][] } }) => (): Forwarded[] => + updateData.mock.calls.map((c) => ({ expectedVersion: c[0]?.expectedVersion, data: c[0]?.data })); + +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, version, name, updateData } = 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 route = rest.getRoutes().find((r: any) => r.method === 'PATCH' && r.path === `${DATA}/:object/:id`); + if (!route) throw new Error(`PATCH ${DATA}/:object/:id is not registered`); + return { + version, + name, + forwarded: forwardedBy(updateData), + patch: async ({ headers = {}, body } = {}) => { + const res = makeRes(); + await route.handler({ + method: 'PATCH', path: `${DATA}/task/r1`, params: { object: 'task', id: 'r1' }, + // The adapter's header record is lowercased (`c.req.header()`). + headers: Object.fromEntries(Object.entries(headers).map(([k, v]) => [k.toLowerCase(), v])), + query: {}, + body: clone(body), + }, res); + return { status: res.statusCode, code: codeOf(res.body), message: messageOf(res.body) }; + }, + }; +} + +/** + * What `createHonoApp`'s `${prefix}/*` catch-all hands `dispatch()` for a + * `PATCH`: the path below the prefix, the JSON body parsed from a CLONE of the + * request's text (any failure is `{}`, the adapter's `readDispatchBody`), the + * query flattened from the URL, and the raw Fetch `Request` as + * `context.request`. + */ +async function catchAll(dispatcher: HttpDispatcher, raw: Request) { + const url = new URL(raw.url); + const subPath = url.pathname.substring(PREFIX.length); + let body: unknown; + try { + body = JSON.parse(await raw.clone().text()); + } catch { + body = {}; + } + const query: Record = {}; + url.searchParams.forEach((v, k) => { query[k] = v; }); + return dispatcher.dispatch(raw.method, subPath, body, query, { request: raw } as any, PREFIX); +} + +function dispatcherOver(services: Record): HttpDispatcher { + 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 dispatcher; +} + +const patchRequest = ({ headers = {}, body }: PatchOptions = {}) => + new Request(`http://localhost${DATA}/task/r1`, { + method: 'PATCH', + headers: { 'content-type': 'application/json', ...headers }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + +/** A dispatch answer, returned or thrown, as status and code. */ +async function answerOf(run: () => Promise): Promise { + try { + const res = await run(); + const body = res.response?.body; + return { status: res.response?.status ?? 0, code: codeOf(body), message: messageOf(body) }; + } catch (e) { + const thrown = resolveThrownHttpError(e); + return { status: thrown.status, code: thrown.code, message: thrown.message }; + } +} + +async function bootCatchAll(): Promise { + const { protocol, version, name, updateData } = await bootStore(); + const dispatcher = dispatcherOver({ protocol }); + return { + version, + name, + forwarded: forwardedBy(updateData), + patch: (opts = {}) => answerOf(() => catchAll(dispatcher, patchRequest(opts))), + }; +} + +const DOORS: ReadonlyArray Promise]> = [ + ['RestServer', bootRestServer], + ['the @objectstack/hono catch-all', bootCatchAll], +]; + +const updated = (a: Answer) => expect(a.status, JSON.stringify(a)).toBe(200); +const conflict = (a: Answer) => + expect({ status: a.status, code: a.code }, JSON.stringify(a)).toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + +// ── The cases, on each door ─────────────────────────────────────────────────── + +for (const [doorName, boot] of DOORS) { + describe(`[#23462] ${doorName}: PATCH /data/:object/:id honours a body expectedVersion and If-Match`, () => { + it('control: a patch with no version updates, and hands updateData no token', async () => { + const door = await boot(); + updated(await door.patch({ body: { name: AFTER } })); + expect(await door.name()).toBe(AFTER); + expect(door.forwarded()).toEqual([{ expectedVersion: undefined, data: { name: AFTER } }]); + }, 60_000); + + it('a body expectedVersion naming the current version updates, and is never written as a field', async () => { + const door = await boot(); + updated(await door.patch({ body: { name: AFTER, expectedVersion: door.version } })); + expect(await door.name()).toBe(AFTER); + // The token rides beside the patch, never inside it. + expect(door.forwarded()).toEqual([{ expectedVersion: door.version, data: { name: AFTER } }]); + }, 60_000); + + it('a stale body expectedVersion is refused 409 CONCURRENT_UPDATE and the row is unchanged', async () => { + const door = await boot(); + conflict(await door.patch({ body: { name: AFTER, expectedVersion: STALE } })); + expect(await door.name()).toBe(BEFORE); + expect(door.forwarded()).toEqual([{ expectedVersion: STALE, data: { name: AFTER } }]); + }, 60_000); + + it('If-Match behaves like the body token: the current version updates, a stale one is refused', async () => { + const stale = await boot(); + conflict(await stale.patch({ body: { name: AFTER }, headers: { 'If-Match': `"${STALE}"` } })); + expect(await stale.name()).toBe(BEFORE); + + const current = await boot(); + updated(await current.patch({ body: { name: AFTER }, headers: { 'If-Match': `"${current.version}"` } })); + expect(await current.name()).toBe(AFTER); + }, 60_000); + + it('the body token is read before If-Match, so the body token is the one judged', async () => { + const staleBody = await boot(); + conflict(await staleBody.patch({ body: { name: AFTER, expectedVersion: STALE }, headers: { 'If-Match': staleBody.version } })); + expect(await staleBody.name()).toBe(BEFORE); + + const currentBody = await boot(); + updated(await currentBody.patch({ body: { name: AFTER, expectedVersion: currentBody.version }, headers: { 'If-Match': STALE } })); + expect(await currentBody.name()).toBe(AFTER); + }, 60_000); + }); +} + +// ── The same answers, side by side ──────────────────────────────────────────── + +/** + * The shapes a caller can put the token in beyond the plain string. Each door + * answers each one, and the two must agree on the status, the code and the + * 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 only when it is truthy. + */ +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 } })], + ['a body that is only the current token', (v) => ({ body: { expectedVersion: v } })], +]; + +describe('[#23462] the two doors answer each token shape with the same status, code and row', () => { + for (const [shape, opts] of EDGE_SHAPES) { + it(shape, async () => { + const seen: Array<{ status: number; code?: string; name?: string }> = []; + for (const [, boot] of DOORS) { + const door = await boot(); + const answer = await door.patch(opts(door.version)); + seen.push({ status: answer.status, code: answer.code, name: await door.name() }); + // Whatever the token's shape, the key never reaches the engine as a field. + for (const call of door.forwarded()) { + expect(call.data && typeof call.data === 'object' && 'expectedVersion' in call.data, JSON.stringify(call)).toBe(false); + } + } + const [rest, catchAllAnswer] = seen; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } + + it('a Node host\'s plain header record carries If-Match the way the Fetch Headers does', async () => { + const { protocol, name } = await bootStore(); + const dispatcher = dispatcherOver({ protocol }); + const answer = await answerOf(() => dispatcher.dispatch( + 'PATCH', '/data/task/r1', { name: AFTER }, {}, { request: { headers: { 'if-match': `"${STALE}"` } } } as any, PREFIX, + )); + conflict(answer); + expect(await name()).toBe(BEFORE); + }, 60_000); +}); + +// ── The dispatcher's fallback updater ───────────────────────────────────────── + +/** + * The dispatcher alone has a second updater: a host whose `protocol` brings no + * `updateData` falls to the ObjectQL engine's `update`, which takes no version. + * A caller who sent one is refused `501` before anything is read or written, + * never updated unguarded, which is the drop this card closes. + */ +describe('[#23462] the catch-all\'s fallback updater cannot judge a version, so sending one is refused', () => { + const fallbackHost = () => { + const objectql = { + find: vi.fn(async () => [{ id: 'r1', name: BEFORE }]), + update: vi.fn(async () => ({ id: 'r1', name: AFTER })), + }; + return { dispatcher: dispatcherOver({ protocol: {}, objectql }), objectql }; + }; + + for (const [asked, opts] of [ + ['a body expectedVersion', { body: { name: AFTER, expectedVersion: STALE } }], + ['If-Match', { body: { name: AFTER }, headers: { 'If-Match': `"${STALE}"` } }], + ] as const) { + it(`${asked}: 501 NOT_IMPLEMENTED, and the fallback neither reads nor writes`, async () => { + const { dispatcher, objectql } = fallbackHost(); + const answer = await answerOf(() => catchAll(dispatcher, patchRequest(opts as PatchOptions))); + expect({ status: answer.status, code: answer.code }).toEqual({ status: 501, code: 'NOT_IMPLEMENTED' }); + expect(objectql.find).not.toHaveBeenCalled(); + expect(objectql.update).not.toHaveBeenCalled(); + }); + } + + it('control: an unguarded update still reaches the fallback updater', async () => { + const { dispatcher, objectql } = fallbackHost(); + const answer = await answerOf(() => catchAll(dispatcher, patchRequest({ body: { name: AFTER } }))); + expect(answer.status).toBe(200); + expect(objectql.update).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 78de85f9f16..daba6c63ab3 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -162,8 +162,31 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // PATCH /data/:object/:id if (parts.length === 2 && m === 'PATCH') { const id = parts[1]; + // [#23462] The optimistic-concurrency token, read the way + // `RestServer`'s twin of this route reads it: the body's + // `expectedVersion`, else `If-Match`, and forwarded to `updateData`, + // which refuses an update whose version moved (`409 + // CONCURRENT_UPDATE`). This branch read neither and handed the + // whole body over as the patch, so through the `@objectstack/hono` + // catch-all a stale `If-Match` update was APPLIED, and a body + // `expectedVersion` reached the engine as a field it refused as + // unknown. The key is stripped from the patch whatever its value, + // and a token is forwarded only when truthy: the twin's own + // statements, so both doors send `updateData` the same thing. + const bodyVersion = (body && typeof body === 'object') ? body.expectedVersion : undefined; + const expectedVersion = bodyVersion ?? requestIfMatch(_context.request?.headers); + let data = body; + if (body && typeof body === 'object' && 'expectedVersion' in body) { + const { expectedVersion: _token, ...patch } = body; + data = patch; + } // Spec: returns UpdateDataResponse = { object, id, record } - const result = await actionExec.callData(deps, _context, 'update', { object: objectName, id, data: body }, _context.dataDriver, _context.environmentId, _context.executionContext); + const result = await actionExec.callData(deps, _context, 'update', { + object: objectName, + id, + data, + ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}), + }, _context.dataDriver, _context.environmentId, _context.executionContext); return { handled: true, response: deps.success(result) }; }