diff --git a/.changeset/findata-query-param-arity.md b/.changeset/findata-query-param-arity.md new file mode 100644 index 0000000000..e21306cbd5 --- /dev/null +++ b/.changeset/findata-query-param-arity.md @@ -0,0 +1,38 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +`findData`'s shared list-query normalizer now checks the ARITY of every query +parameter it reads, instead of coercing a repeated one blind (#7321). + +`IHttpRequest.query` is `Record< string, string | string[] >` and the array arm +is produced by a real first-party adapter (`NodeHttpServer` hands `?x=1&x=2` +through as `['1','2']`). Every coercion in this normalizer was written for the +string arm, so a repeated parameter was coerced into a value nobody asked for +and served under a 200: + +- `?$top=1&$top=2` → `Number(['1','2'])` is `NaN` → the driver was called with + `limit: NaN`. Same for `$skip` / `offset`. +- `?status=open&status=won` → the leftover-key bucket lowered it to + `where: { status: ['open','won'] }`, and a bare array is not a valid field + spec — it matches no row on any backend. An empty page, 200 OK. +- `?$search=a&$search=b`, `?$count=true&$count=false` and a repeated body + `object` behaved the same way, each in its own flavour. + +Those are now refused with `400` / `error.code: INVALID_REQUEST` — the code this +same normalizer already answers for the identical condition reached the other +way (two SPELLINGS of one slot given different values, #4181 → #3795). A +one-element array is one occurrence and is unwrapped, not refused; an empty +array is no occurrence. + +**Unchanged on purpose — this is a per-parameter judgement, not a sweep.** +`$select` / `select` / `fields`, `$expand` / `populate` / `expand`, +`$searchFields`, `$orderby` / `sort` / `orderBy`, `$filter` / `filter` / +`filters` / `where` (whose array arm is a FILTER AST, not a repetition), +`groupBy` and `aggregations` all accept the array arm on purpose and keep it +byte for byte. A blanket "reject repeated parameters" rule would have broken +every one of them. + +Not reachable on today's production Hono adapter, which collapses repeated +parameters to the first value before any handler runs; it becomes reachable when +that collapse is removed (#6878 route 2). diff --git a/packages/metadata-protocol/src/protocol.query-param-arity.test.ts b/packages/metadata-protocol/src/protocol.query-param-arity.test.ts new file mode 100644 index 0000000000..79e2da226c --- /dev/null +++ b/packages/metadata-protocol/src/protocol.query-param-arity.test.ts @@ -0,0 +1,316 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7321 — `findData`'s list-query normalizer coerces repeated query parameters + * without checking arity. + * + * `IHttpRequest.query` is `Record< string, string | string[] >` + * (`packages/spec/src/contracts/http-server.ts`) and the array arm is produced + * by a real first-party adapter: `NodeHttpServer` hands `?x=1&x=2` through as + * `['1','2']`, measured over a socket on #6878. Every coercion in this + * normalizer was written for the string arm, so the array arm was coerced + * blind — `Number(['1','2'])` is `NaN`, and `?$top=1&$top=2` reached the driver + * as `limit: NaN`. That is the one MEASURED line the card was filed on; the + * work is the survey around it, and the survey found the same shape on the + * leftover-key bucket (`?status=open&status=won` lowers to + * `where: {status: ['open','won']}`, which `matches-filter.ts` answers with a + * bare `if (Array.isArray(spec)) return false` — an empty page under a 200). + * + * ## Three assertion classes, labelled, because only one of them is evidence + * + * 1. **REFUSAL** — a repeated single-valued parameter answers `400` + * `INVALID_REQUEST` and the engine is never reached. Both `code` AND + * `status` are asserted on every one of these: a bare `toThrow()` here + * would be a permanently-green test for half the cases, because the + * unfixed normalizer ALSO throws for some of them (a repeated `?filter=` + * hits `malformedFilterArrayError` — a true refusal with a false + * diagnosis), and would be blind for the other half, where the unfixed + * normalizer answers 200 with the wrong rows. + * + * 2. **PRESERVATION of the legitimately-multi parameters** — `$select`, + * `$expand`, `$searchFields`, `$orderby` and `$filter`'s AST array accept + * the array arm ON PURPOSE. These assertions are GREEN IN BOTH DIRECTIONS + * against the fix (they pass on `origin/main` unchanged), so on their own + * they are GUARDS, not evidence. What makes them evidence is the VARIANT + * measured on the PR: emptying `ARRAY_VALUED_QUERY_SLOTS` — i.e. replacing + * the per-parameter disposition with a blanket "no parameter may repeat" — + * turns this whole block red while block 1 stays green. That is the + * damage case the card was filed to prevent, and it is what makes the + * disposition table NECESSARY rather than merely sufficient. + * + * 3. **PRESERVATION of the ordinary single-valued request** — one occurrence, + * as a bare string, is untouched. Also green in both directions, also a + * guard: it is what a refusal is cheapest to break. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'invoice', + nameField: 'name', + searchableFields: ['name', 'status'], + fields: { + name: { name: 'name', type: 'text' }, + status: { name: 'status', type: 'text' }, + amount: { name: 'amount', type: 'number' }, + owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' }, + account_id: { name: 'account_id', type: 'lookup', reference: 'account' }, + }, +}; + +function makeProtocol() { + const find = vi.fn(async () => [] as unknown[]); + const aggregate = vi.fn(async () => [] as unknown[]); + const engine = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + find, + aggregate, + count: vi.fn(async () => 0), + }; + return { p: new ObjectStackProtocolImplementation(engine as any), find, aggregate }; +} + +/** The option bag `engine.find` was actually handed, for an accepted query. */ +async function optionsFor(query: Record): Promise> { + const { p, find } = makeProtocol(); + await p.findData({ object: 'invoice', query } as never); + expect(find, `${JSON.stringify(query)} never reached engine.find`).toHaveBeenCalledTimes(1); + return (find.mock.calls[0] as unknown[])[1] as Record; +} + +/** The refusal a rejected query produced, plus proof the engine was not reached. */ +async function refusalFor(query: Record): Promise<{ + message: string; status?: number; code?: string; param?: string; +}> { + const { p, find, aggregate } = makeProtocol(); + let answered: unknown; + try { + answered = await p.findData({ object: 'invoice', query } as never); + } catch (e) { + const err = e as Error & { status?: number; code?: string; param?: string }; + expect(find, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + expect(aggregate, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + return { message: err.message, status: err.status, code: err.code, param: err.param }; + } + throw new Error( + `${JSON.stringify(query)} was ACCEPTED (answered ${JSON.stringify(answered)}) instead of refused`, + ); +} + +// --------------------------------------------------------------------------- +// 1. REFUSAL — the parameters whose declared type is a scalar +// --------------------------------------------------------------------------- + +describe('#7321 — a repeated single-valued parameter is refused, not coerced', () => { + it.each<[string, Record, string]>([ + // [wire spelling the caller wrote, the query, what it used to become] + ['$top', { $top: ['1', '2'] }, 'limit: NaN'], + ['top', { top: ['1', '2'] }, 'limit: NaN'], + ['limit', { limit: ['1', '2'] }, 'limit: NaN'], + ['$skip', { $skip: ['10', '20'] }, 'offset: NaN'], + ['skip', { skip: ['10', '20'] }, 'offset: NaN'], + ['offset', { offset: ['10', '20'] }, 'offset: NaN'], + ['$search', { $search: ['a', 'b'] }, 'a two-element search term'], + ['search', { search: ['a', 'b'] }, 'a two-element search term'], + ['$count', { $count: ['true', 'false'] }, 'neither true nor false'], + ['count', { count: ['true', 'false'] }, 'neither true nor false'], + ['object', { object: ['invoice', 'account'] }, 'a bogus object mismatch'], + ['having', { having: [{ a: 1 }, { b: 2 }], groupBy: ['status'] }, 'AST junk on aggregate'], + ])('refuses a repeated %s with 400 INVALID_REQUEST (was: %s)', async (param, query) => { + const err = await refusalFor(query); + + // The ADR-0112 envelope, not merely the throw: `code` AND `status`. + expect(err.code).toBe('INVALID_REQUEST'); + expect(err.status).toBe(400); + // #4226 discipline — the message names the spelling the caller WROTE, + // not the canonical key the fold would have rewritten it to. + expect(err.param).toBe(param); + expect(err.message).toContain(`'${param}' query parameter was supplied 2 times`); + }); + + it('refuses TWO IDENTICAL values too — the rule counts occurrences, not distinct values', async () => { + // "At most one DISTINCT value" would be a de-duplication rule no caller + // can predict; "supply it at most once" is checkable client-side + // (#6877). `?$count=true&$count=true` is still two occurrences. + const err = await refusalFor({ $count: ['true', 'true'] }); + + expect(err.code).toBe('INVALID_REQUEST'); + expect(err.status).toBe(400); + expect(err.message).toContain('supplied 2 times'); + }); + + it('reports the real count, not just "more than one"', async () => { + const err = await refusalFor({ $top: ['1', '2', '3', '4'] }); + + expect(err.message).toContain('supplied 4 times'); + }); + + it('refuses a repeated LEFTOVER key — the implicit field-filter bucket', async () => { + // `?status=open&status=won` lowered to `where: {status: ['open','won']}`. + // A bare array is not a valid field spec (`{ $in: [...] }` is), so + // `matches-filter.ts` answers `false` for every row: an empty page under + // a 200, which is the #4134 failure exactly. + const err = await refusalFor({ status: ['open', 'won'] }); + + expect(err.code).toBe('INVALID_REQUEST'); + expect(err.status).toBe(400); + expect(err.param).toBe('status'); + }); + + it('refuses the repeated parameter BEFORE the alias fold mis-diagnoses it', async () => { + // `?top=1&top=2&limit=1` reaches the #3795 fold as `['1','2']` vs `'1'`, + // which `JSON.stringify` calls two different values for one slot — a + // true refusal (`Conflicting query parameters`) with a false diagnosis. + // Arity runs first, so the caller is told what is actually wrong. + const err = await refusalFor({ top: ['1', '2'], limit: '1' }); + + expect(err.message).toContain("'top' query parameter was supplied 2 times"); + expect(err.message).not.toContain('Conflicting query parameters'); + }); +}); + +// --------------------------------------------------------------------------- +// 2. PRESERVATION — GUARDS. Green in both directions; see the variant file. +// --------------------------------------------------------------------------- + +describe('#7321 [GUARD — green in both directions] the legitimately-multi parameters keep their array arm', () => { + it('$select repeated IS the projection list', async () => { + const options = await optionsFor({ $select: ['name', 'status'] }); + + expect(options.fields).toEqual(['name', 'status']); + }); + + it('select / fields repeated are the same projection under their other spellings', async () => { + expect((await optionsFor({ select: ['name', 'status'] })).fields).toEqual(['name', 'status']); + expect((await optionsFor({ fields: ['name', 'status'] })).fields).toEqual(['name', 'status']); + }); + + it('$expand repeated IS the relation list', async () => { + const options = await optionsFor({ $expand: ['owner_id', 'account_id'] }); + + expect(options.expand).toEqual({ + owner_id: { object: 'owner_id' }, + account_id: { object: 'account_id' }, + }); + }); + + it('$searchFields repeated IS the narrowed search set', async () => { + const options = await optionsFor({ $search: 'acme', $searchFields: ['name', 'status'] }); + + expect(options.searchFields).toEqual(['name', 'status']); + }); + + it('$orderby repeated COMPOSES into a multi-key sort', async () => { + // Repetition on a list-valued slot concatenates; it does not conflict. + // `normalizeSortNodes` has had an explicit `string[]` arm since #4226. + const options = await optionsFor({ $orderby: ['name', '-amount'] }); + + expect(options.orderBy).toEqual([ + { field: 'name', order: 'asc' }, + { field: 'amount', order: 'desc' }, + ]); + }); + + it('a filter AST stays readable — `where`\'s array arm is a FILTER, not a repetition', async () => { + // The single most expensive thing a blanket arity rule would break: + // `['status','=','open']` is a three-element array that IS one filter. + const options = await optionsFor({ $filter: ['status', '=', 'open'] }); + + expect(options.where).toEqual({ status: 'open' }); + }); + + it.each<[string, Record]>([ + // Every WIRE spelling that folds into an array-valued slot, with a + // two-element value that is otherwise valid for that slot. The source's + // set is DERIVED from the same alias tables the fold uses, so a new + // alias inherits its array arm automatically — this case is the + // behavioural half of that derivation, and a new alias belongs here too. + ['$select', { $select: ['name', 'status'] }], + ['select', { select: ['name', 'status'] }], + ['fields', { fields: ['name', 'status'] }], + ['$orderby', { $orderby: ['name', '-amount'] }], + ['sort', { sort: ['name', '-amount'] }], + ['orderBy', { orderBy: ['name', '-amount'] }], + ['$expand', { $expand: ['owner_id', 'account_id'] }], + ['populate', { populate: ['owner_id', 'account_id'] }], + ['expand', { expand: ['owner_id', 'account_id'] }], + ['$searchFields', { $search: 'acme', $searchFields: ['name', 'status'] }], + ['searchFields', { search: 'acme', searchFields: ['name', 'status'] }], + ['$filter', { $filter: ['status', '=', 'open'] }], + ['filter', { filter: ['status', '=', 'open'] }], + ['filters', { filters: ['status', '=', 'open'] }], + ['where', { where: ['status', '=', 'open'] }], + ['groupBy', { groupBy: ['status', 'name'] }], + ['aggregations', { aggregations: [ + { function: 'sum', field: 'amount', alias: 'total' }, + { function: 'count', field: 'amount', alias: 'n' }, + ] }], + ])('%s accepts a two-element array without a 400', async (_param, query) => { + // Asserted as "not refused" rather than "reached engine.find", because + // `groupBy` / `aggregations` legitimately route to `engine.aggregate`. + const { p } = makeProtocol(); + + await expect(p.findData({ object: 'invoice', query } as never)).resolves.toBeDefined(); + }); + + it('groupBy / aggregations keep their array arms', async () => { + const { p, aggregate } = makeProtocol(); + await p.findData({ + object: 'invoice', + query: { groupBy: ['status'], aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }] }, + } as never); + + expect(aggregate).toHaveBeenCalledTimes(1); + const opts = (aggregate.mock.calls[0] as unknown[])[1] as Record; + expect(opts.groupBy).toEqual(['status']); + }); +}); + +describe('#7321 [GUARD — green in both directions] an ordinary single-valued request is untouched', () => { + it('?$top=5&$skip=10 still normalizes to limit/offset numbers', async () => { + const options = await optionsFor({ $top: '5', $skip: '10' }); + + expect(options.limit).toBe(5); + expect(options.offset).toBe(10); + }); + + it('a single leftover key is still an implicit equality predicate', async () => { + expect((await optionsFor({ status: 'open' })).where).toEqual({ status: 'open' }); + }); + + it('a comma-list projection is still split, not treated as multi-valued', async () => { + expect((await optionsFor({ $select: 'name,status' })).fields).toEqual(['name', 'status']); + }); +}); + +// --------------------------------------------------------------------------- +// 3. The one-occurrence array arm — an adapter's encoding, not a repetition +// --------------------------------------------------------------------------- + +describe('#7321 — a ONE-element array is one occurrence, unwrapped rather than refused', () => { + it('unwraps a leftover key, which used to match nothing at all', async () => { + // The signal case. `{status: ['open']}` is a bare array field spec, so + // `matches-filter.ts` answered `false` for every row: the query looked + // served and returned an empty page. + expect((await optionsFor({ status: ['open'] })).where).toEqual({ status: 'open' }); + }); + + it('[GUARD] unwraps a one-element window, which `Number()` already got right', async () => { + // Green in both directions on purpose: `Number(['5'])` is 5, because a + // one-element array stringifies to its element. Pinned so the unwrap + // cannot silently start producing something else. + expect((await optionsFor({ $top: ['5'] })).limit).toBe(5); + }); + + it('treats an EMPTY array as not supplied, rather than as a value', async () => { + // `Number([])` is 0, so an empty `limit` used to become `limit: 0`; and + // a key left behind carrying `undefined` would be lowered into an + // implicit `{status: undefined}` predicate by the leftover bucket. + const options = await optionsFor({ limit: [], status: [] }); + + expect(options).not.toHaveProperty('limit'); + expect(options).not.toHaveProperty('status'); + expect(options.where).toBeUndefined(); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 9763b804f4..047d833abe 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1505,6 +1505,174 @@ const WIRE_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = (() => { })); })(); +/** + * The OData `$`-prefixed spelling of each bare wire parameter this normalizer + * consumes, hoisted out of the loop in `findData` that used to own it so the + * arity survey below and that loop read ONE table (#7321). Adding a `$` alias + * in one place and not the other is exactly how a parameter ends up folded but + * unchecked. + * + * `$filter` / `$expand` are deliberately absent: they are declared as slot + * aliases on {@link WIRE_QUERY_ALIAS_SLOTS} instead, because they fold straight + * to a canonical key rather than to a bare wire spelling. + */ +const WIRE_DOLLAR_ALIASES: readonly (readonly [string, string])[] = [ + ['$top', 'top'], + ['$skip', 'skip'], + ['$orderby', 'orderBy'], + ['$select', 'select'], + ['$count', 'count'], + ['$search', 'search'], + ['$searchFields', 'searchFields'], +]; + +/** + * [#7321] The list-query slots whose DECLARED value type admits an array, by + * canonical key. Everything else this normalizer reads — including a leftover + * key lowered into an implicit field filter — is single-valued, and an array on + * it is a repeated wire parameter (see {@link assertQueryParamArity}). + * + * This set is the whole judgement. `IHttpRequest.query` is + * `Record< string, string | string[] >` and the array arm is produced by a real + * first-party adapter (`NodeHttpServer` hands `?x=1&x=2` through as + * `['1','2']`, measured over a socket on #6878), so `Array.isArray` at this + * boundary means one of exactly two things: a repeated querystring parameter, + * or a JSON array in a `POST /data/:object/query` body. This normalizer serves + * BOTH ingresses and cannot tell them apart — which is why the rule keys off the + * declared TYPE rather than off the request. On a slot that never declares an + * array, `Array.isArray` is unambiguous evidence of repetition; on a slot that + * does, it is the ordinary shape and must not be touched. + * + * Per member, why the array is legal (`packages/spec/src/data/query.zod.ts`): + * - `fields` — `z.array(FieldNodeSchema)`; `?$select=a&$select=b` IS the + * projection `['a','b']`. + * - `orderBy` — `z.array(SortNodeSchema)`, and `normalizeSortNodes` has an + * explicit `string[]` arm: repetition COMPOSES into a + * multi-key sort (`?sort=name&sort=-age`), it does not + * conflict. + * - `expand` — the name-array arm lowers to `{name: {object: name}}`. + * - `searchFields` — `z.array(z.string())`; the engine reads the comma-string + * and the array from either slot. + * - `where` — a FILTER AST is an array (`['status','=','open']`), so a + * blanket arity refusal here would reject the AST body form + * outright. A repeated `?filter=` is still refused, one + * block down, by `isFilterAST` failing to read it. + * - `groupBy` — `z.array(GroupByNodeSchema)`. + * - `aggregations` — `z.array(AggregationNodeSchema)`. + * - `joins` / `windowFunctions` — retired ARRAY keys (#4286). The tombstone, + * not an arity refusal, must stay their answer. + * + * Deliberately NOT here, each because the spec declares a scalar: `limit`/`top` + * and `offset` (`z.number()`), `search` (`string | FullTextSearch`), `object` + * (`z.string()`), `having` (a `FilterCondition` OBJECT — the engine has no AST + * arm for it), the `count` response flag, and the retired scalars `cursor` / + * `distinct`. + */ +const ARRAY_VALUED_QUERY_SLOTS: readonly string[] = [ + 'fields', 'orderBy', 'expand', 'searchFields', 'where', + 'groupBy', 'aggregations', 'joins', 'windowFunctions', +]; + +/** + * [#7321] {@link ARRAY_VALUED_QUERY_SLOTS} expanded to every WIRE spelling that + * reaches it, derived from the same two tables the fold uses so a new alias + * cannot silently lose its array arm — the failure mode would be `?$select=a& + * $select=b` starting to 400, i.e. the damage case this card exists to avoid. + */ +const ARRAY_VALUED_LIST_QUERY_PARAMS: ReadonlySet = (() => { + const names = new Set(ARRAY_VALUED_QUERY_SLOTS); + for (const slot of WIRE_QUERY_ALIAS_SLOTS) { + if (!names.has(slot.canonical)) continue; + for (const alias of slot.aliases) names.add(alias); + } + for (const [dollar, bare] of WIRE_DOLLAR_ALIASES) { + if (names.has(bare)) names.add(dollar); + } + return names; +})(); + +/** + * [#7321] A parameter this normalizer reads as single-valued, supplied more + * than once. + * + * ## Why a refusal, and why this code + * + * `?$top=1&$top=2` is a well-formed request carrying two irreconcilable + * intents. Picking one is the silent drop itself, and coercing the pair is + * worse: `Number(['1','2'])` is `NaN`, so the window reached the driver as + * `limit: NaN` — driver-dependent behaviour under a 200, never an error. That + * is the same class #6928 / PR #7299 refused one layer over on + * `GET /api/v1/notifications`, and the same rule #6307 / #6877 landed in + * `packages/rest` (`readSingleQueryValue`); the wording below is theirs + * verbatim so a caller who repeats a parameter on two different routes is told + * the same thing twice, not two things once. + * + * `INVALID_REQUEST` / 400 is what {@link conflictingQueryParamsError} in this + * same normalizer already answers for the IDENTICAL condition reached the other + * way — two SPELLINGS of one slot carrying different values (#4181 → #3795). + * One slot given two values is one defect; it must not carry two codes + * depending on whether the caller repeated `filter` or wrote `filter` and + * `where`. (The rest layer spells its 400 `VALIDATION_ERROR` and the runtime + * layer `VALIDATION_FAILED`; those are each package's house catalog member for + * a 400, registered per package in `error-code-ledger.zod.ts`. The RULE and the + * status are what have to agree across the three, and do.) + * + * ## Why the count and not the values + * + * Two identical values are still two occurrences and are still refused: "at + * most one DISTINCT value" would be a de-duplication rule no caller can + * predict, while "supply it at most once" is checkable client-side without + * knowing anything about our semantics (#6877). + */ +function repeatedQueryParamError(param: string, count: number): Error { + const err: any = new Error( + `The '${param}' query parameter was supplied ${count} times. Supply it at most once — ` + + 'this endpoint will not choose between conflicting values. It was NOT applied as a ' + + 'list: a single-valued parameter given an array coerces to a value nobody asked for ' + + `(Number(['1','2']) is NaN), which the driver then answers under a 200.`, + ); + err.status = 400; + err.code = 'INVALID_REQUEST'; + err.param = param; + return err; +} + +/** + * [#7321] Refuse a repeated occurrence of any list-query parameter this + * normalizer reads as single-valued, and normalise the benign one-element array + * away so nothing below has to know the union existed. + * + * Runs at the TOP of `findData`, ahead of the `$`-alias pass and the #3795 slot + * fold, for two reasons: + * + * 1. The message then quotes the parameter the caller actually wrote — the + * #4226 discipline. After the fold, `?$top=1&$top=2` would be reported as + * `'limit'`, a name absent from the request. + * 2. The fold compares slot spellings by `JSON.stringify`, so an unchecked + * `?top=1&top=2&limit=1` reaches it as `['1','2']` vs `'1'` and is refused + * as a "conflicting query parameters" problem — a true refusal with a false + * diagnosis. Checking arity first means the caller is told what is actually + * wrong. + * + * Length 0 is DELETED rather than set to `undefined` (which is what the rest + * layer's `refuseRepeatedQueryParams` does with it): the leftover-key bucket + * below reads `Object.keys(options)`, so a key left behind carrying `undefined` + * would be lowered into an implicit `{field: undefined}` predicate. "Not + * supplied" has to mean absent here, not present-and-empty. + */ +function assertQueryParamArity(options: Record): void { + for (const name of Object.keys(options)) { + const value = options[name]; + if (!Array.isArray(value)) continue; + if (ARRAY_VALUED_LIST_QUERY_PARAMS.has(name)) continue; + if (value.length > 1) throw repeatedQueryParamError(name, value.length); + // length 1 → one occurrence an adapter encoded as an array; length 0 → + // no occurrence at all. + if (value.length === 0) delete options[name]; + else options[name] = value[0]; + } +} + /** * [#4181 → #3795] Spellings of ONE slot carrying DIFFERENT values. Two values * for one slot cannot be reconciled — merging them would invent an intent the @@ -5653,6 +5821,20 @@ export class ObjectStackProtocolImplementation implements // `context` unconditionally: the protocol must not depend on a gate // above it staying switched on. delete options.context; + + // [#7321] Arity BEFORE any read, fold or coercion. `IHttpRequest.query` + // is `Record< string, string | string[] >`; the array arm is real (the + // `node:http` adapter hands `?x=1&x=2` through as `['1','2']`, measured + // over a socket on #6878), and every coercion below was written for the + // string arm. `Number(['1','2'])` is `NaN`, so `?$top=1&$top=2` used to + // reach the driver as `limit: NaN` — a wrong answer served as a 200. + // + // Single-vs-multi is a PER-PARAMETER judgement, never a sweep: `$select` + // / `$expand` / `$searchFields` / `$orderby` accept the array arm on + // purpose and are untouched here. See + // {@link ARRAY_VALUED_QUERY_SLOTS} for the full disposition. + assertQueryParamArity(options); + // Forward the dispatcher's ExecutionContext so RBAC/RLS middleware // can apply per-request enforcement. The protocol layer is purely // a normalizer — it must never strip security context. @@ -5675,16 +5857,12 @@ export class ObjectStackProtocolImplementation implements // 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. + // + // [#7321] The table itself now lives at module scope + // ({@link WIRE_DOLLAR_ALIASES}) so the arity survey above and this fold + // cannot drift apart on which `$` spellings exist. const wireSpelling: Record = {}; - for (const [dollar, bare] of [ - ['$top', 'top'], - ['$skip', 'skip'], - ['$orderby', 'orderBy'], - ['$select', 'select'], - ['$count', 'count'], - ['$search', 'search'], - ['$searchFields', 'searchFields'], - ] as const) { + for (const [dollar, bare] of WIRE_DOLLAR_ALIASES) { if (options[dollar] != null && options[bare] == null) { options[bare] = options[dollar]; wireSpelling[bare] = dollar;