diff --git a/.changeset/request-body-schema-validation.md b/.changeset/request-body-schema-validation.md new file mode 100644 index 0000000000..6d537c9088 --- /dev/null +++ b/.changeset/request-body-schema-validation.md @@ -0,0 +1,31 @@ +--- +'@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. + +**`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. +- `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/content/docs/data-modeling/queries.mdx b/content/docs/data-modeling/queries.mdx index c19f79ab03..8e44f1bb15 100644 --- a/content/docs/data-modeling/queries.mdx +++ b/content/docs/data-modeling/queries.mdx @@ -432,6 +432,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', @@ -444,8 +459,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 e76fad45c6..2b8168a8e1 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) @@ -751,25 +752,36 @@ 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 can never widen the search — and over the REST/protocol ingress it is -`400 INVALID_FIELD` outright (#4254), because the engine-side intersection alone used to -drop the unknown name and fall back to scanning the full searchable set. Internal -callers reaching `engine.find()` directly keep the tolerant intersection. -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 can never widen the +search — and over the REST/protocol ingress it is `400 INVALID_FIELD` outright (#4254), +because the engine-side intersection alone used to drop the unknown name and fall back to +scanning the full searchable set. Internal callers reaching `engine.find()` directly keep +the tolerant intersection. 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 — 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 8d489137c2..7e3cfae753 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/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7476ab43b2..2898244a86 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -840,10 +840,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, }; /** @@ -882,7 +882,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', ]); 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); + } +} 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"] 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..88fde01faa --- /dev/null +++ b/packages/rest/src/request-schema-gate.conformance.test.ts @@ -0,0 +1,227 @@ +// 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); + } + } + }); + + 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, () => { + 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 d4279be74c..5d36d8ceb9 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -3968,9 +3968,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); @@ -4007,9 +4026,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); @@ -4059,10 +4105,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 } : {}), @@ -7173,11 +7241,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, @@ -7210,9 +7296,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/authorable-surface.json b/packages/spec/authorable-surface.json index bb8ea97615..036e34c53b 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -3751,6 +3751,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/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", 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 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)'),