From 7986982336088a1a22e755eac592d6d1657ab530 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:46:31 +0000 Subject: [PATCH 1/2] fix(data): sort / select / expand naming a missing field are rejected, not dropped (#4226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list path has four axes on which a caller names a field. `filter` was closed over #4134 / #4164 / #4181 / #4121; the other three still answered 200 when the name was wrong: sort=no_such_field -> 200 CAEBD identical to "no sort at all" select=no_such_field -> 200 asked for one column, got all expand=no_such_rel -> 200 no relation, no complaint Each is now refused in the shared normalizer, so the list route, the single-record route, `POST /data/:object/query`, the export route and the runtime dispatcher give one answer instead of five. - sort -> 400 INVALID_SORT. The row set is unchanged, so this is not #4181's "returned everything" — but sort + top is how a caller asks for "the latest N", and a dropped sort makes that an arbitrary N. INVALID_SORT had sat in the standard catalog with no emitter since it was written. - select -> 400 INVALID_FIELD. `engine.find()` drops unknown columns and then falls back to `*` when that empties the projection, so `?select=` asked for ONE column and received EVERY column: a parameter that exists to return less, failing by returning more. The partially-unknown case is refused on the same terms. - expand -> 400 INVALID_FIELD, with distinct messages for "no such field", "field holds no reference" and "reference field declares no target". Two sort spellings that were silently never applied now are: the client SDK's declared `orderBy: string[]`, and the `{field: direction}` map that the export route, `GET /data/import/jobs` and objectui's calendar emit. Both fell through the normalizer untouched and were then declined by every driver's `Array.isArray(orderBy)` guard, so the import-job history has served insertion order while asking for `created_at desc` since it was written. `$expand` of a `tree` field also works now: `REFERENCE_VALUE_TYPES` lists it among the types that expand, but `expandRelatedRecords` tested membership with a hand-copied `!==` chain that omitted it. The loop and the new expand gate both read the shared spec set, so the gate cannot admit a field the engine then skips. The engine's own tolerance is untouched — it guards internal callers (hooks, flows, expand sub-reads, registry-less hosts) that never pass through this ingress, the same tiering the object-existence and unknown-field gates use. A legacy array-shaped field map now disables all four gates rather than rejecting every real field name. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0158f3QC22Lf2VfJrT9nQ9om --- .../rest-list-sort-select-expand-rejected.md | 69 +++ content/docs/api/data-api.mdx | 47 +- content/docs/references/api/odata.mdx | 2 +- packages/metadata-protocol/src/protocol.ts | 408 +++++++++++-- packages/objectql/src/engine.ts | 22 +- packages/objectql/src/protocol-data.test.ts | 5 + .../src/query-expression-conformance.test.ts | 541 ++++++++++++++++++ packages/spec/src/api/odata.zod.ts | 2 +- 8 files changed, 1051 insertions(+), 45 deletions(-) create mode 100644 .changeset/rest-list-sort-select-expand-rejected.md create mode 100644 packages/objectql/src/query-expression-conformance.test.ts diff --git a/.changeset/rest-list-sort-select-expand-rejected.md b/.changeset/rest-list-sort-select-expand-rejected.md new file mode 100644 index 0000000000..011a64976c --- /dev/null +++ b/.changeset/rest-list-sort-select-expand-rejected.md @@ -0,0 +1,69 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/objectql": patch +--- + +fix(data): `sort` / `select` / `expand` naming a field that does not exist are rejected, not silently dropped (#4226) + +The list path has four axes on which a caller names a field. `filter` was +closed over #4134 / #4164 / #4181 / #4121 — a filter the server cannot apply is +now a 400, never a 200 over the wrong rows. The other three still leaked, all +answering `200`: + +``` +sort=no_such_field -> 200 CAEBD byte-identical to "no sort at all" +select=no_such_field -> 200 asked for one column, got all of them +expand=no_such_rel -> 200 no relation, no complaint +``` + +Each is now refused at the shared normalizer, so `GET /data/:object`, +`GET /data/:object/:id`, `POST /data/:object/query`, the export route and the +runtime dispatcher give one answer instead of five. + +- **`sort` → `400 INVALID_SORT`.** The row set is unchanged, so this is not + #4181's "returned everything" — it is worse in one specific way: `sort` + + `top` is how a caller asks for "the latest N", and a dropped sort makes that + an arbitrary N that nothing in the response reveals. This is the list half of + the bug #4181 fixed on the export route's `orderby`. `INVALID_SORT` had sat + in the standard catalog since it was written with no emitter. +- **`select` → `400 INVALID_FIELD`.** `engine.find()` drops unknown columns + (deliberate `SELECT *` tolerance) and then falls back to `*` when that empties + the projection, and the two compose into `?select=` asking for ONE + column and receiving EVERY column — a parameter whose purpose is to return + less, failing by returning more, against both FLS and data minimisation. The + partially-unknown case (`?select=title,no_such`) is refused on the same terms: + half a projection is not the one that was asked for, and the tolerant reading + would have to explain why `?status=` is a 400 and `?select=` is + not, on one endpoint, about one field map. +- **`expand` → `400 INVALID_FIELD`.** The lightest of the three — same rows, + same columns, the relation simply is not there — but the response cannot be + told apart from "every foreign key is null", and the client renders raw ids + where names belong. A name that is no field at all and a name that is a field + holding no reference (`?expand=title`) get different messages, since the fixes + differ. + +**Sorts that were silently never applied now are.** Two wire spellings reached +the normalizer and fell through it untouched, and every driver then declined +them (`SqlDriver` guards its ORDER BY with `Array.isArray(orderBy)`): the +client SDK's own declared `orderBy: string[]`, and the `{field: direction}` map +that `GET /data/:object/export`, `GET /data/import/jobs` and objectui's calendar +all emit. Both are now folded to `SortNode[]` — so the import-job history, which +has asked for `created_at desc` since it was written and served insertion order, +sorts. A sort shape that still cannot be read (a number, an entry naming no +field, a direction that is neither `asc` nor `desc`) is `400 INVALID_SORT` +rather than a silent no-op. + +**`$expand` of a `tree` field works.** `REFERENCE_VALUE_TYPES` lists `tree` +among the types whose value "points at another record … the related record +object in expanded form", and objectui requests it, but +`engine.expandRelatedRecords` tested membership with a hand-copied `!==` chain +that omitted it — so a hierarchy field came back as a raw parent id. The loop +now reads the shared spec set, which is also what the new expand gate validates +against, so the gate cannot admit a field the engine then skips. + +**What changes for callers:** requests naming a non-existent field in `sort`, +`select` or `expand` now fail loudly instead of receiving an unsorted, widened +or unexpanded response. Every axis naming real fields is unaffected. The +engine's own tolerance is untouched — it guards internal callers (hooks, flows, +expand sub-reads, registry-less hosts) that never pass through this ingress, +the same tiering the object-existence and unknown-field gates already use. diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index 0ddec579b6..1f2a6d9a27 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -18,12 +18,12 @@ Query records with filtering, sorting, selection, and pagination. | Parameter | Location | Description | |:----------|:---------|:------------| | `object` | path | Object name | -| `select` | query | Comma-separated field names | +| `select` | query | Comma-separated field names. Every name must exist — an unknown one is `400 INVALID_FIELD`, never dropped. | | `filter` | query | Filter expression (JSON). `filters` also accepted for backward compatibility. Malformed JSON is rejected with `400 INVALID_FILTER` — never ignored. | -| `sort` | query | Sort expression (e.g. `name asc` or `-created_at`) | +| `sort` | query | Sort expression (e.g. `name asc` or `-created_at`). Must name a real field — otherwise `400 INVALID_SORT`. | | `top` | query | Max records to return. No default — omitting it returns all matching records. | | `skip` | query | Offset | -| `expand` | query | Comma-separated list of relations to eager-load | +| `expand` | query | Comma-separated list of relations to eager-load. Must name a reference field (`lookup` / `master_detail` / `user` / `tree`) — otherwise `400 INVALID_FIELD`. | | `search` | query | Full-text search query | > **Note:** OData-style `$`-prefixed parameters (`$filter`, `$select`, `$orderby`, `$top`, `$skip`, `$expand`, `$count`, `$search`) are also accepted directly on this same endpoint as aliases — they're normalized internally to the parameter names above. There is no separate standalone OData endpoint. @@ -78,6 +78,43 @@ successful query: The same rule applies to `orderby` on `GET /data/:object/export`. +#### Nor is a sort, a projection, or an expansion + +`filter` is not the only parameter that names a field. `sort`, `select` and +`expand` do too, and each one used to be dropped in silence when the name was +wrong — three more responses that looked exactly like successful ones: + +| Request | Result | +|:---|:---| +| `?sort=-created_at` | sorts | +| `?sort=no_such_field` | `400 INVALID_SORT` | +| `?sort={oops` / `?sort=title:desc` | `400 INVALID_SORT` — the list route spells a direction with a space (`title desc`) or a leading `-` | +| `?select=id,title` | projects those two columns | +| `?select=no_such_field`, `?select=title,no_such_field` | `400 INVALID_FIELD` | +| `?expand=owner_id` | expands the reference | +| `?expand=no_such_rel` | `400 INVALID_FIELD` — no such field | +| `?expand=title` | `400 INVALID_FIELD` — real field, but it holds no reference | + +Why each one matters, since none of them changes which rows match: + +- **`sort`** — `sort` + `top` is how you ask for "the latest N". A sort that is + dropped turns that into an **arbitrary** N, and nothing in the response says so. +- **`select`** — an unknown column used to be dropped, and a projection left with + no known column fell back to *every* column: a parameter that exists to return + **less** failed by returning **more**. +- **`expand`** — an unexpanded relation is indistinguishable from one whose + foreign keys are all null, so clients render raw ids where names belong. + +Sorts accept any of these spellings, all equivalent: +`?sort=-created_at`, `?$orderby=-created_at`, and — on +`POST /data/:object/query` — `{"orderBy": [{"field": "created_at", "order": +"desc"}]}`, `{"orderBy": ["-created_at"]}` or `{"orderBy": {"created_at": +"desc"}}`. A shape that is none of these (a number, an entry naming no field, +a direction that is neither `asc` nor `desc`) is `400 INVALID_SORT`. + +`GET /data/:object/:id` applies the same `select` and `expand` rules, so the +list and single-record routes cannot disagree about one field map. + **Response**: ```json { @@ -96,8 +133,8 @@ Get a single record by ID. Only `select` and `expand` query parameters are allow |:----------|:---------|:------------| | `object` | path | Object name | | `id` | path | Record ID | -| `select` | query | Comma-separated field names to include | -| `expand` | query | Comma-separated list of relations to eager-load | +| `select` | query | Comma-separated field names to include. Unknown name → `400 INVALID_FIELD`. | +| `expand` | query | Comma-separated list of relations to eager-load. Not a reference field → `400 INVALID_FIELD`. | **Response**: `{ object: "account", id: "1", record: { ... } }` diff --git a/content/docs/references/api/odata.mdx b/content/docs/references/api/odata.mdx index 1ce9e0f576..40811154ae 100644 --- a/content/docs/references/api/odata.mdx +++ b/content/docs/references/api/odata.mdx @@ -205,7 +205,7 @@ const result = ODataConfig.parse(data); | **$orderby** | `string \| string[]` | optional | Sort order | | **$top** | `integer` | optional | Max results to return | | **$skip** | `integer` | optional | Results to skip | -| **$expand** | `string \| string[]` | optional | Navigation properties to expand (lookup/master_detail fields) | +| **$expand** | `string \| string[]` | optional | Navigation properties to expand (reference fields: lookup/master_detail/user/tree) | | **$count** | `boolean` | optional | Include total count | | **$search** | `string` | optional | Search expression | | **$format** | `Enum<'json' \| 'xml' \| 'atom'>` | optional | Response format | diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e6e7dc2066..27df5c9de9 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -18,7 +18,7 @@ import type { } from '@objectstack/spec/api'; import type { MetadataCacheRequest, MetadataCacheResponse, ServiceInfo, ApiRoutes, WellKnownCapabilities } from '@objectstack/spec/api'; import { readServiceSelfInfo } from '@objectstack/spec/api'; -import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; +import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, REFERENCE_VALUE_TYPES, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared'; import { type FormView, isAggregatedViewContainer } from '@objectstack/spec/ui'; import { METADATA_FORM_REGISTRY, CORE_SERVICE_PROVIDER, serviceUnavailableMessage } from '@objectstack/spec/system'; @@ -882,6 +882,118 @@ function unusableFilterError(param: string, detail: string): Error { return err; } +/** + * [#4226] A sort the normalizer cannot turn into a usable `SortNode[]`, or one + * that names a field the object does not have. + * + * Carries `INVALID_SORT` — the standard-catalog code (`errors.zod.ts`, + * "Invalid sort specification") that had sat in the catalog with no emitter + * since it was written. One condition, one wire code, however the caller + * reached it. + * + * The message spells out what an unapplied sort costs, because that is the part + * a caller cannot infer from the response: `sort` + `top` is how you ask for + * "the latest N", and a dropped sort turns that into an ARBITRARY N with a + * perfectly ordinary-looking 200 over it. The rows are all there and all real — + * which is exactly why nobody notices. + */ +function invalidSortError( + param: string, + detail: string, + opts?: { hint?: string, extra?: Record }, +): Error { + const err: any = new Error( + `Query parameter '${param}' ${detail}. It was not applied, and an unapplied ` + + "sort returns the rows in an arbitrary order — which 'top'/'limit' then " + + 'slices into an arbitrary page.' + + (opts?.hint ?? ''), + ); + err.status = 400; + err.code = 'INVALID_SORT'; + err.param = param; + Object.assign(err, opts?.extra ?? {}); + return err; +} + +/** + * [#4226] Every wire spelling of `sort`/`orderBy`, folded to the one shape the + * QueryAST declares (`SortNodeSchema[]`) — or a rejection. + * + * Four spellings arrive here today and only two of them ever reached a driver: + * + * | spelling | example | before | + * |:---|:---|:---| + * | string | `?sort=-created_at` | worked | + * | `SortNode[]` | `[{field,order}]` | worked | + * | `string[]` | `['-created_at']` | **dropped** — the client's own declared type (`orderBy?: string \| string[] \| SortNode[]`) | + * | `Record` | `{created_at:'desc'}` | **dropped** — what `GET /data/:object/export`, `GET /data/import/jobs` and objectui's calendar all emit | + * + * The two dropped ones never failed: `SqlDriver` guards its ORDER BY clause + * with `Array.isArray(query.orderBy)`, so a shape it could not read produced no + * clause at all. `GET /data/import/jobs` has been asking for `created_at desc` + * and serving insertion order ever since it was written; #4181 taught the + * export route to REJECT an unparseable `orderby`, but the parsed result then + * fell into this same hole one layer down. + * + * Normalizing here — in the one shared normalizer every ingress funnels through + * — is what gives `GET /data/:object`, `POST /data/:object/query`, the export + * route and the runtime dispatcher a single answer instead of four. Anything + * that still cannot be read as a sort is a 400 rather than a silent no-op: per + * #3948, an unapplied sort must not look like an applied one. + */ +function normalizeSortNodes(value: unknown, param: string): Array<{ field: string, order: 'asc' | 'desc' }> { + const direction = (raw: unknown, subject: string): 'asc' | 'desc' => { + if (raw === undefined || raw === null || raw === '') return 'asc'; + const dir = String(raw).trim().toLowerCase(); + if (dir === 'asc' || dir === 'desc') return dir; + throw invalidSortError(param, `gives ${subject} the direction '${raw}', which is neither 'asc' nor 'desc'`); + }; + // `-field` / `field` / `field desc` — the querystring shorthand, also used + // for each element of the `string[]` form. + const fromShorthand = (raw: string): { field: string, order: 'asc' | 'desc' } | undefined => { + const trimmed = raw.trim(); + if (!trimmed) return undefined; + if (trimmed.startsWith('-')) { + const field = trimmed.slice(1).trim(); + return field ? { field, order: 'desc' } : undefined; + } + const [field, order] = trimmed.split(/\s+/); + return field ? { field, order: direction(order, `'${field}'`) } : undefined; + }; + const fromElement = (el: unknown, index: number): { field: string, order: 'asc' | 'desc' } | undefined => { + if (typeof el === 'string') return fromShorthand(el); + if (el && typeof el === 'object' && !Array.isArray(el)) { + const node = el as { field?: unknown, order?: unknown }; + if (typeof node.field === 'string' && node.field.trim()) { + return { field: node.field.trim(), order: direction(node.order, `'${node.field}'`) }; + } + } + throw invalidSortError( + param, + `has an entry at position ${index} that names no field (received ${JSON.stringify(el) ?? typeof el})`, + ); + }; + + if (value === undefined || value === null || value === '') return []; + if (typeof value === 'string') { + return value.split(',').map(fromShorthand).filter((s): s is { field: string, order: 'asc' | 'desc' } => !!s); + } + if (Array.isArray(value)) { + return value.map(fromElement).filter((s): s is { field: string, order: 'asc' | 'desc' } => !!s); + } + if (typeof value === 'object') { + const entries = Object.entries(value as Record); + if (entries.length === 0) return []; + // A single `SortNode` sent unwrapped (`{field:'x',order:'desc'}`) is the + // one-element array it obviously means; anything else is read as the + // `{field: direction}` map, whose VALUES must be directions — that check + // is what stops a stray object being silently accepted as "no sort". + if (typeof (value as any).field === 'string') return [fromElement(value, 0)!]; + return entries.map(([field, dir]) => ({ field, order: direction(dir, `'${field}'`) })); + } + throw invalidSortError(param, `is a ${typeof value}, which names no field to sort by`); +} + /** Fold a parameter name to its near-miss lookup key. */ function nearMissKey(name: string): string { return name.toLowerCase().replace(/[_-]/g, ''); @@ -919,7 +1031,19 @@ function suggestQueryParam(param: string, knownFields: readonly string[]): strin + (odata ? ` (OData spelling '${odata}')` : '') + '?'; } - const folded = nearMissKey(param); + return suggestFieldName(param, knownFields); +} + +/** + * [#4226] The field-typo half of {@link suggestQueryParam}, on its own. + * + * The `sort` / `select` / `expand` axes name a field DIRECTLY, so the + * parameter-dialect half above is not merely useless there but actively wrong: + * it would answer `?sort=order` with "did you mean the 'sort' query parameter", + * which is the parameter the caller already used. + */ +function suggestFieldName(name: string, knownFields: readonly string[]): string { + const folded = nearMissKey(name); // Only worth guessing for names long enough that a small distance is // meaningful — at 3 chars everything is within 2 edits of everything. if (folded.length >= 4) { @@ -3028,39 +3152,64 @@ export class ObjectStackProtocolImplementation implements } /** - * [#4134] Read-path unknown-field gate for the implicit filters `findData` - * derives from leftover query parameters. + * [#4134] The names a list query may legitimately use on `object`, or + * `null` when nothing authoritative is available to check against. * - * Rejects with the SAME envelope the write path produces for the same - * mistake — `400 INVALID_FIELD` + `field` + `object` (see `mapDataError` in - * `@objectstack/rest`) — so "does this field exist" has one answer on both - * sides of the API instead of being enforced on write and silently - * zeroed on read. + * ONE resolution shared by all four read axes — filter (#4134), sort, + * projection and expand (#4226) — because "does this field exist" is one + * question and four answers to it is exactly the state these issues were + * filed about. It is also the read half of a question the WRITE path + * already answers loudly (`400 INVALID_FIELD` via `mapDataError` in + * `@objectstack/rest`). * * Tiering mirrors {@link assertObjectRegistered}, one level down: * * - **Schema present with a field map → authoritative.** An unlisted name * is a 400. The registry injects the audit/tenant/owner columns * (`created_at`, `created_by`, `updated_at`, `updated_by`, - * `organization_id`, `owner_id`) into `fields`, so those filter normally; - * `id` is added here because it is the primary key rather than a declared - * field. Dotted paths are judged on their head segment only - * (`owner_id.name`), matching how `engine.find()` validates projections. + * `organization_id`, `owner_id`) into `fields`, so those are usable + * normally; `id` and the two audit timestamps are added defensively + * because they are primary-key/engine-assigned rather than declared, and + * `engine.find()` admits the same three unconditionally — a gate stricter + * than the engine it guards would reject queries that used to work. * - **No registry, or a schema with no field map → skip.** Nothing to check * against (registry-less Lite/edge hosts, engine doubles, external * datasources whose columns are not mirrored locally). The * object-existence gate above already warns once when the registry itself * is missing, so this stays quiet rather than warning twice per process. */ - private assertQueryParamsAreFields(object: string, params: readonly string[]): void { + private resolveQueryFields(object: string): { known: ReadonlySet, declared: readonly string[], fields: any } | null { const schema: any = this.engine?.registry?.getObject?.(object); const declared = schema?.fields; - if (!declared || typeof declared !== 'object') return; + if (!declared || typeof declared !== 'object') return null; + // A legacy ARRAY field map is not checkable: `Object.keys` on it yields + // '0', '1', '2' …, so every gate below would reject every real field + // name. Skipping is the same call the no-field-map case makes — there + // is nothing here to answer "does this field exist" with. + if (Array.isArray(declared)) return null; const fieldNames = Object.keys(declared); - if (fieldNames.length === 0) return; + if (fieldNames.length === 0) return null; const known = new Set(fieldNames); known.add('id'); - const unknown = params.filter((p) => !known.has(String(p).split('.')[0])); + known.add('created_at'); + known.add('updated_at'); + return { known, declared: fieldNames, fields: declared }; + } + + /** + * [#4134] Read-path unknown-field gate for the implicit filters `findData` + * derives from leftover query parameters. + * + * Rejects with the SAME envelope the write path produces for the same + * mistake — `400 INVALID_FIELD` + `field` + `object` — so a name that + * cannot be written cannot be silently filtered on either. Dotted paths are + * judged on their head segment only (`owner_id.name`), matching how + * `engine.find()` validates projections. + */ + private assertQueryParamsAreFields(object: string, params: readonly string[]): void { + const gate = this.resolveQueryFields(object); + if (!gate) return; + const unknown = params.filter((p) => !gate.known.has(String(p).split('.')[0])); if (unknown.length === 0) return; const first = unknown[0]; const err: any = new Error( @@ -3068,7 +3217,7 @@ export class ObjectStackProtocolImplementation implements + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : '') + '. Query parameters that are not reserved are read as field filters, so an ' + 'unknown name can only match zero records.' - + suggestQueryParam(first, fieldNames), + + suggestQueryParam(first, gate.declared), ); err.code = 'INVALID_FIELD'; err.status = 400; @@ -3078,6 +3227,160 @@ export class ObjectStackProtocolImplementation implements throw err; } + /** + * [#4226] SORT axis. A sort naming a field the object does not have is + * refused (`400 INVALID_SORT`) instead of being dropped on the floor. + * + * The stakes sit between the other two axes: the row SET is unchanged, so + * this is not #4181's "returned everything" — but `sort` + `top` is how a + * caller asks for "the latest N", and a dropped sort makes that an + * arbitrary N that no amount of inspecting the response can reveal. + * `SqlDriver` has a deliberate backstop that drops an unknown ORDER BY + * column and returns the rows unordered (objectstack#3821 — rows matter + * more than their order); that backstop is for a *driver* that has already + * been handed the query. Refusing HERE, before it is handed over, is what + * keeps it from doubling as a silent tolerance at the API boundary. + * + * The colon form gets its own hint: `?sort=title:desc` is the spelling + * `GET /data/:object/export` accepts, and a caller who moved between the + * two routes deserves better than "no such field 'title:desc'". + */ + private assertSortFieldsExist(object: string, orderBy: ReadonlyArray<{ field: string }>, param: string): void { + if (orderBy.length === 0) return; + const gate = this.resolveQueryFields(object); + if (!gate) return; + const unknown = orderBy + .map((s) => String(s.field)) + .filter((f) => !gate.known.has(f.split('.')[0])); + if (unknown.length === 0) return; + const first = unknown[0]; + const hint = first.includes(':') + ? ` The list route spells a direction with a space or a leading '-'` + + ` ('sort=${first.split(':')[0]} desc', 'sort=-${first.split(':')[0]}');` + + " 'field:direction' is the export route's spelling." + : suggestFieldName(first, gate.declared); + throw invalidSortError( + param, + `sorts by '${first}', which is not a field on object '${object}'` + + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : ''), + { hint, extra: { field: first, fields: unknown, object } }, + ); + } + + /** + * [#4226] PROJECTION axis. A `select`/`fields` naming a column the object + * does not have is refused (`400 INVALID_FIELD`). + * + * This axis fails in the direction nobody expects. `engine.find()` drops + * unknown columns (deliberate `SELECT *` / OData tolerance) and then falls + * back to `*` when that leaves the projection empty (so the driver is not + * handed an empty SELECT list) — which compose into: `?select=` asked + * for ONE column and got EVERY column. A parameter whose entire purpose is + * to return less had "return more" as its failure mode, pointing away from + * both FLS and data minimisation. + * + * Rejecting the partially-unknown case too (`?select=title,no_such`) is the + * #3948 reading — an unapplied projection must not look like a satisfied + * one — and the same rule the filter axis already lives by. The tolerant + * reading (align with Salesforce/OData leniency) would have to explain why + * `?status=` is a 400 and `?select=` is not, on one endpoint, + * about the same field map. + * + * The engine's tolerance is untouched: it guards INTERNAL callers (hooks, + * flows, expand sub-reads, registry-less hosts) that never pass through + * this ingress, exactly like the object-existence gate above. + */ + private assertProjectionFieldsExist(object: string, fields: unknown, param: string): void { + if (!Array.isArray(fields) || fields.length === 0) return; + const gate = this.resolveQueryFields(object); + if (!gate) return; + const unknown = fields.map(String).filter((f) => !gate.known.has(f.split('.')[0])); + if (unknown.length === 0) return; + const first = unknown[0]; + const err: any = new Error( + `Unknown field '${first}' on object '${object}'` + + (unknown.length > 1 ? ` (also: ${unknown.slice(1).join(', ')})` : '') + + `. '${param}' chooses which fields to return; dropping an unknown one silently ` + + 'answered a NARROWER projection with a WIDER one — a projection naming no known ' + + 'field fell all the way back to every field.' + + suggestFieldName(first, gate.declared), + ); + err.code = 'INVALID_FIELD'; + err.status = 400; + err.field = first; + err.fields = unknown; + err.object = object; + err.param = param; + throw err; + } + + /** + * [#4226] EXPAND axis. An `expand`/`populate` naming something the engine + * cannot expand is refused (`400 INVALID_FIELD`). + * + * The lightest of the three — neither the row set nor the returned columns + * change, the relation simply is not there — but the response gives the + * caller nothing to distinguish "this relation does not exist" from "every + * row happens to have a null foreign key", and the client then renders raw + * ids where names belong. + * + * Two rejections, one code, different messages, because the fixes differ: + * a name that is no field at all is a typo, while a name that IS a field + * but holds no reference (`?expand=title`) is a misunderstanding of what + * expansion does. {@link REFERENCE_VALUE_TYPES} is the spec's own list of + * types whose value "points at another record … the related record object + * in expanded form" — the same set `engine.expandRelatedRecords` resolves, + * so this gate cannot drift from what expansion actually delivers. + */ + private assertExpandTargetsExist(object: string, names: readonly string[]): void { + if (names.length === 0) return; + const gate = this.resolveQueryFields(object); + if (!gate) return; + const unknown: string[] = []; + const notRelations: string[] = []; + const targetless: string[] = []; + for (const raw of names) { + const name = String(raw); + const def: any = gate.fields[name.split('.')[0]]; + if (!def) { unknown.push(name); continue; } + if (!REFERENCE_VALUE_TYPES.has(def.type)) { notRelations.push(name); continue; } + // A reference-typed field with no `reference` names no target + // object, so `expandRelatedRecords` has nothing to batch-load. That + // is an authoring bug on the OBJECT, not on the request, and saying + // "not a relationship" about a declared lookup would send the + // caller looking in the wrong place. + if (!def.reference) targetless.push(name); + } + const [offenders, reason] = + unknown.length > 0 ? [unknown, 'unknown' as const] + : notRelations.length > 0 ? [notRelations, 'not-a-reference' as const] + : [targetless, 'targetless' as const]; + if (offenders.length === 0) return; + const first = offenders[0]; + const err: any = new Error( + (reason === 'unknown' + ? `Unknown field '${first}' on object '${object}'` + : reason === 'not-a-reference' + ? `Field '${first}' on object '${object}' is not a relationship` + : `Field '${first}' on object '${object}' declares no target object`) + + (offenders.length > 1 ? ` (also: ${offenders.slice(1).join(', ')})` : '') + + '. \'expand\' resolves a reference field into the related record, so ' + + (reason === 'unknown' + ? 'a name the object does not declare can never be expanded.' + : reason === 'not-a-reference' + ? `only ${[...REFERENCE_VALUE_TYPES].join(' / ')} fields can be expanded.` + : "the field's `reference` must name the object it points at.") + + (reason === 'unknown' ? suggestFieldName(first, gate.declared) : ''), + ); + err.code = 'INVALID_FIELD'; + err.status = 400; + err.field = first; + err.fields = offenders; + err.object = object; + err.param = 'expand'; + throw err; + } + async findData(request: { object: string, query?: any, context?: any }) { // [#3770] Existence first: an unregistered object is a 404 before any // query parameter is even parsed, so an unknown name can never be @@ -3124,6 +3427,12 @@ export class ObjectStackProtocolImplementation implements // implicit-filter pass below and get merged into `where` as // bogus field-equality predicates (e.g. `where.$top = "2"`), // which silently returns zero rows for every list endpoint. + // + // [#4226] `wireSpelling` remembers which alias each rewritten slot + // arrived under, so a rejection quotes the parameter the caller + // actually wrote. Telling someone who sent `?$orderby=…` that + // "'orderBy' is invalid" names a parameter absent from their request. + const wireSpelling: Record = {}; for (const [dollar, bare] of [ ['$top', 'top'], ['$skip', 'skip'], @@ -3135,6 +3444,7 @@ export class ObjectStackProtocolImplementation implements ] as const) { if (options[dollar] != null && options[bare] == null) { options[bare] = options[dollar]; + wireSpelling[bare] = dollar; } delete options[dollar]; } @@ -3156,6 +3466,7 @@ export class ObjectStackProtocolImplementation implements if (options.offset != null) options.offset = Number(options.offset); // Select → fields: comma-separated string → array + const projectionKey = options.select !== undefined ? (wireSpelling.select ?? 'select') : 'fields'; if (typeof options.select === 'string') { options.fields = options.select.split(',').map((s: string) => s.trim()).filter(Boolean); } else if (Array.isArray(options.select)) { @@ -3172,23 +3483,38 @@ export class ObjectStackProtocolImplementation implements } else if (options.fields !== undefined && !Array.isArray(options.fields)) { delete options.fields; } + // [#4226] Unknown projection columns are refused rather than dropped — + // see `assertProjectionFieldsExist` for why this axis' silent failure + // returned MORE than was asked for. + this.assertProjectionFieldsExist(request.object, options.fields, projectionKey); - // Sort/orderBy → orderBy: string → SortNode[] array - const sortValue = options.orderBy ?? options.sort; - if (typeof sortValue === 'string') { - const parsed = sortValue.split(',').map((part: string) => { - const trimmed = part.trim(); - if (trimmed.startsWith('-')) { - return { field: trimmed.slice(1), order: 'desc' as const }; - } - const [field, order] = trimmed.split(/\s+/); - return { field, order: (order?.toLowerCase() === 'desc' ? 'desc' : 'asc') as 'asc' | 'desc' }; - }).filter((s: any) => s.field); - options.orderBy = parsed; - } else if (Array.isArray(sortValue)) { - options.orderBy = sortValue; - } + // Sort/orderBy → orderBy: every wire spelling → SortNode[]. + // + // [#4226] `normalizeSortNodes` folds the two shapes that used to fall + // through this block untouched — `string[]` and `{field: direction}` — + // and refuses the ones it cannot read. Before it, "not a string and not + // an array" simply skipped the branch, leaving a value on `orderBy` + // that `SqlDriver`'s `Array.isArray` guard then declined to turn into + // an ORDER BY clause: no sort, no error, no way to tell. + const usesOrderBy = options.orderBy !== undefined && options.orderBy !== null; + const sortValue = usesOrderBy ? options.orderBy : options.sort; + const sortKey = usesOrderBy ? (wireSpelling.orderBy ?? 'orderBy') : 'sort'; delete options.sort; + if (sortValue === undefined || sortValue === null) { + // Nothing to sort by — and an explicit `orderBy: null` must not ride + // to the engine as a value every driver quietly declines to read. + delete options.orderBy; + } else { + const orderBy = normalizeSortNodes(sortValue, sortKey); + // [#4226] Validated on the NORMALIZED nodes, so one gate covers + // every spelling — `?sort=no_such`, `['-no_such']`, `{no_such: + // 'desc'}` and `[{field:'no_such'}]` are one mistake with one + // answer. Assigned only after it passes, so a rejected sort cannot + // leave a half-applied one behind. + this.assertSortFieldsExist(request.object, orderBy, sortKey); + if (orderBy.length > 0) options.orderBy = orderBy; + else delete options.orderBy; + } // Filter/filters/$filter → where: normalize all filter aliases. // @@ -3320,6 +3646,13 @@ export class ObjectStackProtocolImplementation implements options.expand[rel] = { object: rel }; } } + // [#4226] Both routes into `expand` are gated: the comma-list spellings + // collected above, and the advanced `{rel: QueryAST}` map a caller may + // send directly on `POST /data/:object/query`. Validating the map's KEYS + // rather than `expandNames` is what covers the second one. + if (options.expand && typeof options.expand === 'object') { + this.assertExpandTargetsExist(request.object, Object.keys(options.expand)); + } // Boolean fields for (const key of ['distinct', 'count']) { @@ -3485,10 +3818,18 @@ export class ObjectStackProtocolImplementation implements } // Support fields for single-record retrieval + // + // [#4226] Gated exactly as on the list path. `GET /data/:object/:id` and + // `GET /data/:object` read the same `select`/`expand` against the same + // field map, so answering one with a 400 and the other with a silently + // widened projection would recreate, one route over, the very split + // ("two routes, opposite answers for one input") this issue was filed + // about. if (request.select) { queryOptions.fields = typeof request.select === 'string' ? request.select.split(',').map((s: string) => s.trim()).filter(Boolean) : request.select; + this.assertProjectionFieldsExist(request.object, queryOptions.fields, 'select'); } // Support expand for single-record retrieval @@ -3496,6 +3837,7 @@ export class ObjectStackProtocolImplementation implements const expandNames = typeof request.expand === 'string' ? request.expand.split(',').map((s: string) => s.trim()).filter(Boolean) : request.expand; + this.assertExpandTargetsExist(request.object, expandNames); queryOptions.expand = {} as Record; for (const rel of expandNames) { queryOptions.expand[rel] = { object: rel }; diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index ab7d117af5..3b90fbcfad 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -12,7 +12,7 @@ import { type DroppedFieldsEvent } from '@objectstack/spec/data'; import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; -import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, FILE_REFERENCE_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; +import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, FILE_REFERENCE_TYPES, REFERENCE_VALUE_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; import { DATA_MIGRATION_FLAG_OBJECT, FILE_REFERENCES_MIGRATION_ID, @@ -2455,11 +2455,23 @@ export class ObjectQL implements IDataEngine { for (const [fieldName, nestedAST] of Object.entries(expand)) { const fieldDef = objectSchema.fields[fieldName]; - // Skip if field not found or not a relationship type + // Skip if field not found or not a relationship type. + // + // `user` is a lookup specialized to sys_user and `tree` is a hierarchical + // self-reference — both carry the same `reference` + id storage, so both + // expand through this exact path (single or multiple). + // + // [#4226] The membership test is `REFERENCE_VALUE_TYPES` — the spec's own + // list of types whose value "points at another record … the related record + // object in expanded form" — rather than a hand-copied `!==` chain. The + // chain had drifted: it omitted `tree`, so a `$expand` of a hierarchy + // field returned the raw parent id and rendered as a bare placeholder, + // while `field-value.zod.ts` and objectui's `EXPANDABLE_FIELD_TYPES` both + // declared it expandable. Reading the shared set is what stops the + // protocol's expand gate (which validates against the same set) from ever + // admitting a field this loop then silently skips. if (!fieldDef || !fieldDef.reference) continue; - // `user` is a lookup specialized to sys_user — it carries the same `reference` - // and id storage, so it expands through this exact path (single or multiple). - if (fieldDef.type !== 'lookup' && fieldDef.type !== 'master_detail' && fieldDef.type !== 'user') continue; + if (!REFERENCE_VALUE_TYPES.has(fieldDef.type)) continue; const referenceObject = fieldDef.reference; diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index 80f4e9cabf..2ba76a05d8 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -746,6 +746,11 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { const bare = spelling.replace(/^\$/, '').toLowerCase(); if (bare === 'top' || bare === 'skip') return 1; if (bare === 'filter' || bare === 'filters') return { status: 'open' }; + // [#4226] `expand` needs a RELATIONSHIP, not merely a real + // field: expanding `title` is now a legitimate 400 for the same + // reason `filter: 'title'` is one — the parameter is accepted, + // that value for it is not. `owner_id` is this object's lookup. + if (bare === 'expand' || bare === 'populate') return 'owner_id'; return 'title'; }; for (const spelling of suggested) { diff --git a/packages/objectql/src/query-expression-conformance.test.ts b/packages/objectql/src/query-expression-conformance.test.ts new file mode 100644 index 0000000000..2b2e8cc4b1 --- /dev/null +++ b/packages/objectql/src/query-expression-conformance.test.ts @@ -0,0 +1,541 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4226 — the REST list path has FOUR axes on which a caller names a field, and + * only one of them used to answer when the name was wrong. + * + * `filter` was closed over four issues (#4134 / #4164 / #4181 / #4121): a bad + * filter is now a 400, never a 200 over the wrong rows. The same machine went + * on leaking on the other three — `sort`, `select` and `expand` pointed at a + * field that does not exist were silently not applied, and answered `200`: + * + * ``` + * sort=no_such_field -> 200 CAEBD byte-identical to "no sort at all" + * select=no_such_field -> 200 asked for one column, got all of them + * expand=no_such_rel -> 200 no relation, no complaint + * ``` + * + * Each fails differently and each is now refused: + * + * - **sort** — the row SET is unchanged, so this is not #4181's "returned + * everything". It is worse in one specific way: `sort` + `top` IS how a + * caller asks for "the latest N", so a dropped sort silently hands back an + * ARBITRARY N. `400 INVALID_SORT` — the standard-catalog code that had no + * emitter until now. + * - **select** — `engine.find()` drops unknown columns (deliberate `SELECT *` + * tolerance) and then falls back to `*` when that empties the projection, so + * `?select=` asked for ONE column and received EVERY column. A + * parameter whose whole purpose is to return less failed by returning more. + * `400 INVALID_FIELD`. + * - **expand** — lightest: same rows, same columns, the relation just is not + * there. But the response cannot be told apart from "every foreign key is + * null", and the client renders raw ids where names belong. + * `400 INVALID_FIELD`. + * + * Every axis here carries a CONTROL: a real field that demonstrably works. A + * conformance test that only asserts rejections passes just as happily when the + * feature is broken outright, and pins nothing. + * + * Driven against a REAL {@link ObjectQL} engine — not an engine double — + * because the authority these gates consult is the REGISTRY's field map, which + * is not what the author declared (`applySystemFields` injects the audit / + * tenant / owner columns at registration), and because expansion is engine + * work: only the real one can show `$expand` resolving a `tree` field. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; + +const projectObject = { + name: 'showcase_project', + label: 'Project', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +const taskObject = { + name: 'showcase_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const, required: true }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + project_id: { name: 'project_id', label: 'Project', type: 'lookup' as const, reference: 'showcase_project' }, + parent_id: { name: 'parent_id', label: 'Parent', type: 'tree' as const, reference: 'showcase_task' }, + }, +}; + +/** + * An in-memory driver that really sorts, really projects and really paginates. + * + * This matters more than usual here: a driver that ignored `orderBy` would make + * every "a real sort field works" control vacuously true, and the pins above it + * would then pass against a completely broken sort axis. + */ +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((arm) => matchesWhere(row, arm))) return false; + continue; + } + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.map(String).includes(String(row[k]))) return false; + continue; + } + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + const applySort = (rows: Record[], orderBy: any): Record[] => { + // Mirrors every real driver: an `orderBy` that is not a SortNode array + // produces NO ordering. That is the shape of the bug the sort axis had — + // reproduced faithfully here so the normalizer is what has to fix it. + if (!Array.isArray(orderBy) || orderBy.length === 0) return rows; + return [...rows].sort((x, y) => { + for (const node of orderBy) { + if (!node?.field) continue; + const a = x[node.field]; + const b = y[node.field]; + if (a === b) continue; + const cmp = String(a) < String(b) ? -1 : 1; + return node.order === 'desc' ? -cmp : cmp; + } + return 0; + }); + }; + const project = (rows: Record[], fields: any): Record[] => { + if (!Array.isArray(fields) || fields.length === 0) return rows; + return rows.map((r) => Object.fromEntries(fields.map((f: string) => [f, r[f]]))); + }; + const driver: any = { + name: 'memory', + version: '0.0.0', + supports: {} as any, + async connect() {}, + async disconnect() {}, + async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + const matched = Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + const sorted = applySort(matched, ast?.orderBy); + const from = typeof ast?.offset === 'number' ? ast.offset : 0; + const page = typeof ast?.limit === 'number' ? sorted.slice(from, from + ast.limit) : sorted.slice(from); + return project(page, ast?.fields); + }, + findStream() { throw new Error('not implemented'); }, + async findOne(object: string, ast: any) { + const rows = await this.find(object, ast); + return rows[0] ?? null; + }, + async create(object: string, data: Record) { + const id = String(data.id); + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const updated = { ...(s.get(id) ?? {}), ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { return this.create(object, data); }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)).length; + }, + async aggregate(object: string, ast: any) { return [{ count: await this.count(object, ast) }]; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, + async rollback() {}, + }; + return { driver, stores }; +} + +describe('#4226 — sort / select / expand on the list path (real ObjectQL engine)', () => { + let engine: ObjectQL; + let protocol: ObjectStackProtocolImplementation; + + /** The issue's transcript order: five rows inserted `C A E B D`. */ + const INSERTION_ORDER = ['C', 'A', 'E', 'B', 'D']; + + const titles = (r: any): string[] => r.records.map((x: any) => x.title); + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver, stores } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(projectObject as any, 'test-package'); + engine.registry.registerObject(taskObject as any, 'test-package'); + protocol = new ObjectStackProtocolImplementation(engine); + + stores.set('showcase_project', new Map([ + ['p1', { id: 'p1', name: 'Apollo' }], + ])); + const tasks = new Map>(); + INSERTION_ORDER.forEach((letter, i) => { + tasks.set(`t_${letter}`, { + id: `t_${letter}`, + title: letter, + status: i === 0 ? 'done' : 'open', + project_id: 'p1', + parent_id: letter === 'A' ? null : 't_A', + owner_id: 'usr_1', + created_at: '2026-07-30T00:00:00.000Z', + }); + }); + stores.set('showcase_task', tasks); + }); + + it('baseline — no sort returns the rows in insertion order', async () => { + const r: any = await protocol.findData({ object: 'showcase_task' }); + expect(titles(r)).toEqual(INSERTION_ORDER); + }); + + // ───────────────────────────────────────────────────────────── + // SORT — control group + // ───────────────────────────────────────────────────────────── + + it('a real field sorts, in both directions', async () => { + await expect(protocol.findData({ object: 'showcase_task', query: { sort: 'title' } })) + .resolves.toMatchObject({ records: expect.any(Array) }); + expect(titles(await protocol.findData({ object: 'showcase_task', query: { sort: 'title' } }))) + .toEqual(['A', 'B', 'C', 'D', 'E']); + expect(titles(await protocol.findData({ object: 'showcase_task', query: { sort: '-title' } }))) + .toEqual(['E', 'D', 'C', 'B', 'A']); + expect(titles(await protocol.findData({ object: 'showcase_task', query: { sort: 'title desc' } }))) + .toEqual(['E', 'D', 'C', 'B', 'A']); + }); + + it('EVERY wire spelling of a sort reaches the driver, not just the two that used to', async () => { + // `string[]` is the client SDK's own declared type + // (`orderBy?: string | string[] | SortNode[]`) and `{field: direction}` + // is what `GET /data/:object/export`, `GET /data/import/jobs` and + // objectui's calendar all emit. Both fell through the normalizer + // untouched and were then declined by every driver's + // `Array.isArray(orderBy)` guard: no ORDER BY clause, no error. + for (const query of [ + { sort: '-title' }, + { orderBy: ['-title'] }, + { orderBy: [{ field: 'title', order: 'desc' }] }, + { orderBy: { title: 'desc' } }, + { $orderby: { title: 'desc' } }, + { orderBy: { field: 'title', order: 'desc' } }, + ]) { + const r: any = await protocol.findData({ object: 'showcase_task', query }); + expect(titles(r), `spelling ${JSON.stringify(query)}`).toEqual(['E', 'D', 'C', 'B', 'A']); + } + }); + + // ───────────────────────────────────────────────────────────── + // SORT — rejected + // ───────────────────────────────────────────────────────────── + + it.each([ + ['bare string', { sort: 'no_such_field' }], + ['descending', { sort: '-no_such_field' }], + ['second of two', { sort: 'title,no_such_field' }], + ['string array', { orderBy: ['no_such_field'] }], + ['SortNode array', { orderBy: [{ field: 'no_such_field', order: 'desc' }] }], + ['direction map', { orderBy: { no_such_field: 'desc' } }], + ['OData spelling', { $orderby: 'no_such_field' }], + ])('sorting by an unknown field is a 400, not insertion order — %s', async (_label, query) => { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ + status: 400, + code: 'INVALID_SORT', + field: 'no_such_field', + object: 'showcase_task', + }); + }); + + it('garbage that parses to a field name is refused on the same terms', async () => { + // The issue's `sort={oops`: it reads as a field called `{oops`, matches + // nothing in the field map, and used to produce exactly the insertion + // order an unsorted query returns. + await expect(protocol.findData({ object: 'showcase_task', query: { sort: '{oops' } })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_SORT' }); + }); + + it.each([ + ['a number', { sort: 42 }], + ['a boolean', { orderBy: true }], + ['an entry naming no field', { orderBy: [{ order: 'desc' }] }], + ['an unreadable direction', { orderBy: [{ field: 'title', order: 'sideways' }] }], + ['an unreadable direction in the map form', { orderBy: { title: 'sideways' } }], + ])('a sort the normalizer cannot read is refused rather than dropped — %s', async (_label, query) => { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_SORT' }); + }); + + it('an unapplied sort can no longer hide behind `top` — the "latest N" footgun', async () => { + // This pairing is the whole reason the sort axis matters. Pre-fix it + // answered 200 with an arbitrary 2 of 5 rows and no way to tell. + expect(titles(await protocol.findData({ + object: 'showcase_task', query: { sort: '-title', top: 2 }, + }))).toEqual(['E', 'D']); + await expect(protocol.findData({ + object: 'showcase_task', query: { sort: '-no_such_field', top: 2 }, + })).rejects.toMatchObject({ status: 400, code: 'INVALID_SORT' }); + }); + + it('the rejection says which field and how to spell the direction', async () => { + await expect(protocol.findData({ object: 'showcase_task', query: { sort: 'stauts' } })) + .rejects.toThrow(/Did you mean the field 'status'/); + // `field:direction` is the EXPORT route's spelling. A caller moving + // between the two routes gets the list route's syntax, not a bare + // "no such field 'title:desc'". + await expect(protocol.findData({ object: 'showcase_task', query: { sort: 'title:desc' } })) + .rejects.toThrow(/sort=title desc/); + }); + + it('the rejection quotes the parameter the caller actually wrote', async () => { + // A caller who sent `?$orderby=…` must not be told that 'orderBy' — + // a name absent from their request — is the problem. + for (const [query, param] of [ + [{ sort: 'no_such_field' }, 'sort'], + [{ orderBy: 'no_such_field' }, 'orderBy'], + [{ $orderby: 'no_such_field' }, '$orderby'], + ] as const) { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ param }); + } + for (const [query, param] of [ + [{ select: 'no_such_field' }, 'select'], + [{ fields: 'no_such_field' }, 'fields'], + [{ $select: 'no_such_field' }, '$select'], + ] as const) { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ param }); + } + }); + + it('an empty sort is ABSENT, not malformed', async () => { + for (const query of [{ sort: '' }, { orderBy: [] }, { orderBy: {} }, { orderBy: null }]) { + expect(titles(await protocol.findData({ object: 'showcase_task', query }))) + .toEqual(INSERTION_ORDER); + } + }); + + it('a null orderBy alongside a real sort does not shadow it', async () => { + // `options.orderBy ?? options.sort` picks `sort` here, so the rejection + // (and the applied sort) must be attributed to `sort` too. + expect(titles(await protocol.findData({ + object: 'showcase_task', query: { orderBy: null, sort: '-title' }, + }))).toEqual(['E', 'D', 'C', 'B', 'A']); + await expect(protocol.findData({ + object: 'showcase_task', query: { orderBy: null, sort: 'no_such_field' }, + })).rejects.toMatchObject({ code: 'INVALID_SORT', param: 'sort' }); + }); + + // ───────────────────────────────────────────────────────────── + // SELECT — control group, then rejected + // ───────────────────────────────────────────────────────────── + + it('a real projection narrows the response', async () => { + const r: any = await protocol.findData({ object: 'showcase_task', query: { select: 'id,title' } }); + expect(Object.keys(r.records[0]).sort()).toEqual(['id', 'title']); + }); + + it('a projection naming NO known field no longer widens to every field', async () => { + // The failure this axis actually had: unknown columns are dropped, an + // empty projection falls back to `*`, and the two compose into "asked + // for one column, received all of them" — over-return from a parameter + // that exists to under-return. + await expect(protocol.findData({ object: 'showcase_task', query: { select: 'no_such_field' } })) + .rejects.toMatchObject({ + status: 400, + code: 'INVALID_FIELD', + field: 'no_such_field', + object: 'showcase_task', + param: 'select', + }); + }); + + it('a partially-unknown projection is refused too — half a projection is not the one asked for', async () => { + await expect(protocol.findData({ object: 'showcase_task', query: { select: 'title,no_such_field' } })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'no_such_field' }); + }); + + it.each([ + ['select', { select: 'no_such_field' }], + ['select as array', { select: ['no_such_field'] }], + ['fields', { fields: 'no_such_field' }], + ['$select', { $select: 'no_such_field' }], + ])('every projection spelling gets the same answer — %s', async (_label, query) => { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'no_such_field' }); + }); + + it('the system columns the registry injected still project', async () => { + // `applySystemFields` adds these at registration — a gate reading only + // the AUTHORED field map would reject every one of them. + expect(taskObject.fields).not.toHaveProperty('owner_id'); + const r: any = await protocol.findData({ + object: 'showcase_task', query: { select: 'id,owner_id,created_at' }, + }); + expect(Object.keys(r.records[0]).sort()).toEqual(['created_at', 'id', 'owner_id']); + }); + + // ───────────────────────────────────────────────────────────── + // EXPAND — control group, then rejected + // ───────────────────────────────────────────────────────────── + + it('a real relation expands', async () => { + const r: any = await protocol.findData({ object: 'showcase_task', query: { expand: 'project_id' } }); + expect(r.records[0].project_id).toMatchObject({ id: 'p1', name: 'Apollo' }); + }); + + it('a `tree` self-reference expands too — the spec always said it would', async () => { + // `REFERENCE_VALUE_TYPES` lists `tree` among the types whose value "points + // at another record … the related record object in expanded form", and + // objectui requests it. `expandRelatedRecords` used a hand-copied + // `!==` chain that omitted it, so it came back as a raw parent id. + const r: any = await protocol.findData({ + object: 'showcase_task', query: { expand: 'parent_id', sort: 'title' }, + }); + expect(r.records[0].parent_id).toBeNull(); // 'A' is the root + expect(r.records[1].parent_id).toMatchObject({ id: 't_A', title: 'A' }); + }); + + it.each([ + ['expand', { expand: 'no_such_rel' }], + ['populate', { populate: 'no_such_rel' }], + ['$expand', { $expand: 'no_such_rel' }], + ['the advanced map form', { expand: { no_such_rel: { object: 'no_such_rel' } } }], + ])('expanding something the object does not declare is a 400 — %s', async (_label, query) => { + await expect(protocol.findData({ object: 'showcase_task', query })) + .rejects.toMatchObject({ + status: 400, + code: 'INVALID_FIELD', + field: 'no_such_rel', + object: 'showcase_task', + param: 'expand', + }); + }); + + it('expanding a field that exists but holds no reference is refused, with its own reason', async () => { + // A different mistake from a typo, so a different message: `title` is a + // real column, it just has nothing to expand into. + await expect(protocol.findData({ object: 'showcase_task', query: { expand: 'title' } })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'title' }); + await expect(protocol.findData({ object: 'showcase_task', query: { expand: 'title' } })) + .rejects.toThrow(/is not a relationship/); + }); + + it('a reference field with no target gets a third message — the bug is on the OBJECT', async () => { + // A `lookup` whose `reference` was never authored: the engine has + // nothing to batch-load, so expansion cannot work — but telling the + // caller "not a relationship" about a declared lookup sends them to + // fix the wrong end. + engine.registry.registerObject({ + name: 'showcase_broken', + label: 'Broken', + fields: { + id: { name: 'id', label: 'ID', type: 'text', primaryKey: true }, + orphan: { name: 'orphan', label: 'Orphan', type: 'lookup' }, + }, + } as any, 'test-package'); + await expect(protocol.findData({ object: 'showcase_broken', query: { expand: 'orphan' } })) + .rejects.toThrow(/declares no target object/); + }); + + // ───────────────────────────────────────────────────────────── + // The single-record route answers identically (#4226) + // ───────────────────────────────────────────────────────────── + + it('GET /data/:object/:id gives the same answer as the list route', async () => { + // "Two routes, opposite answers for one input" is the failure mode this + // whole family of issues keeps rediscovering. `getData` reads the same + // `select`/`expand` against the same field map. + await expect(protocol.getData({ object: 'showcase_task', id: 't_A', select: 'id,title' })) + .resolves.toMatchObject({ record: { title: 'A' } }); + await expect(protocol.getData({ object: 'showcase_task', id: 't_A', select: 'no_such_field' })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD' }); + await expect(protocol.getData({ object: 'showcase_task', id: 't_A', expand: 'no_such_rel' })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD' }); + }); + + // ───────────────────────────────────────────────────────────── + // Tiering — the gates must not overreach + // ───────────────────────────────────────────────────────────── + + it('an unknown OBJECT is still a 404 — no axis gate may turn it into a 400', async () => { + for (const query of [ + { sort: 'no_such_field' }, + { select: 'no_such_field' }, + { expand: 'no_such_rel' }, + ]) { + await expect(protocol.findData({ object: 'no_such_object', query })) + .rejects.toMatchObject({ status: 404, code: 'OBJECT_NOT_FOUND' }); + } + }); + + it('a legacy ARRAY field map disables every gate rather than rejecting everything', async () => { + // `Object.keys` on an array yields '0','1','2' …, so a gate that read + // one as a field map would 400 on every real field name it was handed. + // Same call the no-field-map case makes: nothing here can answer "does + // this field exist". + const arrayEngine: any = { + find: async () => [], + count: async () => 0, + registry: { getObject: (name: string) => ({ name, fields: [{ name: 'title' }] }) }, + }; + const lenient = new ObjectStackProtocolImplementation(arrayEngine); + for (const query of [ + { sort: 'no_such_field' }, + { select: 'no_such_field' }, + { expand: 'no_such_rel' }, + ]) { + await expect(lenient.findData({ object: 'legacy_object', query })).resolves.toBeDefined(); + } + }); + + it('the filter axis keeps its own codes — the new gates did not swallow them', async () => { + await expect(protocol.findData({ object: 'showcase_task', query: { filter: '{status:done' } })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' }); + await expect(protocol.findData({ object: 'showcase_task', query: { pageSize: '5' } })) + .rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD' }); + await expect(protocol.findData({ object: 'showcase_task', query: { $nope: '5' } })) + .rejects.toMatchObject({ status: 400, code: 'UNSUPPORTED_QUERY_PARAM' }); + }); + + it('all four axes compose on one request when every name is real', async () => { + const r: any = await protocol.findData({ + object: 'showcase_task', + query: { + filter: JSON.stringify({ status: 'open' }), + sort: '-title', + select: 'id,title,project_id', + expand: 'project_id', + top: 2, + }, + }); + expect(titles(r)).toEqual(['E', 'D']); + expect(r.total).toBe(4); + expect(r.hasMore).toBe(true); + expect(r.records[0].project_id).toMatchObject({ id: 'p1', name: 'Apollo' }); + expect(Object.keys(r.records[0]).sort()).toEqual(['id', 'project_id', 'title']); + }); +}); diff --git a/packages/spec/src/api/odata.zod.ts b/packages/spec/src/api/odata.zod.ts index b0c0274522..510d50149f 100644 --- a/packages/spec/src/api/odata.zod.ts +++ b/packages/spec/src/api/odata.zod.ts @@ -163,7 +163,7 @@ export const ODataQuerySchema = lazySchema(() => z.object({ $expand: z.union([ z.string(), // "orders" z.array(z.string()), // ["orders", "customer"] - ]).optional().describe('Navigation properties to expand (lookup/master_detail fields)'), + ]).optional().describe('Navigation properties to expand (reference fields: lookup/master_detail/user/tree)'), /** * $count - Include total count From dfab708a4be46dc746375fb3933fd224784b1a15 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:55:34 +0000 Subject: [PATCH 2/2] docs(data): the ObjectQL query pages describe the ingress gates (#4226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `query-syntax.mdx` and `queries.mdx` both taught `expand` as reaching `lookup` / `master_detail` / `user`, which omitted `tree` — the omission the engine's own `!==` chain had, now that the loop reads `REFERENCE_VALUE_TYPES`. They also described the `orderBy` and projection failure modes purely as engine behaviour. That stays true for internal callers and is left standing; what is added is which of them the REST/protocol ingress now refuses, plus the sort spellings it accepts. The dotted-path sort keeps its warning and gains the reason it survives the gate: a dotted path is judged on its head segment, so it reaches the driver backstop rather than a 400. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0158f3QC22Lf2VfJrT9nQ9om --- content/docs/data-modeling/queries.mdx | 17 ++++++++++++++++- content/docs/protocol/objectql/query-syntax.mdx | 14 +++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/content/docs/data-modeling/queries.mdx b/content/docs/data-modeling/queries.mdx index 7f550d9e57..219f0313eb 100644 --- a/content/docs/data-modeling/queries.mdx +++ b/content/docs/data-modeling/queries.mdx @@ -216,6 +216,14 @@ Sort results with `orderBy` (array of sort nodes): | `field` | `string` | Field name to sort by | | `order` | `'asc' \| 'desc'` | Sort direction | + +Over the REST/protocol ingress, `orderBy` also accepts `'-created_at'`, +`['-created_at']` and `{created_at: 'desc'}`, all normalized to the node array +above. A sort naming a field the object does not have — or an `order` that is +neither `asc` nor `desc` — is `400 INVALID_SORT` there rather than a dropped +sort. Internal callers reaching `engine.find()` directly are unaffected. + + --- ## Pagination @@ -288,13 +296,20 @@ and you get every column; if it doesn't, the error is rethrown. Either way there `owner.name` key. Use `expand` to pull in a relationship's fields instead. + +Both of these are `engine.find()` behaviours, reached by internal callers. Over +the REST/protocol ingress a projection column that names no field at all is +`400 INVALID_FIELD` — including the case where *no* requested column is known, +which used to fall back to `SELECT *` and answer a one-column request with every +column. --- ## Expand (Related Records) -Load related records through `lookup`, `master_detail`, and `user` fields with +Load related records through the reference field types — `lookup`, +`master_detail`, `user` and `tree` (`REFERENCE_VALUE_TYPES`) — with `expand`. Each key is a relationship field name; the value is a nested query that can select fields, filter, and expand further (max depth 3 — a fixed constant, not configurable). diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index c7e1b97dea..db068ac02a 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -415,6 +415,14 @@ const opportunities = await engine.find('opportunity', { Sorting uses the **`orderBy`** array of `SortNode` objects. + +Over the REST/protocol ingress, `orderBy` also accepts `'-created_at'`, +`['-created_at']` and `{created_at: 'desc'}` — all normalized to the +`SortNode[]` above. A sort naming a field the object does not have is +`400 INVALID_SORT` there, rather than being dropped. Internal callers reaching +`engine.find()` directly are unaffected. + + ### Single Field Sort ```typescript @@ -449,13 +457,17 @@ const query: QueryAST = { `SqlDriver.find()` then retries **without the sort**, so the rows come back unordered rather than sorted — and no error surfaces. Denormalise the value onto the queried object (for example with a formula or rollup field) when you need to sort by it. + +This is the one sort that still degrades silently: the REST ingress judges a +dotted path on its **head segment** (`account` here, a real field), so the +`INVALID_SORT` gate passes it through to the driver backstop. --- ## 4. Relationships (Expand) -The `expand` property enables **recursive loading of related records** through `lookup`, `master_detail`, and `user` fields. Each key is a relationship field name; the value is a nested `QueryAST`. +The `expand` property enables **recursive loading of related records** through the reference field types — `lookup`, `master_detail`, `user` and `tree` (`REFERENCE_VALUE_TYPES`). Each key is a relationship field name; the value is a nested `QueryAST`. Over the REST/protocol ingress, a key that is not one of those is `400 INVALID_FIELD` rather than a silently absent relation. ### Basic Expand