From 6b73612e41b31ed4c362b1f90f18b9b913d6041f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 05:56:39 +0000 Subject: [PATCH 1/8] fix(spec,rest,runtime): request bodies are checked against the schemas the catalog declares (#3899) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API catalog (plugin-rest-api.zod.ts) declared requestSchema on 16 routes while almost no mounted entry point validated its body — a malformed request did not 400, it executed different semantics (an unfiltered full read on /query, markRead(userId, []) on a misnamed key, a flow registered under the key `undefined`, a one-letter-off toggle body ENABLING the flow it meant to disable). Wire the declared schemas at the real entry points, make the catalog stop promising what nothing performs, and gate both directions: - rest: POST /data/:object/query, POST /data/:object, PATCH /data/:object/:id, POST /data/:object/batch and POST /data/:object/createMany now safeParse the declared contract and answer 400 VALIDATION_FAILED + fields[] (the #3933/#3944 pattern); the query route also pins the PATH object into the forwarded query. - runtime: POST /notifications/read validates MarkNotificationsReadRequestSchema; the automation registerFlow / updateFlow / toggle bodies get strict hand-written guards (keys.ts pattern); the analytics-entry validationFailure helper is hoisted to validation-failure.ts and shared. - spec: the catalog drops the four ghost notification endpoints (#3612 removed the routes), fixes the automation trigger path to /trigger/:name and drops its never-true request schema, adds the POST /:object/query entry, repoints create/update at the schemas the routes actually validate, and drops requestSchema from bodyless GET/DELETE entries. - gates: schema-name references must resolve to real exports and sit on body-carrying methods (spec); every declared requestSchema on a mounted route has a violating-body -> 400 conformance case with a completeness ratchet (rest + runtime request-schema-gate suites). Suites: spec 7132, rest 536, runtime 977 all green; check:generated 8/8. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- .changeset/request-body-schema-validation.md | 29 +++ .../request-schema-gate.conformance.test.ts | 213 ++++++++++++++++++ packages/rest/src/rest-server.ts | 126 ++++++++++- packages/runtime/src/domains/analytics.ts | 24 +- .../automation-body-validation.test.ts | 155 +++++++++++++ packages/runtime/src/domains/automation.ts | 66 +++++- packages/runtime/src/domains/notifications.ts | 21 +- .../request-schema-gate.conformance.test.ts | 152 +++++++++++++ packages/runtime/src/validation-failure.ts | 32 +++ .../api/plugin-rest-api.schema-refs.test.ts | 104 +++++++++ packages/spec/src/api/plugin-rest-api.test.ts | 22 +- packages/spec/src/api/plugin-rest-api.zod.ts | 140 +++++++----- 12 files changed, 982 insertions(+), 102 deletions(-) create mode 100644 .changeset/request-body-schema-validation.md create mode 100644 packages/rest/src/request-schema-gate.conformance.test.ts create mode 100644 packages/runtime/src/domains/automation-body-validation.test.ts create mode 100644 packages/runtime/src/request-schema-gate.conformance.test.ts create mode 100644 packages/spec/src/api/plugin-rest-api.schema-refs.test.ts diff --git a/.changeset/request-body-schema-validation.md b/.changeset/request-body-schema-validation.md new file mode 100644 index 0000000000..3b353b7b34 --- /dev/null +++ b/.changeset/request-body-schema-validation.md @@ -0,0 +1,29 @@ +--- +'@objectstack/spec': patch +'@objectstack/rest': minor +'@objectstack/runtime': minor +--- + +Request bodies are now checked against the schemas the API catalog declares for them (#3899, the request-side dual of #3877). + +**Routes that now answer `400 VALIDATION_FAILED` + `fields[]` for a body violating their declared `requestSchema`** (previously the body was consumed raw, and a malformed one silently executed different semantics): + +- `POST /data/:object/query` — body must be a QueryAST (`FindDataRequestSchema`); a garbage body used to degrade into an unfiltered full read. The path `object` is now pinned into the forwarded query (a body `object` can no longer contradict the path). +- `POST /data/:object` / `PATCH /data/:object/:id` — body must be a record object (`CreateDataRequestSchema` / `UpdateDataRequestSchema`). +- `POST /data/:object/batch` — body must be a `BatchUpdateRequestSchema` (`operation` + `records[]`). +- `POST /data/:object/createMany` — body must be a bare JSON array of records (`CreateManyDataRequestSchema`); `{ records: [...] }` (updateMany's envelope) is rejected with a pointer. +- `POST /notifications/read` — body must be `{ ids: string[] }` (`MarkNotificationsReadRequestSchema`); a misnamed key used to become `markRead(userId, [])` — a 200 no-op that never cleared the badge. + +**Dispatcher automation routes now validate their bodies** (no catalog schema; hand-written guards): + +- `POST /automation` and `PUT /automation/:name` require a flow-definition object, and POST requires a non-empty `name` — a mistyped `name` used to register the flow under the key `undefined` and echo 200. +- `POST /automation/:name/toggle` is strictly `{ enabled?: boolean }` — `{"enable": false}` (one letter off) used to ENABLE the flow and answer 200 `{enabled: true}`; it is now a 400 naming the offending key. An empty body still means enable. + +**Catalog corrections in `@objectstack/spec` (`plugin-rest-api.zod.ts`)** — documentation-only tables, no schema exports changed: + +- `DEFAULT_NOTIFICATION_ROUTES` drops the four device/preferences endpoints — those server routes were removed in #3612 (never built), yet the table kept declaring them, `requestSchema` and all. +- `DEFAULT_AUTOMATION_ROUTES`' trigger endpoint path is corrected `/trigger` → `/trigger/:name` (the mounted path; the flow name rides the path) and its `AutomationTriggerRequestSchema` declaration is removed — that schema never described this route's wire shape. +- `DEFAULT_DATA_CRUD_ROUTES` gains the `POST /:object/query` entry (mounted since forever, previously undeclared), repoints create/update to the schemas the routes actually validate (`CreateDataRequestSchema` / `UpdateDataRequestSchema` — the old `CreateRequestSchema`/`UpdateRequestSchema` names described a `{ data }` envelope the wire never had), and drops `requestSchema` from GET/DELETE entries (path/query-bound inputs; nothing can violate them as a body). +- New gates: catalog `requestSchema`/`responseSchema` strings must resolve to real exported Zod schemas, `requestSchema` may only sit on body-carrying methods, and every declared `requestSchema` on a mounted route has a violating-body → 400 conformance case (`packages/rest` + `packages/runtime` request-schema-gate suites). + +Migration: clients that already send the documented shapes are unaffected. If you relied on a malformed body being silently accepted (e.g. posting `{ records: [...] }` to `createMany`, a non-boolean `enabled` to toggle, or an off-schema analytics/query body), fix the request to the declared shape — the 400's `fields[]` names each offending key. diff --git a/packages/rest/src/request-schema-gate.conformance.test.ts b/packages/rest/src/request-schema-gate.conformance.test.ts new file mode 100644 index 0000000000..da93db5e50 --- /dev/null +++ b/packages/rest/src/request-schema-gate.conformance.test.ts @@ -0,0 +1,213 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3899 ④ — the request-side gate, REST half: every route the catalog + * (`plugin-rest-api.zod.ts`) declares a `requestSchema` for and + * `@objectstack/rest` serves must answer a violating body with + * `400 VALIDATION_FAILED` + `fields[]`, with the protocol NEVER invoked. + * + * The catalog is documentation with no runtime consumer, which is exactly how + * its promises drifted from the mounts (#3899 ①): `requestSchema` strings sat + * on routes that read `req.body || {}` raw, so a malformed body did not 400 — + * it executed different semantics (an unfiltered full read on `/query`, a + * garbage row reaching the engine on `/createMany`). These cases drive the + * REAL registered handlers, and the completeness test is the ratchet: a new + * catalog `requestSchema` on a rest-served route with no violating-body case + * here fails the suite. + * + * Siblings: `packages/runtime/src/request-schema-gate.conformance.test.ts` + * (dispatcher-served groups) and + * `packages/spec/src/api/plugin-rest-api.schema-refs.test.ts` (names resolve; + * requestSchema only on body-carrying methods). + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + DEFAULT_DATA_CRUD_ROUTES, + DEFAULT_BATCH_ROUTES, + DEFAULT_METADATA_ROUTES, +} from '@objectstack/spec/api'; +import { RestServer } from './rest-server'; + +const BODY_METHODS = new Set(['POST', 'PUT', 'PATCH']); + +/** The catalog groups whose mounted routes @objectstack/rest serves. */ +const REST_GROUPS = [DEFAULT_DATA_CRUD_ROUTES, DEFAULT_BATCH_ROUTES, DEFAULT_METADATA_ROUTES]; + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); + return res; +} + +function setup() { + const spies = { + findData: vi.fn().mockResolvedValue({ object: 'invoice', records: [] }), + createData: vi.fn().mockResolvedValue({ object: 'invoice', id: 'r1', record: {} }), + updateData: vi.fn().mockResolvedValue({ object: 'invoice', id: 'r1', record: {} }), + batchData: vi.fn().mockResolvedValue({ success: true, total: 0, succeeded: 0, failed: 0, results: [] }), + createManyData: vi.fn().mockResolvedValue({ object: 'invoice', records: [], count: 0 }), + updateManyData: vi.fn().mockResolvedValue({ success: true, total: 0, succeeded: 0, failed: 0, results: [] }), + deleteManyData: vi.fn().mockResolvedValue({ success: true, total: 0, succeeded: 0, failed: 0, results: [] }), + }; + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: { data: '', metadata: '', ui: '', auth: '/auth' } }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([{ name: 'invoice' }]), + getMetaItem: vi.fn().mockResolvedValue({}), + ...spies, + }; + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + return { rest, ...spies }; +} + +async function drive(rest: any, method: string, path: string, body: unknown, params: Record) { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`route not registered: ${method} ${path}`); + const res = makeRes(); + await route.handler({ method, params, query: {}, headers: {}, body }, res); + return res; +} + +interface GateCase { + /** `METHOD prefix+path` exactly as the catalog spells it. */ + route: string; + params: Record; + violating: unknown[]; + valid: unknown; + spy: keyof ReturnType & string; +} + +const PARAMS = { object: 'invoice' }; +const PARAMS_ID = { object: 'invoice', id: 'r1' }; + +/** + * One case per rest-served catalog route with a `requestSchema`. The + * completeness test below forces this table to keep up with the catalog. + */ +const CASES: GateCase[] = [ + { + route: 'POST /api/v1/data/:object/query', + params: PARAMS, + violating: [ + 'garbage', // non-object body — used to become `query: 'garbage'` + [{ limit: 5 }], // array body + { limit: 'ten' }, // mistyped clause — used to be forwarded as-is + { joins: [] }, // retired key (#4286) — the tombstone must be audible + ], + valid: { where: { status: 'active' }, limit: 5 }, + spy: 'findData', + }, + { + route: 'POST /api/v1/data/:object', + params: PARAMS, + violating: ['garbage', 42, [{ name: 'x' }]], + valid: { name: 'ACME' }, + spy: 'createData', + }, + { + route: 'PATCH /api/v1/data/:object/:id', + params: PARAMS_ID, + violating: ['garbage', 42, [{ name: 'x' }]], + valid: { name: 'ACME (renamed)' }, + spy: 'updateData', + }, + { + route: 'POST /api/v1/data/:object/batch', + params: PARAMS, + violating: [ + {}, // operation + records missing + { operation: 'frobnicate', records: [] }, // not a batch operation + { operation: 'update' }, // records missing + { operation: 'update', records: {} }, // records not an array + ], + valid: { operation: 'update', records: [{ id: 'r1', data: { name: 'z' } }] }, + spy: 'batchData', + }, + { + route: 'POST /api/v1/data/:object/createMany', + params: PARAMS, + violating: [ + { records: [{ name: 'a' }] }, // updateMany's envelope — an easy cross-route slip + { name: 'a' }, // a single record instead of an array + 'garbage', + ], + valid: [{ name: 'a' }, { name: 'b' }], + spy: 'createManyData', + }, + { + route: 'POST /api/v1/data/:object/updateMany', + params: PARAMS, + violating: [{}, { records: [{}] }, { records: [{ id: 'r1' }] }], + valid: { records: [{ id: 'r1', data: { name: 'z' } }] }, + spy: 'updateManyData', + }, + { + route: 'POST /api/v1/data/:object/deleteMany', + params: PARAMS, + violating: [{}, { ids: 'r1' }, { ids: [{ id: 'r1' }] }], + valid: { ids: ['r1', 'r2'] }, + spy: 'deleteManyData', + }, +]; + +describe('#3899 ④ — rest routes with a declared requestSchema reject violating bodies', () => { + it('every rest-served catalog requestSchema has a violating-body case (ratchet)', () => { + const covered = new Set(CASES.map((c) => c.route)); + for (const group of REST_GROUPS) { + for (const ep of group.endpoints ?? []) { + if (!ep.requestSchema || !BODY_METHODS.has(ep.method)) continue; + const route = `${ep.method} ${group.prefix}${ep.path}`; + expect( + covered.has(route), + `${route} declares requestSchema '${ep.requestSchema}' but has no violating-body case in this suite — add one, or the declaration is a promise nothing checks`, + ).toBe(true); + } + } + }); + + for (const gateCase of CASES) { + const [method, path] = gateCase.route.split(' ') as [string, string]; + describe(gateCase.route, () => { + it('answers 400 VALIDATION_FAILED + fields[], protocol never called', async () => { + for (const body of gateCase.violating) { + const ctx = setup(); + const res = await drive(ctx.rest, method, path, body, gateCase.params); + expect( + res.statusCode, + `${gateCase.route} accepted ${JSON.stringify(body)} (status ${res.statusCode}: ${JSON.stringify(res.body)})`, + ).toBe(400); + expect(res.body).toMatchObject({ code: 'VALIDATION_FAILED' }); + expect(Array.isArray(res.body.fields)).toBe(true); + expect(res.body.fields.length).toBeGreaterThan(0); + expect((ctx as any)[gateCase.spy]).not.toHaveBeenCalled(); + } + }); + + it('still serves a conforming body (the 400 above is the schema, not a broken route)', async () => { + const ctx = setup(); + const res = await drive(ctx.rest, method, path, gateCase.valid, gateCase.params); + expect( + [200, 201].includes(res.statusCode), + `expected 2xx, got ${res.statusCode}: ${JSON.stringify(res.body)}`, + ).toBe(true); + expect((ctx as any)[gateCase.spy]).toHaveBeenCalledTimes(1); + }); + }); + } +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 33ff4f3d9d..ee4e6fb526 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -3961,9 +3961,28 @@ export class RestServer { const context = await this.resolveExecCtx(environmentId, req); if (this.enforceAuth(req, res, context)) return; if (await this.enforceApiAccess(req, res, p, environmentId, 'create')) return; + // [#3899] The wire body IS the record. Validate the + // assembled protocol request (`CreateDataRequestSchema`, + // catalog `requestSchema`) so a non-record body — an + // array, a string, a number — answers 400 instead of + // reaching the engine as `data`. Per-field checks stay + // downstream (object metadata / validation rules); this + // gate is about the SHAPE the contract declares. + const { CreateDataRequestSchema } = await import('@objectstack/spec/api'); + const createInput = { object: req.params.object, data: req.body ?? {} }; + const parsedCreate = (CreateDataRequestSchema as any).safeParse(createInput); + if (!parsedCreate.success) { + res.status(400).json({ + error: 'Invalid create request', + code: 'VALIDATION_FAILED', + fields: zodIssuesToFields(parsedCreate.error?.issues, createInput), + object: req.params?.object, + }); + return; + } const result = await p.createData({ object: req.params.object, - data: req.body, + data: req.body ?? {}, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), } as any); @@ -4000,9 +4019,36 @@ export class RestServer { const context = await this.resolveExecCtx(environmentId, req); if (this.enforceAuth(req, res, context)) return; if (await this.enforceApiAccess(req, res, p, environmentId, 'list')) return; + // [#3899] Validate the QueryAST body against the declared + // contract (`FindDataRequestSchema`, catalog `requestSchema`). + // A malformed body used to be forwarded as-is — and since a + // dropped/mistyped clause does not narrow a query, it WIDENS + // it, `{"filter": …}` degraded into an unfiltered full read + // with a 200. The PATH object is written last (#3946) so a + // body `object` can neither dodge the `enforceApiAccess` + // gate above nor move the read. Validation only: the merged + // ORIGINAL body is forwarded, not the parse output, so the + // schema cannot inject defaults the engine did not receive + // before (the analytics-entry precedent, #3878). + const { FindDataRequestSchema } = await import('@objectstack/spec/api'); + const rawQuery = req.body ?? {}; + const query = (rawQuery && typeof rawQuery === 'object' && !Array.isArray(rawQuery)) + ? { ...rawQuery, object: req.params.object } + : rawQuery; // non-object bodies go to the schema as-is and fail with `query: invalid_type` + const findInput = { object: req.params.object, query }; + const parsedFind = (FindDataRequestSchema as any).safeParse(findInput); + if (!parsedFind.success) { + res.status(400).json({ + error: 'Invalid query request', + code: 'VALIDATION_FAILED', + fields: zodIssuesToFields(parsedFind.error?.issues, findInput), + object: req.params?.object, + }); + return; + } const result = await p.findData({ object: req.params.object, - query: req.body || {}, + query, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), } as any); @@ -4052,10 +4098,32 @@ export class RestServer { data = rest; } if (await this.enforceApiAccess(req, res, p, environmentId, 'update')) return; + // [#3899] Same gate as create: the wire body is the bare + // field patch (`expectedVersion` already stripped above), + // validated as the assembled `UpdateDataRequestSchema` + // request so a non-record body 400s instead of reaching + // the engine. + const { UpdateDataRequestSchema } = await import('@objectstack/spec/api'); + const updateInput = { + object: req.params.object, + id: req.params.id, + data: data ?? {}, + ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}), + }; + const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput); + if (!parsedUpdateOne.success) { + res.status(400).json({ + error: 'Invalid update request', + code: 'VALIDATION_FAILED', + fields: zodIssuesToFields(parsedUpdateOne.error?.issues, updateInput), + object: req.params?.object, + }); + return; + } const result = await p.updateData({ object: req.params.object, id: req.params.id, - data, + data: data ?? {}, ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}), ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), @@ -7166,11 +7234,29 @@ export class RestServer { // [#3391] bulk ∧ child(body.operation) — the object must grant // the `bulk` primitive AND the batched write kind. if (await this.enforceApiAccess(req, res, p, environmentId, 'bulk', { bulkChild: req.body?.operation })) return; - // [#3939] `BatchUpdateRequestSchema` declared `max(200)`, - // but this route hands the body straight to the protocol - // without validating it — so nothing enforced the bound. - if (Array.isArray(req.body?.records) - && this.enforceBatchSize(res, req.body.records.length, maxBatch, req.params?.object)) return; + // [#3899] Validate against the declared contract + // (`BatchUpdateRequestSchema`, catalog `requestSchema`) — + // this route used to hand the body straight to the + // protocol, so `{ operation: 'updat', records: {} }` + // reached the engine as-is. Validation only: the ORIGINAL + // body is forwarded, not the parse output, so + // `BatchOptionsSchema`'s defaults (e.g. `atomic: true`) + // are not injected into a request that never sent them. + const { BatchUpdateRequestSchema } = await import('@objectstack/spec/api'); + const batchInput = req.body ?? {}; + const parsedBatch = (BatchUpdateRequestSchema as any).safeParse(batchInput); + if (!parsedBatch.success) { + res.status(400).json({ + error: 'Invalid batch request', + code: 'VALIDATION_FAILED', + fields: zodIssuesToFields(parsedBatch.error?.issues, batchInput), + object: req.params?.object, + }); + return; + } + // [#3939] Cap AFTER the shape check, so a caller gets the + // more specific answer first. + if (this.enforceBatchSize(res, parsedBatch.data.records.length, maxBatch, req.params?.object)) return; const result = await p.batchData!({ object: req.params.object, request: req.body, @@ -7203,9 +7289,27 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; // [#3391] bulk ∧ create — createMany requires the `bulk` primitive. if (await this.enforceApiAccess(req, res, p, environmentId, 'bulk', { bulkChild: 'create' })) return; - // [#3939] Body IS the records array on this route. - if (Array.isArray(req.body) - && this.enforceBatchSize(res, req.body.length, maxBatch, req.params?.object)) return; + // [#3899] Body IS the records array on this route (what + // `client.data.createMany` posts). Validate the assembled + // protocol request (`CreateManyDataRequestSchema`, catalog + // `requestSchema`) so `{ records: [...] }` — updateMany's + // envelope, an easy cross-route slip — or any other + // non-array body 400s instead of reaching the engine as a + // single garbage "record". + const { CreateManyDataRequestSchema } = await import('@objectstack/spec/api'); + const createManyInput = { object: req.params.object, records: req.body ?? [] }; + const parsedCreateMany = (CreateManyDataRequestSchema as any).safeParse(createManyInput); + if (!parsedCreateMany.success) { + res.status(400).json({ + error: 'Invalid createMany request — the body must be a JSON array of record objects', + code: 'VALIDATION_FAILED', + fields: zodIssuesToFields(parsedCreateMany.error?.issues, createManyInput), + object: req.params?.object, + }); + return; + } + // [#3939] Cap AFTER the shape check. + if (this.enforceBatchSize(res, parsedCreateMany.data.records.length, maxBatch, req.params?.object)) return; const result = await p.createManyData!({ object: req.params.object, records: req.body || [], diff --git a/packages/runtime/src/domains/analytics.ts b/packages/runtime/src/domains/analytics.ts index 6e05dcb061..402c6da84c 100644 --- a/packages/runtime/src/domains/analytics.ts +++ b/packages/runtime/src/domains/analytics.ts @@ -15,22 +15,14 @@ import { CoreServiceName } from '@objectstack/spec/system'; import { AnalyticsQueryRequestSchema } from '@objectstack/spec/api'; import { isServiceServeable } from '../service-serveable.js'; +import { validationFailure, fieldsFromZodIssues } from '../validation-failure.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; -/** - * [#3878] The duck-typed validation-failure shape both dispatcher error exits - * already map to 400 `VALIDATION_FAILED` + `details.fields[]` (see - * `validation-failure.ts`, #3918) — thrown here so entry validation needs no - * new error channel and no dependency on objectql's `ValidationError` class. - */ -function validationFailure(message: string, fields: unknown[]): Error { - const err = new Error(message) as Error & { code: string; fields: unknown[] }; - err.name = 'ValidationError'; - err.code = 'VALIDATION_FAILED'; - err.fields = fields; - return err; -} +// [#3878] `validationFailure` — the duck-typed shape both dispatcher error +// exits map to 400 `VALIDATION_FAILED` + `details.fields[]` (#3918) — began +// life inline here; it lives in `../validation-failure.js` now, next to the +// recogniser, shared with the notifications/automation entry gates (#3899). /** * [#3878] Reject a malformed `AnalyticsQuery` body AT THE ENTRY with a 400 @@ -61,11 +53,7 @@ function assertAnalyticsQueryBody(body: unknown): void { } const parsed = AnalyticsQueryRequestSchema.safeParse(body); if (!parsed.success) { - const fields = parsed.error.issues.map((issue) => ({ - field: issue.path.length > 0 ? issue.path.join('.') : '(body)', - code: issue.code, - message: issue.message, - })); + const fields = fieldsFromZodIssues(parsed.error.issues); throw validationFailure( `Invalid AnalyticsQuery body: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`, fields, diff --git a/packages/runtime/src/domains/automation-body-validation.test.ts b/packages/runtime/src/domains/automation-body-validation.test.ts new file mode 100644 index 0000000000..8dcd529732 --- /dev/null +++ b/packages/runtime/src/domains/automation-body-validation.test.ts @@ -0,0 +1,155 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3899 ③ — the automation domain's silent-semantics bodies. + * + * Neither route carries a catalog `requestSchema` (the flow name rides the + * path; the definitions have no wire schema), so they sit outside the + * request-schema gate — but they were two of the issue's four worked examples + * of the failure mode the gate exists for: a malformed body that does not 400 + * but EXECUTES DIFFERENT SEMANTICS. + * + * - `POST /automation` ran `registerFlow(body?.name, body)`, so a definition + * with a missing/mistyped `name` registered under the key `undefined` — + * 200, body echoed, flow unreachable forever. + * - `POST /automation/:name/toggle` read `body?.enabled ?? true`, so + * `{"enable": false}` — one letter off from "switch it OFF" — switched the + * flow ON and answered 200 `{enabled: true}`. + * + * The thrown shape here is the same duck-typed validation failure both HTTP + * error exits map to `400 VALIDATION_FAILED` (`dispatcher-validation-error + * .test.ts`, #3918). + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import { validationFailureDetails } from '../validation-failure.js'; + +function makeDispatcher() { + const spies = { + registerFlow: vi.fn(), + toggleFlow: vi.fn(async () => undefined), + execute: vi.fn(async () => ({ success: true })), + }; + const services: Record = { automation: spies }; + const resolve = (name: string) => services[name]; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + return { dispatcher: new HttpDispatcher(kernel), spies }; +} + +const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any; + +async function expectValidationFailure(run: Promise, label: string) { + let thrown: unknown; + try { + await run; + } catch (e) { + thrown = e; + } + expect(thrown, `${label} was accepted instead of rejected`).toBeDefined(); + const details = validationFailureDetails(thrown); + expect(details?.code).toBe('VALIDATION_FAILED'); + expect(details?.fields.length).toBeGreaterThan(0); + return thrown as Error; +} + +describe('#3899 ③ — POST /automation (registerFlow) requires a named definition', () => { + it.each([ + ['missing name', { trigger: { type: 'manual' } }], + ['empty name', { name: ' ' }], + ['non-string name', { name: 42 }], + ['array body', [{ name: 'f1' }]], + ['string body', 'garbage'], + ['null body', null], + ])('rejects %s — a flow must never register under the key `undefined`', async (_label, body) => { + const { dispatcher, spies } = makeDispatcher(); + await expectValidationFailure( + dispatcher.handleAutomation('', 'POST', body, CTX), + JSON.stringify(body), + ); + expect(spies.registerFlow).not.toHaveBeenCalled(); + }); + + it('registers a named definition under its name', async () => { + const { dispatcher, spies } = makeDispatcher(); + const body = { name: 'welcome_flow', trigger: { type: 'manual' } }; + const result = await dispatcher.handleAutomation('', 'POST', body, CTX); + expect(result.response?.status).toBe(200); + expect(spies.registerFlow).toHaveBeenCalledWith('welcome_flow', body); + }); +}); + +describe('#3899 ③ — POST /automation/:name/toggle is { enabled?: boolean }, strictly', () => { + it('rejects the one-letter-off key that used to ENABLE a flow being switched off', async () => { + const { dispatcher, spies } = makeDispatcher(); + const thrown = await expectValidationFailure( + dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enable: false }, CTX), + '{"enable": false}', + ); + // The rejection names the offending key — the caller sent `enable`. + expect(validationFailureDetails(thrown)?.fields).toMatchObject([{ field: 'enable' }]); + expect(spies.toggleFlow).not.toHaveBeenCalled(); + }); + + it.each([ + ['string enabled', { enabled: 'false' }], + ['numeric enabled', { enabled: 0 }], + ['array body', [true]], + ])('rejects %s — a truthy non-boolean must not silently enable', async (_label, body) => { + const { dispatcher, spies } = makeDispatcher(); + await expectValidationFailure( + dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', body, CTX), + JSON.stringify(body), + ); + expect(spies.toggleFlow).not.toHaveBeenCalled(); + }); + + it('honours an explicit disable', async () => { + const { dispatcher, spies } = makeDispatcher(); + const result = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enabled: false }, CTX); + expect(result.response?.status).toBe(200); + expect(result.response?.body?.data ?? result.response?.body).toMatchObject({ enabled: false }); + expect(spies.toggleFlow).toHaveBeenCalledWith('welcome_flow', false); + }); + + it('keeps the documented legacy shape: an empty body enables', async () => { + const { dispatcher, spies } = makeDispatcher(); + const result = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', undefined, CTX); + expect(result.response?.status).toBe(200); + expect(spies.toggleFlow).toHaveBeenCalledWith('welcome_flow', true); + }); +}); + +describe('#3899 ③ — PUT /automation/:name requires a definition object', () => { + it.each([ + ['string body', 'garbage'], + ['array body', [{}]], + ['null body', null], + ['array definition', { definition: ['not-a-flow'] }], + ])('rejects %s', async (_label, body) => { + const { dispatcher, spies } = makeDispatcher(); + await expectValidationFailure( + dispatcher.handleAutomation('/welcome_flow', 'PUT', body, CTX), + JSON.stringify(body), + ); + expect(spies.registerFlow).not.toHaveBeenCalled(); + }); + + it('accepts a bare definition and the pre-existing { definition } envelope alike', async () => { + const { dispatcher, spies } = makeDispatcher(); + const definition = { trigger: { type: 'manual' } }; + + const bare = await dispatcher.handleAutomation('/welcome_flow', 'PUT', definition, CTX); + expect(bare.response?.status).toBe(200); + expect(spies.registerFlow).toHaveBeenLastCalledWith('welcome_flow', definition); + + const wrapped = await dispatcher.handleAutomation('/welcome_flow', 'PUT', { definition }, CTX); + expect(wrapped.response?.status).toBe(200); + expect(spies.registerFlow).toHaveBeenLastCalledWith('welcome_flow', definition); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 61f200469d..70c596f911 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -12,6 +12,7 @@ import { CoreServiceName } from '@objectstack/spec/system'; import type { IAutomationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; +import { validationFailure } from '../validation-failure.js'; import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -160,7 +161,22 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // POST / → createFlow if (parts.length === 0 && m === 'POST') { if (typeof automationService.registerFlow === 'function') { - automationService.registerFlow(body?.name, body); + // [#3899] `registerFlow(body?.name, body)` used to run unchecked, so + // a definition whose `name` was missing or mistyped registered the + // flow under the key `undefined` — 200, body echoed back, caller + // convinced it succeeded, flow unreachable ever after. The name is + // the registry key; require it before touching the registry. + if (!body || typeof body !== 'object' || Array.isArray(body)) { + throw validationFailure('Flow definition body required', [ + { field: '(body)', code: 'invalid_type', message: 'expected a flow definition object' }, + ]); + } + if (typeof body.name !== 'string' || body.name.trim() === '') { + throw validationFailure('Flow definition requires a non-empty `name` (the registry key this flow is stored and triggered under)', [ + { field: 'name', code: body.name === undefined ? 'required' : 'invalid_type', message: 'expected a non-empty string' }, + ]); + } + automationService.registerFlow(body.name, body); return { handled: true, response: deps.success(body) }; } } @@ -253,8 +269,35 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // POST /:name/toggle → toggleFlow if (parts[1] === 'toggle' && m === 'POST') { if (typeof automationService.toggleFlow === 'function') { - await automationService.toggleFlow(name, body?.enabled ?? true); - return { handled: true, response: deps.success({ name, enabled: body?.enabled ?? true }) }; + // [#3899] The old read was `body?.enabled ?? true` on an + // otherwise-unchecked body, so `{"enable": false}` — one letter + // off — ENABLED the flow and answered 200 `{enabled: true}`; + // `{"enabled": "false"}` (a string) toggled on too. The caller + // trying to switch a flow OFF is exactly the caller this must + // not silently invert. Contract: `{ enabled?: boolean }`, empty + // body = enable (the SDK always sends the key; bodyless enable + // is the documented legacy shape). + const toggleBody = body ?? {}; + if (typeof toggleBody !== 'object' || Array.isArray(toggleBody)) { + throw validationFailure('Invalid toggle body — expected { enabled?: boolean }', [ + { field: '(body)', code: 'invalid_type', message: 'expected an object' }, + ]); + } + const unknownKeys = Object.keys(toggleBody).filter((k) => k !== 'enabled'); + if (unknownKeys.length > 0) { + throw validationFailure( + `Unknown key${unknownKeys.length > 1 ? 's' : ''} ${unknownKeys.map((k) => `\`${k}\``).join(', ')} — the toggle body is { enabled?: boolean }`, + unknownKeys.map((k) => ({ field: k, code: 'unrecognized_keys', message: 'not a toggle field — did you mean `enabled`?' })), + ); + } + if ('enabled' in toggleBody && typeof (toggleBody as Record).enabled !== 'boolean') { + throw validationFailure('`enabled` must be a boolean (JSON true/false, not a string)', [ + { field: 'enabled', code: 'invalid_type', message: 'expected a boolean' }, + ]); + } + const enabled = (toggleBody as { enabled?: boolean }).enabled ?? true; + await automationService.toggleFlow(name, enabled); + return { handled: true, response: deps.success({ name, enabled }) }; } } @@ -343,8 +386,21 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // PUT /:name → updateFlow if (parts.length === 1 && m === 'PUT') { if (typeof automationService.registerFlow === 'function') { - automationService.registerFlow(name, body?.definition ?? body); - return { handled: true, response: deps.success(body?.definition ?? body) }; + // [#3899] Same class as POST /: an unchecked body stored + // whatever arrived as the flow definition. The name rides the + // path here, so only the definition's shape needs guarding. + // (`body.definition ?? body` is a pre-existing two-dialect + // unwrap — kept as-is, not a new alias.) + const definition = (body && typeof body === 'object' && !Array.isArray(body)) + ? ((body as { definition?: unknown }).definition ?? body) + : undefined; + if (!definition || typeof definition !== 'object' || Array.isArray(definition)) { + throw validationFailure('Flow definition body required', [ + { field: '(body)', code: 'invalid_type', message: 'expected a flow definition object' }, + ]); + } + automationService.registerFlow(name, definition); + return { handled: true, response: deps.success(definition) }; } } diff --git a/packages/runtime/src/domains/notifications.ts b/packages/runtime/src/domains/notifications.ts index d6538056a6..85807d6af0 100644 --- a/packages/runtime/src/domains/notifications.ts +++ b/packages/runtime/src/domains/notifications.ts @@ -20,8 +20,10 @@ */ import { CoreServiceName } from '@objectstack/spec/system'; +import { MarkNotificationsReadRequestSchema } from '@objectstack/spec/api'; import type { INotificationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; +import { validationFailure, fieldsFromZodIssues } from '../validation-failure.js'; import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -112,8 +114,23 @@ export async function handleNotificationRequest( // POST /notifications/read — mark specific notifications read. if (subPath === 'read' && m === 'POST') { if (typeof inbox.markRead !== 'function') return capabilityUnavailable(deps, 'notification'); - const ids: string[] = Array.isArray(body?.ids) ? body.ids.map((x: unknown) => String(x)) : []; - const result = await inbox.markRead(userId, ids); + // [#3899] Validate against the declared contract + // (`MarkNotificationsReadRequestSchema`, catalog `requestSchema`). + // The old read was `Array.isArray(body?.ids) ? … : []`, so + // `{"notificationIds": [...]}` or `{"ids": "n1"}` silently became + // `markRead(userId, [])` — a success envelope, `readCount: 0`, and an + // unread badge that never cleared. The wrong key is now a 400 naming + // the field, thrown in the shape both dispatcher error exits map + // (#3918); the analytics entry gate is the precedent (#3878). + const parsed = MarkNotificationsReadRequestSchema.safeParse(body ?? {}); + if (!parsed.success) { + const fields = fieldsFromZodIssues(parsed.error.issues); + throw validationFailure( + `Invalid mark-read body — expected { ids: string[] }: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`, + fields, + ); + } + const result = await inbox.markRead(userId, parsed.data.ids); return { handled: true, response: deps.success(result) }; } diff --git a/packages/runtime/src/request-schema-gate.conformance.test.ts b/packages/runtime/src/request-schema-gate.conformance.test.ts new file mode 100644 index 0000000000..1fca9f33d2 --- /dev/null +++ b/packages/runtime/src/request-schema-gate.conformance.test.ts @@ -0,0 +1,152 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3899 ④ — the request-side gate, dispatcher half: every route the catalog + * (`plugin-rest-api.zod.ts`) declares a `requestSchema` for and the DISPATCHER + * serves must reject a body violating that schema — never silently coerce it + * into different semantics (an empty mark-read, an unfiltered aggregate). + * + * Driven at the dispatcher facade (`handleNotification` / `handleAnalytics`), + * where a violating body raises the duck-typed validation failure + * (`validationFailure`, `validation-failure.ts`) that BOTH HTTP error exits map + * to `400 VALIDATION_FAILED` + `details.fields[]` — that thrown-shape → wire + * contract is pinned separately by `dispatcher-validation-error.test.ts` + * (#3918), for the exact `name`/`code`/`fields` shape asserted here. + * + * The COMPLETENESS half is the ratchet: a new catalog `requestSchema` on a + * dispatcher-served route with no violating-body case here fails the suite, so + * a declaration can no longer outrun its enforcement — the #3899 ① disease. + * (The rest-served groups have the same gate in + * `packages/rest/src/request-schema-gate.conformance.test.ts`; the spec-side + * name-resolution half lives in + * `packages/spec/src/api/plugin-rest-api.schema-refs.test.ts`.) + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { + DEFAULT_NOTIFICATION_ROUTES, + DEFAULT_ANALYTICS_ROUTES, + DEFAULT_AUTOMATION_ROUTES, +} from '@objectstack/spec/api'; +import { HttpDispatcher } from './http-dispatcher.js'; +import { validationFailureDetails } from './validation-failure.js'; + +const BODY_METHODS = new Set(['POST', 'PUT', 'PATCH']); + +/** The catalog groups whose mounted routes the dispatcher serves. */ +const DISPATCHER_GROUPS = [ + DEFAULT_NOTIFICATION_ROUTES, + DEFAULT_ANALYTICS_ROUTES, + DEFAULT_AUTOMATION_ROUTES, +]; + +function makeDispatcher() { + const spies = { + markRead: vi.fn(async () => ({ success: true, readCount: 1 })), + listInbox: vi.fn(async () => ({ notifications: [], unreadCount: 0 })), + query: vi.fn(async () => ({ data: [] })), + getMeta: vi.fn(async () => ({ cubes: [] })), + }; + const services: Record = { + notification: { listInbox: spies.listInbox, markRead: spies.markRead }, + analytics: { query: spies.query, getMeta: spies.getMeta }, + }; + const resolve = (name: string) => services[name]; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + return { dispatcher: new HttpDispatcher(kernel), spies }; +} + +/** An authenticated caller — the routes under test sit behind the auth gate. */ +const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any; + +interface GateCase { + /** `METHOD prefix+path` exactly as the catalog spells it. */ + route: string; + /** Bodies that violate the declared requestSchema. */ + violating: unknown[]; + /** A minimal body the schema accepts. */ + valid: unknown; + drive: (dispatcher: HttpDispatcher, body: unknown) => Promise; + /** The service spy the route lands on — never called for a violating body. */ + spy: (spies: ReturnType['spies']) => ReturnType; +} + +/** + * One case per catalog route with a `requestSchema` served by the dispatcher. + * The completeness test below forces this table to keep up with the catalog. + */ +const CASES: GateCase[] = [ + { + route: 'POST /api/v1/notifications/read', + // The #3899 ③ shapes: a misnamed key and a non-array both used to + // become `markRead(userId, [])` — 200, badge never clears. + violating: [{ notificationIds: ['n1'] }, { ids: 'n1' }, { ids: [42] }, 'garbage', [['n1']]], + valid: { ids: ['n1', 'n2'] }, + drive: (d, body) => d.handleNotification('/read', 'POST', body, {}, CTX), + spy: (s) => s.markRead, + }, + { + route: 'POST /api/v1/analytics/query', + // Missing measures / retired envelope key — the #3878 shapes. + violating: [{}, { cube: 'crm_account', query: { measures: ['count'] } }, { cube: 'crm_account' }], + valid: { cube: 'crm_account', measures: ['count'] }, + drive: (d, body) => d.handleAnalytics('/query', 'POST', body, CTX), + spy: (s) => s.query, + }, +]; + +describe('#3899 ④ — dispatcher routes with a declared requestSchema reject violating bodies', () => { + it('every dispatcher-served catalog requestSchema has a violating-body case (ratchet)', () => { + const covered = new Set(CASES.map((c) => c.route)); + for (const group of DISPATCHER_GROUPS) { + for (const ep of group.endpoints ?? []) { + if (!ep.requestSchema || !BODY_METHODS.has(ep.method)) continue; + const route = `${ep.method} ${group.prefix}${ep.path}`; + expect( + covered.has(route), + `${route} declares requestSchema '${ep.requestSchema}' but has no violating-body case in this suite — add one, or the declaration is a promise nothing checks`, + ).toBe(true); + } + } + }); + + for (const gateCase of CASES) { + describe(gateCase.route, () => { + it('rejects each violating body with the VALIDATION_FAILED shape both HTTP exits map to 400', async () => { + for (const body of gateCase.violating) { + const { dispatcher, spies } = makeDispatcher(); + let thrown: unknown; + try { + await gateCase.drive(dispatcher, body); + } catch (e) { + thrown = e; + } + expect( + thrown, + `${gateCase.route} accepted ${JSON.stringify(body)} instead of rejecting it`, + ).toBeDefined(); + // The duck shape `dispatcher-validation-error.test.ts` pins + // to a wire 400 on both exits — recognised by the SAME + // predicate the exits use, so this cannot drift from them. + const details = validationFailureDetails(thrown); + expect(details?.code).toBe('VALIDATION_FAILED'); + expect(details?.fields.length).toBeGreaterThan(0); + expect(gateCase.spy(spies)).not.toHaveBeenCalled(); + } + }); + + it('still serves a conforming body (the 400 above is the schema, not a broken route)', async () => { + const { dispatcher, spies } = makeDispatcher(); + const result = await gateCase.drive(dispatcher, gateCase.valid); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(200); + expect(gateCase.spy(spies)).toHaveBeenCalledTimes(1); + }); + }); + } +}); diff --git a/packages/runtime/src/validation-failure.ts b/packages/runtime/src/validation-failure.ts index af7c7263aa..41ed4aa153 100644 --- a/packages/runtime/src/validation-failure.ts +++ b/packages/runtime/src/validation-failure.ts @@ -45,3 +45,35 @@ export function validationFailureDetails(err: any): ValidationFailureDetails | u fields: Array.isArray(err.fields) ? err.fields : [], }; } + +/** + * [#3878/#3899] The CONSTRUCTOR for the shape {@link validationFailureDetails} + * recognises — kept in the same module so the two can never drift. Thrown from + * a domain handler, both dispatcher error exits map it to + * `400 VALIDATION_FAILED` + `details.fields[]` (#3918) with no new error + * channel and no runtime dependency on objectql's `ValidationError` class. + * First built inline by the analytics domain; hoisted here when notifications + * and automation grew the same entry gates rather than a third copy. + */ +export function validationFailure(message: string, fields: unknown[]): Error { + const err = new Error(message) as Error & { code: string; fields: unknown[] }; + err.name = 'ValidationError'; + err.code = 'VALIDATION_FAILED'; + err.fields = fields; + return err; +} + +/** + * Zod issues → the dispatcher's `fields[]` envelope entries + * (`{ field, code, message }`). `'(body)'` names a root-level failure — a body + * that is the wrong TYPE entirely has no path to point at. + */ +export function fieldsFromZodIssues( + issues: Array<{ path: Array; code: string; message: string }>, +): Array<{ field: string; code: string; message: string }> { + return issues.map((issue) => ({ + field: issue.path.length > 0 ? issue.path.join('.') : '(body)', + code: issue.code, + message: issue.message, + })); +} diff --git a/packages/spec/src/api/plugin-rest-api.schema-refs.test.ts b/packages/spec/src/api/plugin-rest-api.schema-refs.test.ts new file mode 100644 index 0000000000..11fa956403 --- /dev/null +++ b/packages/spec/src/api/plugin-rest-api.schema-refs.test.ts @@ -0,0 +1,104 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3899 — the catalog's schema references are strings, and nothing consumed + * them at runtime, so nothing ever noticed when one was wrong. Two ways wrong, + * both shipped: + * + * - `requestSchema: 'CreateManyRequestSchema'` named a schema no file ever + * exported (#3939 recorded it; repointed later). A reader — or an AI + * generating a client from the catalog — resolved a promise against a + * schema that did not exist. + * - `requestSchema` sat on GET/DELETE routes whose inputs are entirely + * path/query-bound, reading as "this endpoint 400s a bad body" for routes + * that have no body to validate. + * + * These checks make both structural: a dangling name or a body schema on a + * bodyless method fails here, not in a consumer's generated client. + * + * The other half of the same gate — "a declared `requestSchema` actually 400s + * a violating body" — needs the mounted routes, so it lives with the servers: + * `packages/rest/src/request-schema-gate.conformance.test.ts` and + * `packages/runtime/src/request-schema-gate.conformance.test.ts`. This file is + * deliberately only the part the spec package can verify about itself. + */ + +import { describe, it, expect } from 'vitest'; +import { getDefaultRouteRegistrations } from './plugin-rest-api.zod'; +import * as API from './index'; + +/** HTTP methods that carry a request body a schema could reject. */ +const BODY_METHODS = new Set(['POST', 'PUT', 'PATCH']); + +interface EndpointRef { + route: string; + requestSchema?: string; + responseSchema?: string; + method: string; +} + +function allEndpoints(): EndpointRef[] { + return getDefaultRouteRegistrations().flatMap((reg) => + (reg.endpoints ?? []).map((ep) => ({ + route: `${ep.method} ${reg.prefix}${ep.path}`, + requestSchema: ep.requestSchema, + responseSchema: ep.responseSchema, + method: ep.method, + })), + ); +} + +function resolvesToZodSchema(name: string): boolean { + const candidate = (API as Record)[name]; + return !!candidate && typeof (candidate as { safeParse?: unknown }).safeParse === 'function'; +} + +describe('#3899 — catalog schema references resolve and sit on the right methods', () => { + it('every requestSchema names a Zod schema exported from @objectstack/spec/api', () => { + for (const ep of allEndpoints()) { + if (!ep.requestSchema) continue; + expect( + resolvesToZodSchema(ep.requestSchema), + `${ep.route} declares requestSchema '${ep.requestSchema}', which is not an exported Zod schema — a promise no consumer can resolve`, + ).toBe(true); + } + }); + + it('every responseSchema names a Zod schema exported from @objectstack/spec/api', () => { + for (const ep of allEndpoints()) { + if (!ep.responseSchema) continue; + expect( + resolvesToZodSchema(ep.responseSchema), + `${ep.route} declares responseSchema '${ep.responseSchema}', which is not an exported Zod schema`, + ).toBe(true); + } + }); + + it('requestSchema appears only on body-carrying methods (POST/PUT/PATCH)', () => { + // A GET/DELETE input is path/query-bound: strings all the way down, no + // body for a schema to reject. A requestSchema there is a promise of a + // validation that cannot happen — the #3899 ① shape. + for (const ep of allEndpoints()) { + if (!ep.requestSchema) continue; + expect( + BODY_METHODS.has(ep.method), + `${ep.route} declares requestSchema '${ep.requestSchema}' on a bodyless method — nothing can violate it, so the declaration only misleads`, + ).toBe(true); + } + }); + + it('at least the routes wired in #3899 still declare their request schemas', () => { + // Anti-erosion floor: the gate above is vacuous if declarations are simply + // deleted. These five are validated at their mounted routes. + const declared = new Map( + allEndpoints().filter((e) => e.requestSchema).map((e) => [e.route, e.requestSchema]), + ); + expect(declared.get('POST /api/v1/data/:object/query')).toBe('FindDataRequestSchema'); + expect(declared.get('POST /api/v1/data/:object')).toBe('CreateDataRequestSchema'); + expect(declared.get('PATCH /api/v1/data/:object/:id')).toBe('UpdateDataRequestSchema'); + expect(declared.get('POST /api/v1/data/:object/batch')).toBe('BatchUpdateRequestSchema'); + expect(declared.get('POST /api/v1/data/:object/createMany')).toBe('CreateManyDataRequestSchema'); + expect(declared.get('POST /api/v1/notifications/read')).toBe('MarkNotificationsReadRequestSchema'); + expect(declared.get('POST /api/v1/analytics/query')).toBe('AnalyticsQueryRequestSchema'); + }); +}); diff --git a/packages/spec/src/api/plugin-rest-api.test.ts b/packages/spec/src/api/plugin-rest-api.test.ts index 5411f249fd..4648e982f7 100644 --- a/packages/spec/src/api/plugin-rest-api.test.ts +++ b/packages/spec/src/api/plugin-rest-api.test.ts @@ -484,7 +484,13 @@ describe('plugin-rest-api.zod', () => { expect(DEFAULT_DATA_CRUD_ROUTES.methods).toContain('createData'); expect(DEFAULT_DATA_CRUD_ROUTES.methods).toContain('updateData'); expect(DEFAULT_DATA_CRUD_ROUTES.methods).toContain('deleteData'); - expect(DEFAULT_DATA_CRUD_ROUTES.endpoints).toHaveLength(5); + // 6 = the CRUD five + POST /:object/query, the QueryAST-in-body spelling + // of findData the servers have always mounted but this table omitted + // until its request contract was wired (#3899). + expect(DEFAULT_DATA_CRUD_ROUTES.endpoints).toHaveLength(6); + const queryEndpoint = DEFAULT_DATA_CRUD_ROUTES.endpoints?.find(e => e.path === '/:object/query'); + expect(queryEndpoint?.method).toBe('POST'); + expect(queryEndpoint?.requestSchema).toBe('FindDataRequestSchema'); }); it('should validate DEFAULT_BATCH_ROUTES', () => { @@ -507,11 +513,17 @@ describe('plugin-rest-api.zod', () => { expect(DEFAULT_NOTIFICATION_ROUTES.prefix).toBe('/api/v1/notifications'); expect(DEFAULT_NOTIFICATION_ROUTES.service).toBe('notification'); expect(DEFAULT_NOTIFICATION_ROUTES.category).toBe('notification'); - expect(DEFAULT_NOTIFICATION_ROUTES.methods).toContain('registerDevice'); expect(DEFAULT_NOTIFICATION_ROUTES.methods).toContain('listNotifications'); expect(DEFAULT_NOTIFICATION_ROUTES.methods).toContain('markNotificationsRead'); expect(DEFAULT_NOTIFICATION_ROUTES.methods).toContain('markAllNotificationsRead'); - expect(DEFAULT_NOTIFICATION_ROUTES.endpoints).toHaveLength(7); + // 3 = list + read + read/all, the routes the dispatcher actually mounts. + // The devices/preferences endpoints this table used to declare were + // removed as server routes in #3612 (in fact never built) and are gone + // from the catalog too (#3899) — same cure as DEFAULT_AI_ROUTES (#3718). + expect(DEFAULT_NOTIFICATION_ROUTES.endpoints).toHaveLength(3); + const paths = DEFAULT_NOTIFICATION_ROUTES.endpoints?.map(e => e.path) ?? []; + expect(paths).not.toContain('/devices'); + expect(paths).not.toContain('/preferences'); }); it('declares no AI routes — this table cannot vouch for a Cloud/EE surface (#3718)', () => { @@ -559,8 +571,10 @@ describe('plugin-rest-api.zod', () => { expect(DEFAULT_AUTOMATION_ROUTES.service).toBe('automation'); expect(DEFAULT_AUTOMATION_ROUTES.category).toBe('automation'); expect(DEFAULT_AUTOMATION_ROUTES.methods).toContain('triggerAutomation'); - // POST /trigger + GET /actions (ADR-0018 action descriptor registry) + // POST /trigger/:name (the mounted path — flow name rides the PATH, + // #3899) + GET /actions (ADR-0018 action descriptor registry) expect(DEFAULT_AUTOMATION_ROUTES.endpoints).toHaveLength(2); + expect(DEFAULT_AUTOMATION_ROUTES.endpoints?.[0].path).toBe('/trigger/:name'); // Automation trigger should have extended timeout expect(DEFAULT_AUTOMATION_ROUTES.endpoints?.[0].timeout).toBe(120000); // The actions endpoint exposes the live registry and is cacheable. diff --git a/packages/spec/src/api/plugin-rest-api.zod.ts b/packages/spec/src/api/plugin-rest-api.zod.ts index 2e3ef4d018..fecd761a81 100644 --- a/packages/spec/src/api/plugin-rest-api.zod.ts +++ b/packages/spec/src/api/plugin-rest-api.zod.ts @@ -763,7 +763,11 @@ export const DEFAULT_METADATA_ROUTES: RestApiRouteRegistration = { summary: 'Get specific metadata item', description: 'Returns a specific metadata item by type and name', tags: ['Metadata'], - requestSchema: 'GetMetaItemRequestSchema', + // No `requestSchema` (#3899): every input (`:type`, `:name`, `?packageId`) + // is path/query-bound, so `GetMetaItemRequestSchema` — still the protocol + // METHOD contract — can never be violated by a request body. Declaring it + // here read as "this endpoint 400s a bad body", a validation no route + // performs or could perform. responseSchema: 'GetMetaItemResponseSchema', cacheable: true, cacheTtl: 3600, @@ -777,7 +781,11 @@ export const DEFAULT_METADATA_ROUTES: RestApiRouteRegistration = { summary: 'Create or update metadata item', description: 'Creates or updates a metadata item', tags: ['Metadata'], - requestSchema: 'SaveMetaItemRequestSchema', + // No `requestSchema` (#3899): `SaveMetaItemRequestSchema.item` is + // `z.unknown()`, so an entry parse can never fail — the declaration + // promised a gate with no teeth. The REAL request gate is the metadata + // layer's per-type validation behind `saveMetaItem`, which rejects an + // off-spec item with 422 + `issues`. responseSchema: 'SaveMetaItemResponseSchema', permissions: ['metadata.write'], cacheable: false, @@ -810,11 +818,35 @@ export const DEFAULT_DATA_CRUD_ROUTES: RestApiRouteRegistration = { summary: 'Query records', description: 'Query records with filtering, sorting, and pagination', tags: ['Data'], - requestSchema: 'FindDataRequestSchema', + // No `requestSchema` (#3899): a GET carries its query in the query + // string, where every value is a string — `FindDataRequestSchema`'s typed + // `query` (QuerySchema: numeric limit/offset, structured where) cannot + // describe it, and nothing ever validated it. The schema-guarded spelling + // of this operation is `POST /:object/query` below. responseSchema: 'ListRecordResponseSchema', permissions: ['data.read'], cacheable: false, }, + { + // The QueryAST-in-body spelling of `findData` — what `client.data.query()` + // posts. Mounted by `@objectstack/rest` since forever, but absent from + // this table until #3899 wired its request contract: the body is the + // QueryAST (`spec/data/query.zod.ts`), validated at the route via + // `FindDataRequestSchema` with the PATH object written last (#3946), so a + // malformed body 400s instead of degrading into an unfiltered full read. + method: 'POST', + path: '/:object/query', + handler: 'findData', + category: 'data', + public: false, + summary: 'Advanced query (QueryAST in body)', + description: 'Query records with a structured QueryAST request body (filter, sort, aggregation, expansion)', + tags: ['Data'], + requestSchema: 'FindDataRequestSchema', + responseSchema: 'FindDataResponseSchema', + permissions: ['data.read'], + cacheable: false, + }, { method: 'GET', path: '/:object/:id', @@ -824,7 +856,8 @@ export const DEFAULT_DATA_CRUD_ROUTES: RestApiRouteRegistration = { summary: 'Get record by ID', description: 'Retrieve a single record by its ID', tags: ['Data'], - requestSchema: 'IdRequestSchema', + // No `requestSchema` (#3899): `:id` is path-bound and always a string — + // `IdRequestSchema` can never be violated by a request body. responseSchema: 'SingleRecordResponseSchema', permissions: ['data.read'], cacheable: false, @@ -838,7 +871,12 @@ export const DEFAULT_DATA_CRUD_ROUTES: RestApiRouteRegistration = { summary: 'Create record', description: 'Create a new record', tags: ['Data'], - requestSchema: 'CreateRequestSchema', + // Was 'CreateRequestSchema' — the generic service-contract envelope + // (`{ data }`), which is NOT what this route receives: the wire body IS + // the bare record. `CreateDataRequestSchema` is the protocol request the + // route assembles ({ object } from the path + `data` = body) and, since + // #3899, validates before dispatch. + requestSchema: 'CreateDataRequestSchema', responseSchema: 'SingleRecordResponseSchema', permissions: ['data.create'], cacheable: false, @@ -852,7 +890,11 @@ export const DEFAULT_DATA_CRUD_ROUTES: RestApiRouteRegistration = { summary: 'Update record', description: 'Update an existing record', tags: ['Data'], - requestSchema: 'UpdateRequestSchema', + // Was 'UpdateRequestSchema' — same mismatch as create: the wire body is + // the bare field patch (plus optional `expectedVersion`, stripped into + // its own protocol field). `UpdateDataRequestSchema` is what the route + // assembles from path + body and, since #3899, validates. + requestSchema: 'UpdateDataRequestSchema', responseSchema: 'SingleRecordResponseSchema', permissions: ['data.update'], cacheable: false, @@ -866,7 +908,8 @@ export const DEFAULT_DATA_CRUD_ROUTES: RestApiRouteRegistration = { summary: 'Delete record', description: 'Delete a record by ID', tags: ['Data'], - requestSchema: 'IdRequestSchema', + // No `requestSchema` (#3899): DELETE carries no body; `:id` is + // path-bound (`?expectedVersion` rides the query string / If-Match). responseSchema: 'DeleteResponseSchema', permissions: ['data.delete'], cacheable: false, @@ -918,6 +961,10 @@ export const DEFAULT_BATCH_ROUTES: RestApiRouteRegistration = { // Was 'CreateManyRequestSchema' — a name no schema ever exported (the real // request contract is CreateManyDataRequestSchema in protocol.zod.ts). A // dangling documentation reference; point it at the schema that exists. + // NOTE the wire body is the BARE records array (what `client.data + // .createMany` posts); the route assembles `{ object }` from the path + + // `records` = body and, since #3899, validates that assembly before + // dispatch. requestSchema: 'CreateManyDataRequestSchema', responseSchema: 'BatchUpdateResponseSchema', permissions: ['data.create', 'data.batch'], @@ -981,62 +1028,19 @@ export const DEFAULT_NOTIFICATION_ROUTES: RestApiRouteRegistration = { service: 'notification', category: 'notification', methods: [ - 'registerDevice', 'unregisterDevice', - 'getNotificationPreferences', 'updateNotificationPreferences', 'listNotifications', 'markNotificationsRead', 'markAllNotificationsRead', ], authRequired: true, + // The device-registration and preferences endpoints are GONE from this table + // (#3899). `POST/DELETE /devices*` and `GET/PATCH /preferences` were removed + // as SERVER routes in #3612 — in fact they were never built ("the + // /notifications/devices and /notifications/preferences server routes that + // ADR-0012 describes were never built", packages/client/src/index.ts) — yet + // this table kept declaring them, requestSchema and all, so the catalog + // promised validation on routes that 404. Same disease, same cure as + // DEFAULT_AI_ROUTES (#3718). The `RegisterDevice*` / `*Preferences*` schemas + // in protocol.zod.ts remain exported as protocol-method contracts. endpoints: [ - { - method: 'POST', - path: '/devices', - handler: 'registerDevice', - category: 'notification', - public: false, - summary: 'Register device for push notifications', - description: 'Registers a device token for receiving push notifications', - tags: ['Notifications'], - requestSchema: 'RegisterDeviceRequestSchema', - responseSchema: 'RegisterDeviceResponseSchema', - cacheable: false, - }, - { - method: 'DELETE', - path: '/devices/:deviceId', - handler: 'unregisterDevice', - category: 'notification', - public: false, - summary: 'Unregister device', - description: 'Removes a device from push notification registration', - tags: ['Notifications'], - responseSchema: 'UnregisterDeviceResponseSchema', - cacheable: false, - }, - { - method: 'GET', - path: '/preferences', - handler: 'getNotificationPreferences', - category: 'notification', - public: false, - summary: 'Get notification preferences', - description: 'Returns current user notification preferences', - tags: ['Notifications'], - responseSchema: 'GetNotificationPreferencesResponseSchema', - cacheable: false, - }, - { - method: 'PATCH', - path: '/preferences', - handler: 'updateNotificationPreferences', - category: 'notification', - public: false, - summary: 'Update notification preferences', - description: 'Updates user notification preferences', - tags: ['Notifications'], - requestSchema: 'UpdateNotificationPreferencesRequestSchema', - responseSchema: 'UpdateNotificationPreferencesResponseSchema', - cacheable: false, - }, { method: 'GET', path: '', @@ -1237,14 +1241,26 @@ export const DEFAULT_AUTOMATION_ROUTES: RestApiRouteRegistration = { endpoints: [ { method: 'POST', - path: '/trigger', + // Was '/trigger' — a path no server has ever mounted. The real mounts + // are `POST /trigger/:name` (what `client.automation.trigger()` calls) + // and the alias `POST /:name/trigger`; the flow name rides the PATH, not + // the body (#3899). + path: '/trigger/:name', handler: 'triggerAutomation', category: 'automation', public: false, summary: 'Trigger automation', - description: 'Triggers an automation flow or script by name', + description: + 'Triggers an automation flow by name (path param). Alias mount: POST /:name/trigger. ' + + 'The body is the flow trigger context ({ recordId?, objectName?, params? }; unknown ' + + 'top-level keys are forwarded as flow params).', tags: ['Automation'], - requestSchema: 'AutomationTriggerRequestSchema', + // No `requestSchema` (#3899): 'AutomationTriggerRequestSchema' + // ({ trigger, payload }) described a wire shape this route never had — + // the name is path-bound and the body is a lenient params bag translated + // by the dispatcher's `buildAutomationContext`. Declaring that schema + // here promised a 400 nothing performs; it remains exported as the + // protocol-method contract only. responseSchema: 'AutomationTriggerResponseSchema', permissions: ['automation.trigger'], timeout: 120000, // 2 minutes for long-running automations From 188ccf3317234038474a1cd763da96d5a5d2cfdf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:16:47 +0000 Subject: [PATCH 2/8] fix(spec): QuerySchema declares the search contract ADR-0061 actually serves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first CI run of the #3899 request-schema gate rejected the dogfood search proof's own wire shape: `{ search: 'retail', searchFields: ['industry'] }` answered 400. The schema was the wrong half — QuerySchema declared only the structured FullTextSearchSchema form while ADR-0061 D1 ("the client sends only the query text"), the engine executor (search-filter.ts), and the search-conformance ledger all serve the bare string plus the validated `searchFields` narrowing. - `search` becomes `string | FullTextSearch` (string is the canonical Tier-1 spelling; the object form keeps the declared Tier-2 knobs) - `searchFields` is formally declared (the ADR's own P1 item), noted as server-intersected — can only narrow, never widen - rest request-schema gate pins the ADR-0061 wire shape as a positive case so entry validation can never 400 it again - regenerated: references docs + authorable-surface (adds `data/Query:searchFields`); all 8 artifact gates green spec suite 7145 green; both previously-failing dogfood files pass locally (showcase-search 4/4, two-factor-lockout 5/5 — the 2FA pair does not reproduce here and is green on main @ 5d21a48's identical job). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- content/docs/references/api/contract.mdx | 5 +-- content/docs/references/api/protocol.mdx | 2 +- content/docs/references/data/data-engine.mdx | 2 +- content/docs/references/data/mapping.mdx | 2 +- content/docs/references/data/query.mdx | 3 +- .../request-schema-gate.conformance.test.ts | 14 ++++++++ packages/spec/authorable-surface.json | 1 + packages/spec/src/data/query.zod.ts | 32 ++++++++++++++++--- 8 files changed, 51 insertions(+), 10 deletions(-) diff --git a/content/docs/references/api/contract.mdx b/content/docs/references/api/contract.mdx index 7994f13e54..2722ff2045 100644 --- a/content/docs/references/api/contract.mdx +++ b/content/docs/references/api/contract.mdx @@ -149,7 +149,8 @@ const result = ApiError.parse(data); | **object** | `string` | ✅ | Object name (e.g. account) | | **fields** | `string[]` | optional | Fields to retrieve — field names, optionally dotted to reach through a relationship (`owner.name`). Related *records* are selected with `expand`, not from inside this list. | | **where** | `any` | optional | Filtering criteria (WHERE) | -| **search** | `{ query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | Full-text search configuration ($search parameter) | +| **search** | `string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | Full-text search — the query text (canonical, ADR-0061 D1), or a structured FullTextSearch configuration | +| **searchFields** | `string[]` | optional | Narrow the search to these fields (server-intersected with the allowed searchable set — can only narrow, never widen; ADR-0061 D1) | | **orderBy** | `{ field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | Sorting instructions (ORDER BY) | | **limit** | `number` | optional | Max records to return (LIMIT) | | **offset** | `number` | optional | Records to skip (OFFSET) | @@ -161,7 +162,7 @@ const result = ApiError.parse(data); | **having** | `any` | optional | HAVING — filter over the AGGREGATED rows (aggregation aliases + groupBy projections); applied engine-side after aggregation | | **windowFunctions** | `any` | optional | [REMOVED] `query.windowFunctions` was removed in @objectstack/spec 18 (#4286, ADR-0049) — `find()` never applied it: no engine or driver read the key on the query path, so every OVER clause it declared was silently dropped. Delete the key. Window functions are a SQL-driver capability behind `SqlDriver.findWithWindowFunctions(object, query)` (embedder-level; not on the `IDataDriver` contract or the REST surface); request-level analytics are `aggregations` + `groupBy`. | | **distinct** | `any` | optional | [REMOVED] `query.distinct` was removed in @objectstack/spec 18 (#4286, ADR-0049 / ADR-0078) — no driver ever rendered SELECT DISTINCT; the flag's only observable effect was MIS-WIRED: the REST list path treated a distinct query as not countable and silently degraded `total`/`hasMore` to a page-local estimate while still returning duplicate rows. Delete the key; `QueryBuilder.distinct()` was removed with it, and the count suppression is gone (`total` is truthful again). For unique values of one column use the SQL/memory drivers' `distinct(object, field)` door; for unique combinations, `groupBy`; for a deduplicated count, the `count_distinct` aggregation. | -| **expand** | `Record` | optional | Recursive relation loading map. Keys are lookup/master_detail field names; values are nested QueryAST objects that control select (`fields`) and filter (`where`, AND-merged with the batch $in), plus further expansion on the related object. The engine resolves expand via batch $in queries (driver-agnostic) with a default max depth of 3; per-parent `limit`/`offset`/`orderBy` are NOT applied on this path. | +| **expand** | `Record; … }; … }>` | optional | Recursive relation loading map. Keys are lookup/master_detail field names; values are nested QueryAST objects that control select (`fields`) and filter (`where`, AND-merged with the batch $in), plus further expansion on the related object. The engine resolves expand via batch $in queries (driver-agnostic) with a default max depth of 3; per-parent `limit`/`offset`/`orderBy` are NOT applied on this path. | --- diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index a4ac8433b6..8b9ffc283b 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -529,7 +529,7 @@ const result = AiAgentCapabilities.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **object** | `string` | ✅ | The unique machine name of the object to query (e.g. "account"). | -| **query** | `{ object: string; fields?: string[]; where?: any; search?: object; … }` | optional | Structured query definition (filter, sort, select, pagination). | +| **query** | `{ object: string; fields?: string[]; where?: any; search?: string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }; … }` | optional | Structured query definition (filter, sort, select, pagination). | --- diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 9d385c34b5..a844015270 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -548,7 +548,7 @@ QueryAST-aligned query options for IDataEngine.find() operations | **top** | `number` | optional | | | **cursor** | `any` | optional | [REMOVED] `query.cursor` was removed in @objectstack/spec 18 (#4286, ADR-0049) — no driver ever implemented keyset pagination, so the cursor was accepted and ignored and every page came back identical (a caller looping "until hasMore is false" never terminates). Delete the key; `QueryBuilder.cursor()` was removed with it. Express the keyset as an ordinary `where` predicate on your sort key — `where: { created_at: { $gt: last.created_at } }` with the matching `orderBy` — which every driver executes with canonicalised comparands. A first-class cursor, if ever built, will be a response-minted opaque token, not this caller-built record. | | **search** | `{ query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | | -| **expand** | `Record` | optional | | +| **expand** | `Record; … }; … }>` | optional | | | **distinct** | `any` | optional | [REMOVED] `query.distinct` was removed in @objectstack/spec 18 (#4286, ADR-0049 / ADR-0078) — no driver ever rendered SELECT DISTINCT; the flag's only observable effect was MIS-WIRED: the REST list path treated a distinct query as not countable and silently degraded `total`/`hasMore` to a page-local estimate while still returning duplicate rows. Delete the key; `QueryBuilder.distinct()` was removed with it, and the count suppression is gone (`total` is truthful again). For unique values of one column use the SQL/memory drivers' `distinct(object, field)` door; for unique combinations, `groupBy`; for a deduplicated count, the `count_distinct` aggregation. | diff --git a/content/docs/references/data/mapping.mdx b/content/docs/references/data/mapping.mdx index 4cd5f73fd9..3d91ef5025 100644 --- a/content/docs/references/data/mapping.mdx +++ b/content/docs/references/data/mapping.mdx @@ -52,7 +52,7 @@ const result = FieldMapping.parse(data); | **fieldMapping** | `{ source: string \| string[]; target: string \| string[]; transform: Enum<'none' \| 'constant' \| 'lookup' \| 'split' \| 'join' \| 'javascript' \| 'map'>; params?: object }[]` | ✅ | | | **mode** | `Enum<'insert' \| 'update' \| 'upsert'>` | ✅ | | | **upsertKey** | `string[]` | optional | Fields to match for upsert (e.g. email) | -| **extractQuery** | `{ object: string; fields?: string[]; where?: any; search?: object; … }` | optional | Query to run for export only | +| **extractQuery** | `{ object: string; fields?: string[]; where?: any; search?: string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }; … }` | optional | Query to run for export only | | **errorPolicy** | `Enum<'skip' \| 'abort' \| 'retry'>` | ✅ | | | **batchSize** | `number` | ✅ | | diff --git a/content/docs/references/data/query.mdx b/content/docs/references/data/query.mdx index ea085834b8..4f21c1286f 100644 --- a/content/docs/references/data/query.mdx +++ b/content/docs/references/data/query.mdx @@ -126,7 +126,8 @@ Type: `string` | **object** | `string` | ✅ | Object name (e.g. account) | | **fields** | `string[]` | optional | Fields to retrieve — field names, optionally dotted to reach through a relationship (`owner.name`). Related *records* are selected with `expand`, not from inside this list. | | **where** | `any` | optional | Filtering criteria (WHERE) | -| **search** | `{ query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | Full-text search configuration ($search parameter) | +| **search** | `string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | Full-text search — the query text (canonical, ADR-0061 D1), or a structured FullTextSearch configuration | +| **searchFields** | `string[]` | optional | Narrow the search to these fields (server-intersected with the allowed searchable set — can only narrow, never widen; ADR-0061 D1) | | **orderBy** | `{ field: string; order: Enum<'asc' \| 'desc'> }[]` | optional | Sorting instructions (ORDER BY) | | **limit** | `number` | optional | Max records to return (LIMIT) | | **offset** | `number` | optional | Records to skip (OFFSET) | diff --git a/packages/rest/src/request-schema-gate.conformance.test.ts b/packages/rest/src/request-schema-gate.conformance.test.ts index da93db5e50..88fde01faa 100644 --- a/packages/rest/src/request-schema-gate.conformance.test.ts +++ b/packages/rest/src/request-schema-gate.conformance.test.ts @@ -181,6 +181,20 @@ describe('#3899 ④ — rest routes with a declared requestSchema reject violati } }); + it('the ADR-0061 search wire contract stays valid: { search: string, searchFields: [...] }', async () => { + // The first CI run of this gate rejected exactly this body: QuerySchema + // declared only the structured `FullTextSearchSchema` form while the + // executor and the ADR-0061 dogfood proof (`showcase-search.dogfood + // .test.ts`) served the bare string + `searchFields`. The schema was the + // wrong half — fixed there; pinned here so entry validation can never + // again 400 the contract the conformance ledger declares enforced. + const ctx = setup(); + const res = await drive(ctx.rest, 'POST', '/api/v1/data/:object/query', + { search: 'retail', searchFields: ['industry'] }, PARAMS); + expect(res.statusCode, JSON.stringify(res.body)).toBe(200); + expect(ctx.findData).toHaveBeenCalledTimes(1); + }); + for (const gateCase of CASES) { const [method, path] = gateCase.route.split(' ') as [string, string]; describe(gateCase.route, () => { diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 933b9f0740..0cd747cb83 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -3737,6 +3737,7 @@ "data/Query:offset", "data/Query:orderBy", "data/Query:search", + "data/Query:searchFields", "data/Query:top", "data/Query:where", "data/Query:windowFunctions [RETIRED]", diff --git a/packages/spec/src/data/query.zod.ts b/packages/spec/src/data/query.zod.ts index 0ecace2056..32eec59cdd 100644 --- a/packages/spec/src/data/query.zod.ts +++ b/packages/spec/src/data/query.zod.ts @@ -343,10 +343,34 @@ const BaseQuerySchema = z.object({ /** Where Clause (Filtering) */ where: FilterConditionSchema.optional().describe('Filtering criteria (WHERE)'), - - /** Full-Text Search */ - search: FullTextSearchSchema.optional().describe('Full-text search configuration ($search parameter)'), - + + /** + * Full-Text Search. + * + * The bare string IS the canonical Tier-1 contract (ADR-0061 D1: "the + * client sends only the query text; the server resolves which fields to + * search from object metadata") — it is what every surface sends and what + * the dogfood HTTP proof (`showcase-search.dogfood.test.ts`) pins. The + * structured `FullTextSearchSchema` form remains for the declared Tier-2 + * knobs. The union is schema-side drift REPAIR, not a new dialect: the + * schema declared only the object form while the executor and the ADR's own + * conformance ledger served the string — surfaced the moment #3899 started + * validating request bodies against this schema. + */ + search: z.union([z.string(), FullTextSearchSchema]).optional() + .describe('Full-text search — the query text (canonical, ADR-0061 D1), or a structured FullTextSearch configuration'), + + /** + * Per-query narrowing of the searchable field set (ADR-0061 D1). + * Server-validated: intersected with the object's allowed searchable + * fields — it can only narrow, never widen. Formalized here per the ADR's + * P1 ("formalize `$searchFields`"); the enforcement site is + * `objectql/src/search-filter.ts` `resolveSearchFields`, proven at the + * HTTP level by `showcase-search.dogfood.test.ts`. + */ + searchFields: z.array(z.string()).optional() + .describe('Narrow the search to these fields (server-intersected with the allowed searchable set — can only narrow, never widen; ADR-0061 D1)'), + /** Order By Clause (Sorting) */ orderBy: z.array(SortNodeSchema).optional().describe('Sorting instructions (ORDER BY)'), From 8c11136cbb6b65c81405a1600ae57aa1ca1fb8c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:17:11 +0000 Subject: [PATCH 3/8] docs(changeset): record the QuerySchema search-contract widening Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- .changeset/request-body-schema-validation.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/request-body-schema-validation.md b/.changeset/request-body-schema-validation.md index 3b353b7b34..6d537c9088 100644 --- a/.changeset/request-body-schema-validation.md +++ b/.changeset/request-body-schema-validation.md @@ -19,7 +19,9 @@ Request bodies are now checked against the schemas the API catalog declares for - `POST /automation` and `PUT /automation/:name` require a flow-definition object, and POST requires a non-empty `name` — a mistyped `name` used to register the flow under the key `undefined` and echo 200. - `POST /automation/:name/toggle` is strictly `{ enabled?: boolean }` — `{"enable": false}` (one letter off) used to ENABLE the flow and answer 200 `{enabled: true}`; it is now a 400 naming the offending key. An empty body still means enable. -**Catalog corrections in `@objectstack/spec` (`plugin-rest-api.zod.ts`)** — documentation-only tables, no schema exports changed: +**`QuerySchema` now declares the search contract ADR-0061 actually serves** (additive): `search` accepts the canonical bare query string as well as the structured `FullTextSearch` form, and the server-validated `searchFields` narrowing is formally declared. Previously the schema declared only the object form while every surface (and the ADR's own conformance proof) sent the string — drift that surfaced the moment request bodies started being validated. + +**Catalog corrections in `@objectstack/spec` (`plugin-rest-api.zod.ts`)** — documentation-only tables: - `DEFAULT_NOTIFICATION_ROUTES` drops the four device/preferences endpoints — those server routes were removed in #3612 (never built), yet the table kept declaring them, `requestSchema` and all. - `DEFAULT_AUTOMATION_ROUTES`' trigger endpoint path is corrected `/trigger` → `/trigger/:name` (the mounted path; the flow name rides the path) and its `AutomationTriggerRequestSchema` declaration is removed — that schema never described this route's wire shape. From 1cd85c7154251ed85ddcc11f2f76bef57a5a1e43 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:19:14 +0000 Subject: [PATCH 4/8] chore(spec): classify query/search (now a union leaf) and query/searchFields in the liveness ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The search-contract repair turned `search` into a union the liveness walker does not descend, and declared `searchFields` — both landed UNCLASSIFIED on the freshly-seeded query ledger (#4286). `search` becomes a leaf entry (the object form's experimental flags stay audited by their own describe markers inside FullTextSearchSchema); `searchFields` gets its live entry pointing at the resolveSearchFields intersection and the ADR-0061 dogfood proof. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- packages/spec/liveness/query.json | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/packages/spec/liveness/query.json b/packages/spec/liveness/query.json index e417f36d4c..324796edbf 100644 --- a/packages/spec/liveness/query.json +++ b/packages/spec/liveness/query.json @@ -18,19 +18,16 @@ "evidence": "packages/plugins/driver-sql/src/sql-driver.ts:1330-1331 (applyFilters); every driver's find() applies it" }, "search": { - "note": "Container. `query` + `fields` are the executed surface (ADR-0061); the six engine-affordance flags (fuzzy/operator/boost/minScore/language/highlight) resolve `experimental` from their `[EXPERIMENTAL — not enforced]` describe markers (#4286 step 1 — declared search-engine affordances no executor receives).", - "children": { - "query": { - "status": "live", - "verifiedAt": "2026-07-31", - "evidence": "packages/objectql/src/engine.ts:2860-2882 (ADR-0061: expanded into a server-resolved cross-field $or via expandSearchToFilter)" - }, - "fields": { - "status": "live", - "verifiedAt": "2026-07-31", - "evidence": "packages/objectql/src/engine.ts:2867-2870 (scopes the ADR-0061 expansion to the named columns)" - } - } + "status": "live", + "verifiedAt": "2026-07-31", + "note": "Union since #3899's search-contract repair: the canonical bare query STRING (ADR-0061 D1 — what every surface sends and the dogfood HTTP proof pins) | the structured FullTextSearch form. A LEAF here, no longer a container: the walker does not descend into union members, so the object form's six engine-affordance flags (fuzzy/operator/boost/minScore/language/highlight) are audited where they are declared — FullTextSearchSchema's own `[EXPERIMENTAL — not enforced]` describe markers (#4286 step 1). The previously-listed children `query`/`fields` remain live at the same executor site.", + "evidence": "packages/objectql/src/engine.ts:2885-2911 (ADR-0061: string or object expanded into a server-resolved cross-field $or via expandSearchToFilter; object-form `fields` scopes the expansion); proof showcase-search.dogfood.test.ts" + }, + "searchFields": { + "status": "live", + "verifiedAt": "2026-07-31", + "note": "Formally declared by #3899 (ADR-0061 P1, 'formalize $searchFields'): the per-query narrowing override the executor has served since the ADR landed — server-intersected with the object's allowed searchable set, can only narrow, never widen. The schema-side declaration caught up with the executor when #3899's entry validation started rejecting bodies the schema did not describe.", + "evidence": "packages/objectql/src/engine.ts:2893-2898 (requestedFields hand-off) → packages/objectql/src/search-filter.ts resolveSearchFields (intersection with the allowed set); proof showcase-search.dogfood.test.ts + search-conformance.ledger.ts row search-fields-override" }, "orderBy": { "status": "live", From 8c9c63db17f08e2ff44538a0af4b787336e51271 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:31:25 +0000 Subject: [PATCH 5/8] fix(metadata-protocol): QUERY_AST_KEYS names searchFields, as its type demands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `QUERY_AST_KEYS` is typed `Record` precisely so a key added to the spec is a compile error here rather than silent drift at the REST boundary — and it did its job: declaring `searchFields` on QuerySchema turned the whole build red until the key was listed. The hand-maintained `'searchFields'` entry in RESERVED_LIST_QUERY_PARAMS goes with it. It was there as a "transport-only extra the AST does not name"; the AST names it now, so it arrives through the `QUERY_AST_KEYS` spread and the type-level pin covers it. Keeping both would have been the second source that list exists to avoid. No behavior change on either path: the GET normalizer already accepted `?searchFields` / `$searchFields` and the engine already read `ast.searchFields` (ADR-0061). metadata-protocol 122, objectql 1373, full build 71/71 green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- packages/metadata-protocol/src/protocol.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index fd8e0a79d8..7e893fd661 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -788,10 +788,10 @@ function mergeDroppedFieldEvents(events: DroppedFieldsEvent[]): DroppedFieldsEve * which is a behavior change to call out in that changeset. */ const QUERY_AST_KEYS: Readonly> = { - object: true, fields: true, where: true, search: true, orderBy: true, - limit: true, offset: true, top: true, cursor: true, joins: true, - aggregations: true, groupBy: true, having: true, windowFunctions: true, - distinct: true, expand: true, + object: true, fields: true, where: true, search: true, searchFields: true, + orderBy: true, limit: true, offset: true, top: true, cursor: true, + joins: true, aggregations: true, groupBy: true, having: true, + windowFunctions: true, distinct: true, expand: true, }; /** @@ -819,7 +819,10 @@ const RESERVED_LIST_QUERY_PARAMS: ReadonlySet = new Set([ ...Object.keys(QUERY_AST_KEYS), // Transport-only extras the normalizer consumes but the AST does not name. 'count', // ?count / $count — response flag, not a projection - 'searchFields', // ?searchFields / $searchFields — ADR-0061 override + // `searchFields` used to be listed here as such an extra. It is a named + // AST key since #3899 declared it (ADR-0061 P1), so it now arrives through + // the spread above and the type-level pin covers it — the hand-maintained + // copy would have been a second source that could silently fall out of step. // Server-derived, never caller input (stripped then re-set from `request`). 'context', ]); From 7d4313e55f887841852a8a8812789d3e043bc16e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:33:36 +0000 Subject: [PATCH 6/8] docs(query): the search contract's canonical spelling appears in the hand-written docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both hand-written query pages taught only the structured `search: {query, fields}` form — `query-syntax.mdx` even typed it `search?: FullTextSearch`, which is now literally wrong. The bare string is the canonical ADR-0061 spelling (D1: the client says what to search for, the server decides which fields), it is what every surface sends, and it is what the dogfood proof asserts; the top-level `searchFields` narrowing was undocumented entirely. Same gap this PR exists to close, one layer up: a reader following these pages learned half the contract. Both forms are now shown, with the structured one named as equivalent for the two members that drive the expansion. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- content/docs/data-modeling/queries.mdx | 19 ++++++++++-- .../docs/protocol/objectql/query-syntax.mdx | 31 +++++++++++++------ 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/content/docs/data-modeling/queries.mdx b/content/docs/data-modeling/queries.mdx index 582f18b5c7..c30a3f2de9 100644 --- a/content/docs/data-modeling/queries.mdx +++ b/content/docs/data-modeling/queries.mdx @@ -419,6 +419,21 @@ dotted `fields` path (`'customer.name'`) for a single related column: ## Full-Text Search +`search` takes the query text; the server resolves which fields to search from object +metadata (ADR-0061). Pass `searchFields` to narrow that set — it is intersected with what +the object allows, so it can only narrow, never widen. + +```typescript +{ + object: 'article', + search: 'kubernetes deployment', + searchFields: ['title', 'body', 'tags'] // optional +} +``` + +The structured form is equivalent and carries the experimental knobs below — `query` and +`fields` mean exactly what `search` and `searchFields` do: + ```typescript { object: 'article', @@ -431,8 +446,8 @@ dotted `fields` path (`'customer.name'`) for a single related column: | Property | Type | Description | |:---|:---|:---| -| `query` | `string` | Search text | -| `fields` | `string[]` | Fields to search (optional — defaults to all searchable) | +| `query` | `string` | Search text (the bare `search: '…'` string form) | +| `fields` | `string[]` | Fields to search (optional — defaults to all searchable; the top-level spelling is `searchFields`) | | `fuzzy` | `boolean` | `[EXPERIMENTAL — not enforced]` Fuzzy matching for typo tolerance | | `operator` | `'and' \| 'or'` | `[EXPERIMENTAL — not enforced]` How to combine search terms | | `boost` | `Record` | `[EXPERIMENTAL — not enforced]` Field relevance weights | diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index fa97ae763d..1c8786d48d 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -64,7 +64,8 @@ interface QueryAST { object: string; // Target object (required) fields?: FieldNode[]; // Projection (SELECT) — field names where?: FilterCondition; // Filtering (WHERE) — MongoDB-style $op - search?: FullTextSearch; // Full-text search ($search) + search?: string | FullTextSearch; // Full-text search — the query text (canonical), or the structured form + searchFields?: string[]; // Narrow the search (server-intersected — narrows only, never widens) orderBy?: SortNode[]; // Ordering (ORDER BY) limit?: number; // Max records (LIMIT) offset?: number; // Skip records (OFFSET) @@ -738,21 +739,33 @@ an `$or` of `$contains` predicates across the object's server-resolved searchabl already runs `$or`/`$contains`, so no driver support is needed (`SqlDriver` reports `supports.fullTextSearch: false`). +`search` takes the **query text itself** — that is the canonical spelling (ADR-0061 D1: +the client says *what* to search for, the server decides *which fields*), and it is what +every surface sends. The structured form is equivalent for the two members that drive +the expansion, and carries the experimental knobs below: + ```typescript +// Canonical — the server resolves the fields from object metadata const query: QueryAST = { object: 'article', - search: { - query: 'ObjectStack tutorial', - fields: ['title', 'content'], - }, + search: 'ObjectStack tutorial', + searchFields: ['title', 'content'], // optional narrowing + limit: 10, +}; + +// Structured form — `query` + `fields` mean exactly the same thing +const structured: QueryAST = { + object: 'article', + search: { query: 'ObjectStack tutorial', fields: ['title', 'content'] }, limit: 10, }; ``` -Field resolution is server-side and never client-trusted: `search.fields` is -**intersected** with the object's declared `searchableFields` (or, absent those, an -auto-default of the name field plus short-text/enum fields), so naming a field outside -that set does not widen the search. Multiple whitespace-separated terms are AND-ed and +Field resolution is server-side and never client-trusted: the requested fields +(`searchFields`, or `search.fields` in the structured form) are **intersected** with the +object's declared `searchableFields` (or, absent those, an auto-default of the name field +plus short-text/enum fields), so naming a field outside that set does not widen the +search. Multiple whitespace-separated terms are AND-ed and fields are OR-ed. Case sensitivity is the **driver's**, not the expansion's: the expansion emits a plain `$contains`, which `SqlDriver` compiles to a parameterised `LIKE '%…%'` with no case folding — so the dialect's own `LIKE`/collation rules decide — From 354caba4639dc117c5da1b65182fb2011c97d2c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:25:31 +0000 Subject: [PATCH 7/8] test(driver-mongodb): a hung MongoDB binary download skips the suite instead of stopping Test Core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These three suites start a real mongod via mongodb-memory-server, which fetches a ~123 MB binary from fastdl.mongodb.org at module load. Each already anticipated that fetch FAILING — try/catch, warn, skip — precisely so a blocked download costs a skipped suite rather than the monorepo test job. A catch cannot express the other half. When the fetch HANGS, the top-level await never settles: the suite neither runs nor skips, it stops. Observed twice on this branch (Test Core attempts 1 and 2 of run 30610017310), both frozen at the identical point with zero output from this package and no skip warning, both force-killed 10 minutes later by the #4250 stall guard with `@objectstack/driver-mongodb#test` as the surviving task. `instance .launchTimeout` does not cover it — that bounds spawning mongod once the binary is on disk, a later phase than the fetch. `createTestMongod` puts a 120s deadline on the wait so a hang lands in the skip branch the suites already had. Not a workaround for this PR's diff: the same task on main 40 minutes earlier logged `Downloading MongoDB "8.2.6": 0% … 100%` and passed on a cache miss, so this is the network changing under an unbounded wait. Any PR touching @objectstack/spec invalidates the cache and runs straight into it. Verified: 160/160 still pass locally on the success path (binary cached), and a mocked never-settling create() resolves to undefined at the deadline rather than hanging. The helper stays out of dist — tsup entry is src/index.ts only, which does not reference it (mongodb-memory-server is a devDependency). Refs #4250. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- .../src/mongodb-datetime-storage.test.ts | 13 +-- .../driver-mongodb/src/mongodb-driver.test.ts | 26 ++---- .../src/mongodb-temporal-conformance.test.ts | 13 +-- .../plugins/driver-mongodb/src/test-mongod.ts | 82 +++++++++++++++++++ 4 files changed, 97 insertions(+), 37 deletions(-) create mode 100644 packages/plugins/driver-mongodb/src/test-mongod.ts diff --git a/packages/plugins/driver-mongodb/src/mongodb-datetime-storage.test.ts b/packages/plugins/driver-mongodb/src/mongodb-datetime-storage.test.ts index 2d1e9484f2..8b4a6289e0 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-datetime-storage.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-datetime-storage.test.ts @@ -20,18 +20,11 @@ */ import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest'; -import { MongoMemoryServer } from 'mongodb-memory-server'; +import type { MongoMemoryServer } from 'mongodb-memory-server'; import { MongoDBDriver } from './mongodb-driver.js'; +import { createTestMongod } from './test-mongod.js'; -let sharedMongod: MongoMemoryServer | undefined; -try { - sharedMongod = await MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } }); -} catch (err) { - console.warn( - '[driver-mongodb] Skipping datetime-storage suite — mongodb-memory-server could not start: ' + - `${(err as Error)?.message ?? String(err)}`, - ); -} +const sharedMongod: MongoMemoryServer | undefined = await createTestMongod('datetime-storage'); const ids = (rows: any[]) => rows.map((r: any) => r.id).sort(); diff --git a/packages/plugins/driver-mongodb/src/mongodb-driver.test.ts b/packages/plugins/driver-mongodb/src/mongodb-driver.test.ts index cc33b1e299..a2ffb5079b 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-driver.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-driver.test.ts @@ -1,27 +1,19 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest'; -import { MongoMemoryServer } from 'mongodb-memory-server'; +import type { MongoMemoryServer } from 'mongodb-memory-server'; import { MongoDBDriver } from './mongodb-driver.js'; +import { createTestMongod } from './test-mongod.js'; // `mongodb-memory-server` downloads a real MongoDB binary from // fastdl.mongodb.org on first use. In sandboxed / offline CI that download can -// fail; when it does we **skip** this suite rather than failing the whole -// package's test run (and, with it, the monorepo `Test Core` job). The startup -// is attempted once here so availability is known at collection time — a -// throwing `beforeAll` would *fail* every test instead of skipping it. -let sharedMongod: MongoMemoryServer | undefined; -try { - sharedMongod = await MongoMemoryServer.create({ - instance: { launchTimeout: 60_000 }, - }); -} catch (err) { - // eslint-disable-next-line no-console - console.warn( - '[driver-mongodb] Skipping MongoDBDriver suite — mongodb-memory-server could not start ' + - `(MongoDB binary unavailable / download blocked): ${(err as Error)?.message ?? String(err)}`, - ); -} +// fail — or HANG, which this suite could not express until `createTestMongod` +// put a deadline on the wait (see that module); both now land on the same skip. +// When it does we **skip** this suite rather than failing the whole package's +// test run (and, with it, the monorepo `Test Core` job). The startup is +// attempted once here so availability is known at collection time — a throwing +// `beforeAll` would *fail* every test instead of skipping it. +const sharedMongod: MongoMemoryServer | undefined = await createTestMongod('MongoDBDriver'); describe.skipIf(!sharedMongod)('MongoDBDriver', () => { const mongod = sharedMongod as MongoMemoryServer; diff --git a/packages/plugins/driver-mongodb/src/mongodb-temporal-conformance.test.ts b/packages/plugins/driver-mongodb/src/mongodb-temporal-conformance.test.ts index 5cd05bf8fc..b037fadb28 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-temporal-conformance.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-temporal-conformance.test.ts @@ -19,7 +19,7 @@ */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { MongoMemoryServer } from 'mongodb-memory-server'; +import type { MongoMemoryServer } from 'mongodb-memory-server'; import { TEMPORAL_CASES, TEMPORAL_NOW, @@ -29,19 +29,12 @@ import { } from '@objectstack/spec/data'; import { resolveFilterTokens } from '@objectstack/core'; import { MongoDBDriver } from './mongodb-driver.js'; +import { createTestMongod } from './test-mongod.js'; const resolveTokens = (filter: T): T => resolveFilterTokens(filter, { now: new Date(TEMPORAL_NOW) }); -let sharedMongod: MongoMemoryServer | undefined; -try { - sharedMongod = await MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } }); -} catch (err) { - console.warn( - '[driver-mongodb] Skipping temporal conformance — mongodb-memory-server could not start: ' + - `${(err as Error)?.message ?? String(err)}`, - ); -} +const sharedMongod: MongoMemoryServer | undefined = await createTestMongod('temporal conformance'); describe.skipIf(!sharedMongod)('driver-mongodb — temporal conformance', () => { const mongod = sharedMongod as MongoMemoryServer; diff --git a/packages/plugins/driver-mongodb/src/test-mongod.ts b/packages/plugins/driver-mongodb/src/test-mongod.ts new file mode 100644 index 0000000000..cbbb7a97da --- /dev/null +++ b/packages/plugins/driver-mongodb/src/test-mongod.ts @@ -0,0 +1,82 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * One bounded way to obtain a `MongoMemoryServer` for this package's suites. + * + * `mongodb-memory-server` downloads a ~123 MB MongoDB binary from + * fastdl.mongodb.org the first time it runs on a machine, at MODULE LOAD time + * (these suites need the server before `describe.skipIf` can decide anything). + * The three suites already anticipated that download FAILING: each wrapped the + * call in try/catch and skipped itself, so a blocked download costs a skipped + * suite rather than a red `Test Core` — the stated intent in + * `mongodb-driver.test.ts`'s header. + * + * A catch cannot express the other half. When the download HANGS — socket open, + * no bytes, no error — the top-level `await` never settles, so the suite neither + * runs nor skips: it stops, silently, and takes the whole monorepo test job with + * it. Observed twice on one branch (PR #4322): zero output from this package, no + * skip warning, and `Test Core` force-killed 10 minutes later by the #4250 stall + * guard with `@objectstack/driver-mongodb#test` as the surviving task. The same + * task on `main` 40 minutes earlier logged `Downloading MongoDB "8.2.6": 0% … + * 100%` and passed, so this is the network changing under an unbounded wait, not + * a code change. + * + * `instance.launchTimeout` does NOT cover it: that bounds spawning `mongod` + * once the binary is on disk, which is a later phase than the fetch. + * + * So the wait gets a deadline, and a timeout lands in the branch the suites + * already had. The degraded outcome is unchanged and still visible — a warning + * plus a skipped suite — but it is now reached in bounded time instead of never. + * Real coverage of a real mongod is preserved wherever the binary is reachable + * (every developer machine, and CI whenever the download works), which is the + * property that makes skipping acceptable at all. + */ + +import { MongoMemoryServer } from 'mongodb-memory-server'; + +/** + * How long to wait for the binary fetch + first launch before giving up. + * + * Generous against a slow-but-working download (the observed healthy case takes + * seconds; this allows two orders of magnitude more) and still far inside the + * stall guard's 10-minute silence budget, so a hung fetch is reported by THIS + * module — naming the cause — rather than by a guard that can only say the job + * stopped producing output. + */ +const ACQUIRE_TIMEOUT_MS = 120_000; + +/** + * A started `MongoMemoryServer`, or `undefined` when one could not be obtained + * within {@link ACQUIRE_TIMEOUT_MS} — for any reason: download blocked, download + * hung, or launch refused. Callers gate with `describe.skipIf(!mongod)`. + * + * @param suite - Suite name for the warning, e.g. `'MongoDBDriver'`. + */ +export async function createTestMongod(suite: string): Promise { + let timer: ReturnType | undefined; + try { + const started = MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } }); + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error( + `timed out after ${ACQUIRE_TIMEOUT_MS / 1000}s waiting for the MongoDB binary ` + + '(fastdl.mongodb.org unreachable or hanging)', + )), + ACQUIRE_TIMEOUT_MS, + ); + }); + // Whichever settles first. On timeout the underlying `create()` promise + // is abandoned rather than cancelled — the library exposes no cancel — + // but the process is already headed for a skipped suite and vitest tears + // the worker down at the end of the file. + return await Promise.race([started, timeout]); + } catch (err) { + console.warn( + `[driver-mongodb] Skipping ${suite} suite — mongodb-memory-server could not start ` + + `(MongoDB binary unavailable / download blocked or hanging): ${(err as Error)?.message ?? String(err)}`, + ); + return undefined; + } finally { + if (timer) clearTimeout(timer); + } +} From fabb96c2101e5d2865c7660bf92f2fdad9ab9a10 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 07:35:48 +0000 Subject: [PATCH 8/8] =?UTF-8?q?fix(driver-mongodb):=20declare=20Node's=20g?= =?UTF-8?q?lobals=20=E2=80=94=20the=20test=20helper=20is=20typechecked=20s?= =?UTF-8?q?ource?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/test-mongod.ts` uses setTimeout / clearTimeout / console and turned the package's `tsc --noEmit` red. The `.test.ts` files that call it have always used the same globals, but tsconfig excludes `**/*.test.ts`, so this package never needed `types: ["node"]` until a shared helper became typechecked source — and it cannot be named `*.test.ts`, or vitest would collect it as a suite with no tests. Same one-line declaration a dozen sibling packages already carry (client, core, lint, cli, metadata-protocol, …); @types/node is already a devDep here. I missed this locally by verifying with build + eslint + vitest: tsup's dts step only covers the `src/index.ts` entry graph, which this helper is deliberately outside of, so nothing I ran typechecked it. Re-verified with the command CI actually uses — `turbo run build --filter='./packages/*'`, 57/57 — plus `tsc --noEmit` in this package and its 160 tests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EynH7cngDczRGMkGMudJpC --- packages/plugins/driver-mongodb/tsconfig.json | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/plugins/driver-mongodb/tsconfig.json b/packages/plugins/driver-mongodb/tsconfig.json index 6fb7ea9970..1e6eb4ed14 100644 --- a/packages/plugins/driver-mongodb/tsconfig.json +++ b/packages/plugins/driver-mongodb/tsconfig.json @@ -2,7 +2,14 @@ "extends": "../../../tsconfig.json", "compilerOptions": { "outDir": "dist", - "rootDir": "src" + "rootDir": "src", + // `src/test-mongod.ts` uses `setTimeout` / `clearTimeout` / `console`. The + // `.test.ts` files that call it have always used them too, but they are + // excluded below, so this package never needed Node's globals declared + // until a shared test helper — which cannot be named `*.test.ts`, or vitest + // would collect it as a suite with no tests — became typechecked source. + // Same line a dozen sibling packages carry (client, core, lint, cli, …). + "types": ["node"] }, "include": ["src/**/*"], "exclude": ["node_modules", "dist", "**/*.test.ts"]