diff --git a/.changeset/rpc-alias-precedence-one-fold.md b/.changeset/rpc-alias-precedence-one-fold.md new file mode 100644 index 0000000000..d1a8d32e49 --- /dev/null +++ b/.changeset/rpc-alias-precedence-one-fold.md @@ -0,0 +1,55 @@ +--- +"@objectstack/spec": minor +"@objectstack/metadata-protocol": patch +"@objectstack/runtime": patch +--- + +fix(spec,data): the five RPC query aliases resolve by ONE fold — spec table, not per-reader prose (#3795) + +`RpcQueryOptionsSchema` accepts five legacy aliases next to their canonical +QueryAST keys and stated the precedence in prose only ("the normalizer uses +the new key"). With no fold in the schema, every reader re-implemented it — +the #3713 condition — and the two readers disagreed: + +| pair | spec prose | runtime dispatcher | metadata-protocol | +|---|---|---|---| +| `where` > `filter` | canonical | canonical | **alias consulted first** | +| `fields` > `select` | canonical | canonical | **alias clobbered canonical** | +| `offset` > `skip` | canonical | canonical | **alias clobbered canonical** | +| `expand` > `populate` | canonical | — | **alias consulted first** | +| `orderBy` > `sort` | canonical | canonical | canonical | + +Four of five inverted in `protocol.ts`, so `?select=a&fields=b` answered +`[a]` on one path and `[b]` on the other — reachable from a plain HTTP +request. + +**The mapping now lives once, in the spec** (`RPC_QUERY_ALIAS_SLOTS` + +`foldQueryAliasSlots`, both exported), under the rule #4181 already +established for the filter pair: + +- an **alias alone** folds into its canonical key — `filter`→`where`, + `select`→`fields`, `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand` — + and the alias key is **dropped from the parsed output**; +- **both spellings, same value**: redundant, tolerated, alias dropped; +- **both spellings, different values**: irreconcilable — picking a winner IS + the silent drop — so the parse fails (schema) / the request is `400 + INVALID_REQUEST` (wire), naming the spellings and the canonical key; +- an explicit **`null` spelling is a withdrawal**, never a conflict: a null + alias is dropped silently, a null canonical keeps its slot-specific answer. + +`RpcQueryOptionsSchema` and the four `filter`-mixin option schemas +(update/delete/count/aggregate requests) apply the fold as a parse transform, +so parsed output speaks canonical keys only — a TS consumer reading +`parsed.query.populate` now **fails to compile** instead of silently reading +`undefined` (the #3742 / #3764 shape, one layer down; hence the minor). The +protocol normalizer folds raw wire input by the same table (extended with the +wire-only `filters` / `$filter` / `$expand` spellings), and the runtime +dispatcher's second copy of the fold is deleted outright. + +**Authoring/callers unchanged for the supported cases**: every alias alone +keeps working on every path, and identical duplicates still pass. What +changes is mixed vocabularies with **different** values — previously answered +differently per route, now refused loudly on all of them — and a direct +`expand: [names]` array on `POST /data/:object/query`, which used to be read +by its indices ("Unknown field '0'") and now lowers to the expand record like +`populate` always did. diff --git a/content/docs/references/data/data-engine.mdx b/content/docs/references/data/data-engine.mdx index 9d385c34b5..8d489137c2 100644 --- a/content/docs/references/data/data-engine.mdx +++ b/content/docs/references/data/data-engine.mdx @@ -186,7 +186,7 @@ Reference: any | :--- | :--- | :--- | :--- | | **method** | `'findOne'` | ✅ | | | **object** | `string` | ✅ | | -| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order: Enum<'asc' \| 'desc'> }[]; … }` | optional | | +| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order?: Enum<'asc' \| 'desc'> }[]; … }` | optional | | --- @@ -199,7 +199,7 @@ Reference: any | :--- | :--- | :--- | :--- | | **method** | `'find'` | ✅ | | | **object** | `string` | ✅ | | -| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order: Enum<'asc' \| 'desc'> }[]; … }` | optional | | +| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order?: Enum<'asc' \| 'desc'> }[]; … }` | optional | | --- @@ -268,7 +268,7 @@ This schema accepts one of the following structures: | :--- | :--- | :--- | :--- | | **method** | `'find'` | ✅ | | | **object** | `string` | ✅ | | -| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order: Enum<'asc' \| 'desc'> }[]; … }` | optional | | +| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order?: Enum<'asc' \| 'desc'> }[]; … }` | optional | | --- @@ -280,7 +280,7 @@ This schema accepts one of the following structures: | :--- | :--- | :--- | :--- | | **method** | `'findOne'` | ✅ | | | **object** | `string` | ✅ | | -| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order: Enum<'asc' \| 'desc'> }[]; … }` | optional | | +| **query** | `{ context?: object; where?: Record \| any; fields?: string[]; orderBy?: { field: string; order?: Enum<'asc' \| 'desc'> }[]; … }` | optional | | --- diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 6e18740669..74a36aa611 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, REFERENCE_VALUE_TYPES, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; +import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, REFERENCE_VALUE_TYPES, RPC_QUERY_ALIAS_SLOTS, foldQueryAliasSlots, type QueryAliasConflict, type QueryAliasSlot, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared'; import { applyConversionsToStoredItem } from '@objectstack/spec'; import { type FormView, isAggregatedViewContainer } from '@objectstack/spec/ui'; @@ -866,6 +866,49 @@ const ODATA_SPELLING: Readonly> = { filter: '$filter', select: '$select', expand: '$expand', }; +/** + * [#3795] The spec's alias table ({@link RPC_QUERY_ALIAS_SLOTS}) extended with + * the wire-only spellings no schema declares: `filters` (documented plural + * alias of the `filter` transport param) and the OData `$filter` / `$expand`. + * Every spelling of one QueryAST slot resolves through ONE fold — the four + * slots that used to resolve backwards (canonical consulted last), each in its + * own open-coded way, are the reason the table lives in the spec and not here. + */ +const WIRE_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = (() => { + const extra: Record = { + where: ['filters', '$filter'], + expand: ['$expand'], + }; + return RPC_QUERY_ALIAS_SLOTS.map((slot) => ({ + canonical: slot.canonical, + aliases: [...slot.aliases, ...(extra[slot.canonical] ?? [])], + })); +})(); + +/** + * [#4181 → #3795] Spellings of ONE slot carrying DIFFERENT values. Two values + * for one slot cannot be reconciled — merging them would invent an intent the + * caller never expressed, and picking one is the silent drop itself — so an + * ambiguous request is refused. Redundant identical spellings pass. #4181 + * established this on the filter slot; the fold now applies it to all five. + * + * `spellingFor` maps each folded name back to the wire spelling the caller + * actually wrote (`$orderby`, not `orderBy`) — the #4226 discipline. + */ +function conflictingQueryParamsError( + conflict: QueryAliasConflict, + spellingFor: (name: string) => string, +): Error { + const names = conflict.spellings.map((s) => `'${spellingFor(s)}'`).join(', '); + const err: any = new Error( + `Conflicting query parameters: ${names} are spellings of the same parameter ` + + `(canonical '${conflict.canonical}') and were given different values. Send exactly one.`, + ); + err.status = 400; + err.code = 'INVALID_REQUEST'; + return err; +} + /** * [#4181] A filter the normalizer cannot turn into a usable `FilterCondition` * by any route other than the array shapes {@link malformedFilterArrayError} @@ -3589,35 +3632,39 @@ export class ObjectStackProtocolImplementation implements delete options[dollar]; } - // Numeric fields — normalize top → limit, skip → offset + // [#3795] One slot, one value. Every alias spelling of the five + // QueryAST slots resolves HERE, by the spec's own table, before the + // per-slot wire coercion below ever runs — so that coercion reads + // canonical keys only. An alias alone folds into its canonical key; + // redundant identical spellings collapse; different values for one + // slot are refused (the #4181 rule, generalized from the filter slot + // to all five — four of which used to resolve BACKWARDS here, each in + // its own way, disagreeing with the spec's documented precedence and + // with the runtime dispatcher's copy of the same fold). + // + // `arrivedAs` remembers which spelling carried each slot's value; + // composed with `wireSpelling` it names the parameter the caller + // actually wrote in every rejection below (#4226). + const spellingFor = (name: string): string => wireSpelling[name] ?? name; + const arrivedAs = foldQueryAliasSlots(options, WIRE_QUERY_ALIAS_SLOTS, (conflict) => { + throw conflictingQueryParamsError(conflict, spellingFor); + }); + const slotParam = (canonical: string): string => spellingFor(arrivedAs[canonical] ?? canonical); + + // Numeric fields — normalize top → limit ($top is the OData layer, + // outside the #3795 slot table), then coerce querystring strings. if (options.top != null) { options.limit = Number(options.top); delete options.top; } - if (options.skip != null) { - options.offset = Number(options.skip); - } - // Deleted unconditionally, unlike `top` (a declared QueryAST key the - // engine aliases itself): `skip` is wire-only, so a null/undefined one - // left behind would reach the #4134 field gate below and be reported as - // an unknown FIELD — a confusing rejection for a real parameter. - delete options.skip; if (options.limit != null) options.limit = Number(options.limit); 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)) { - options.fields = options.select; - } - if (options.select !== undefined) delete options.select; - - // fields: comma-separated string → array. Clients may pass `?fields=name` - // directly (not only via the `?select=` alias above) — a single-value - // querystring param arrives as a bare string, which drivers' `.map()` + // Projection: comma-separated string → array. A single-value + // querystring param arrives as a bare string — `?fields=name` or the + // folded `?select=` / `$select` spellings — which drivers' `.map()` // calls over `query.fields` would otherwise throw on. + const projectionKey = slotParam('fields'); if (typeof options.fields === 'string') { options.fields = options.fields.split(',').map((s: string) => s.trim()).filter(Boolean); } else if (options.fields !== undefined && !Array.isArray(options.fields)) { @@ -3628,7 +3675,7 @@ export class ObjectStackProtocolImplementation implements // returned MORE than was asked for. this.assertProjectionFieldsExist(request.object, options.fields, projectionKey); - // Sort/orderBy → orderBy: every wire spelling → SortNode[]. + // Sort: every wire shape → SortNode[]. // // [#4226] `normalizeSortNodes` folds the two shapes that used to fall // through this block untouched — `string[]` and `{field: direction}` — @@ -3636,10 +3683,8 @@ export class ObjectStackProtocolImplementation implements // 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; + const sortValue = options.orderBy; + const sortKey = slotParam('orderBy'); 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. @@ -3656,41 +3701,15 @@ export class ObjectStackProtocolImplementation implements else delete options.orderBy; } - // Filter/filters/$filter → where: normalize all filter aliases. - // - // [#4181] These four names are FOUR SPELLINGS OF ONE SLOT (`filters` is - // documented as a deprecated alias of `filter`), so `??` picking the - // first non-null silently discarded the others: a body carrying both - // `where` and a different `filter` ran the `filter` and dropped the - // `where` with no signal. Two different values for one slot cannot be - // reconciled — merging them would invent an intent the caller never - // expressed, and picking one is the silent drop itself — so an - // ambiguous request is refused. Redundant identical spellings are - // harmless and pass. - const filterAliases = (['filter', 'filters', '$filter', 'where'] as const) - .filter((k) => options[k] !== undefined) - .map((k) => ({ key: k, value: options[k] })); - if (filterAliases.length > 1) { - const distinct = new Set(filterAliases.map((a) => JSON.stringify(a.value))); - if (distinct.size > 1) { - const err: any = new Error( - `Conflicting filter parameters: ${filterAliases.map((a) => `'${a.key}'`).join(', ')} ` - + 'are aliases for the same filter and were given different values. Send exactly one.', - ); - err.status = 400; - err.code = 'INVALID_REQUEST'; - throw err; - } - } - - const filterValue = options.filter ?? options.filters ?? options.$filter ?? options.where; - const filterKey = filterAliases[0]?.key ?? 'filter'; - delete options.filter; - delete options.filters; - delete options.$filter; + // Filter: the folded slot value → a usable `FilterCondition` on + // `where`, or a rejection. The four spellings of this slot + // (`where`/`filter`/`filters`/`$filter`) already resolved through the + // #3795 fold above — #4181's one-slot-one-value rule, which this block + // pioneered before the fold generalized it. + const filterKey = slotParam('where'); - if (filterValue !== undefined) { - let parsedFilter = filterValue; + if (options.where !== undefined) { + let parsedFilter = options.where; // A blank `?filter=` is ABSENT, not malformed — the same `length > 0` // guard the export route applies before parsing. Deleting `where` // here (rather than leaving `''` on it) is what lets every consumer @@ -3755,31 +3774,24 @@ export class ObjectStackProtocolImplementation implements } } - // Populate/expand/$expand → expand (Record) - const populateValue = options.populate; - const expandValue = options.$expand ?? options.expand; + // Expand: the folded slot value → `Record`. A comma + // list (string) and a name array both lower to `{name: {object: name}}`; + // the advanced `{rel: QueryAST}` map a caller may send directly on + // `POST /data/:object/query` passes through as-is. Lowering the ARRAY + // shape here (not just the string) also closes a pre-#3795 gap: a raw + // name array used to survive this block whole, so the #4226 gate read + // its INDICES as relation names and refused real requests with + // "Unknown field '0'". + const expandValue = options.expand; const expandNames: string[] = []; - if (typeof populateValue === 'string') { - expandNames.push(...populateValue.split(',').map((s: string) => s.trim()).filter(Boolean)); - } else if (Array.isArray(populateValue)) { - expandNames.push(...populateValue); - } - if (!expandNames.length && expandValue) { - if (typeof expandValue === 'string') { - expandNames.push(...expandValue.split(',').map((s: string) => s.trim()).filter(Boolean)); - } else if (Array.isArray(expandValue)) { - expandNames.push(...expandValue); - } + if (typeof expandValue === 'string') { + expandNames.push(...expandValue.split(',').map((s: string) => s.trim()).filter(Boolean)); + } else if (Array.isArray(expandValue)) { + expandNames.push(...expandValue); } - delete options.populate; - delete options.$expand; - // Clean up non-object expand (e.g. string) BEFORE the Record conversion - // below, so that populate-derived names can create the expand Record even - // when a legacy string expand was also present. - if (typeof options.expand !== 'object' || options.expand === null) { + if (typeof options.expand !== 'object' || options.expand === null || Array.isArray(options.expand)) { delete options.expand; } - // Only set expand if not already an object (advanced usage) if (expandNames.length > 0 && !options.expand) { options.expand = {} as Record; for (const rel of expandNames) { diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index 2ff0278ff1..e82357ccf4 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -119,18 +119,34 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { ); }); - it('should prefer populate names over expand string when both provided', async () => { - await protocol.findData({ - object: 'task', - query: { populate: ['assignee'], expand: 'project' }, - }); + it('refuses conflicting populate/expand values — spellings of ONE slot (#3795)', async () => { + // The deprecated `populate` used to WIN this mix (canonical + // `expand` was only consulted when populate produced nothing) — + // the documented precedence, backwards. Two different values for + // one slot cannot be reconciled, so the mix is refused outright, + // the same #4181 rule the filter slot already had. + await expect( + protocol.findData({ + object: 'task', + query: { populate: ['assignee'], expand: 'project' }, + }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + expect(mockEngine.find).not.toHaveBeenCalled(); + }); - // populate names take precedence; the non-object expand string is - // cleaned up first, then populate-derived names create the Record. - const callArgs = mockEngine.find.mock.calls[0][1]; - expect(callArgs.populate).toBeUndefined(); - expect(callArgs.$expand).toBeUndefined(); - expect(callArgs.expand).toEqual({ assignee: { object: 'assignee' } }); + it('should lower a direct expand ARRAY to the Record, not ride it raw (#3795)', async () => { + // A `{expand: ['a']}` body used to survive the block whole: the + // array is `typeof 'object'`, so the Record conversion was + // skipped and the #4226 gate read its INDICES ('0') as relation + // names — a 400 for a shape `populate` handled fine. + await protocol.findData({ object: 'task', query: { expand: ['assignee', 'project'] } }); + + expect(mockEngine.find).toHaveBeenCalledWith( + 'task', + expect.objectContaining({ + expand: { assignee: { object: 'assignee' }, project: { object: 'project' } }, + }), + ); }); it('should pass expand Record object through as-is', async () => { @@ -1157,4 +1173,146 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { } }); }); + + // ═══════════════════════════════════════════════════════════════ + // #3795 — one slot, one value, for ALL five alias pairs + // + // The spec documented `where` > `filter`, `fields` > `select`, + // `orderBy` > `sort`, `offset` > `skip`, `expand` > `populate` — in prose + // only. This normalizer implemented four of the five BACKWARDS (canonical + // consulted last), and the runtime dispatcher carried a second fold with + // the opposite precedence on three of them, so one request resolved + // differently per path. The fold now lives once, in the spec's own table + // (`RPC_QUERY_ALIAS_SLOTS`), under #4181's rule: alias alone folds, + // identical duplicates collapse, different values are refused. + // ═══════════════════════════════════════════════════════════════ + + describe('alias precedence resolves by the spec table, all five slots (#3795)', () => { + function makeProtocol() { + const engine: any = { + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn().mockResolvedValue(null), + count: vi.fn().mockResolvedValue(0), + registry: { + getObject: vi.fn((name: string) => ({ + name, + fields: { + title: { type: 'text' }, + status: { type: 'text' }, + assignee: { type: 'lookup', reference: 'user' }, + project: { type: 'lookup', reference: 'project' }, + }, + })), + }, + }; + return { protocol: new ObjectStackProtocolImplementation(engine), engine }; + } + + // Each case: [alias spelling, canonical key, alias value, + // conflicting canonical value, expected canonical output of the + // alias-alone fold]. + const SLOTS = [ + ['select', 'fields', ['title'], ['status'], ['title']], + ['skip', 'offset', 5, 0, 5], + ['sort', 'orderBy', 'title', [{ field: 'status', order: 'asc' }], [{ field: 'title', order: 'asc' }]], + ['populate', 'expand', ['assignee'], { project: { object: 'project' } }, { assignee: { object: 'assignee' } }], + ['filter', 'where', { status: 'done' }, { status: 'open' }, { status: 'done' }], + ] as const; + + it.each(SLOTS)( + '%s alone folds into %s — and the alias key is gone', + async (alias, canonical, aliasValue, _conflict, folded) => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ object: 'showcase_task', query: { [alias]: aliasValue } }); + const opts = engine.find.mock.calls[0][1]; + expect(opts[canonical]).toEqual(folded); + expect(alias in opts).toBe(false); + }, + ); + + it.each(SLOTS)( + '%s + %s with different values is refused, before the engine', + async (alias, canonical, aliasValue, conflictValue) => { + const { protocol, engine } = makeProtocol(); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { [alias]: aliasValue, [canonical]: conflictValue }, + }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + expect(engine.find).not.toHaveBeenCalled(); + }, + ); + + it.each(SLOTS)( + '%s + %s with the SAME value passes — redundancy is not ambiguity', + async (alias, canonical, aliasValue, _conflict, folded) => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { [alias]: aliasValue, [canonical]: aliasValue }, + }); + const opts = engine.find.mock.calls[0][1]; + expect(opts[canonical]).toEqual(folded); + expect(alias in opts).toBe(false); + }, + ); + + it('the rejection names the spellings the caller wrote and the canonical key', async () => { + const { protocol } = makeProtocol(); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { select: ['title'], fields: ['status'] }, + }), + ).rejects.toThrow(/'fields'.*'select'.*canonical 'fields'.*Send exactly one/s); + }); + + it("the rejection quotes the OData spelling when that is what arrived (#4226)", async () => { + const { protocol } = makeProtocol(); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { $expand: 'assignee', expand: { project: { object: 'project' } } }, + }), + ).rejects.toThrow(/'\$expand'/); + }); + + it('a canonical key alone is never touched by the fold', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { + where: { status: 'done' }, + fields: ['title'], + orderBy: [{ field: 'title', order: 'desc' }], + offset: 3, + expand: { assignee: { object: 'user' } }, + }, + }); + const opts = engine.find.mock.calls[0][1]; + expect(opts.where).toEqual({ status: 'done' }); + expect(opts.fields).toEqual(['title']); + expect(opts.orderBy).toEqual([{ field: 'title', order: 'desc' }]); + expect(opts.offset).toBe(3); + expect(opts.expand).toEqual({ assignee: { object: 'user' } }); + }); + + it('an explicit null spelling is a withdrawal, never a conflict', async () => { + // A null alias is dropped without folding (the `??` / `!= null` + // guards the fold replaced treated it as absent), and a null + // canonical does not shadow a real alias value (#4226 pinned this + // for `{orderBy: null, sort}`), so "this key intentionally + // carries nothing" never manufactures a 400. + const { protocol, engine } = makeProtocol(); + await protocol.findData({ object: 'showcase_task', query: { filter: null } }); + expect(engine.find.mock.calls[0][1].where).toBeUndefined(); + + await protocol.findData({ + object: 'showcase_task', + query: { offset: null, skip: 7 }, + }); + expect(engine.find.mock.calls[1][1].offset).toBe(7); + }); + }); }); diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index cebe9c8730..27b7dd001a 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -106,49 +106,17 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m } else { // GET /data/:object (List) if (m === 'GET') { - // ── Normalize HTTP transport params → Spec canonical (QueryAST) ── - // HTTP GET query params use transport-level names (filter, sort, top, - // skip, select, expand) which are normalized here to canonical - // QueryAST field names (where, orderBy, limit, offset, fields, - // expand) before forwarding to the data service layer. - // The protocol.ts findData() method performs a deeper normalization - // pass, but pre-normalizing here ensures the data service always receives - // Spec-canonical keys. - const normalized: Record = { ...query }; - - // filter/filters → where - // Note: `filter` is the canonical HTTP *transport* parameter name - // (see HttpFindQueryParamsSchema). It is normalized here to the - // canonical *QueryAST* field name `where` before data dispatch. - // `filters` (plural) is a deprecated alias for `filter`. - if (normalized.filter != null || normalized.filters != null) { - normalized.where = normalized.where ?? normalized.filter ?? normalized.filters; - delete normalized.filter; - delete normalized.filters; - } - // select → fields - if (normalized.select != null && normalized.fields == null) { - normalized.fields = normalized.select; - delete normalized.select; - } - // sort → orderBy - if (normalized.sort != null && normalized.orderBy == null) { - normalized.orderBy = normalized.sort; - delete normalized.sort; - } - // top → limit - if (normalized.top != null && normalized.limit == null) { - normalized.limit = normalized.top; - delete normalized.top; - } - // skip → offset - if (normalized.skip != null && normalized.offset == null) { - normalized.offset = normalized.skip; - delete normalized.skip; - } - + // HTTP transport params (filter/select/sort/top/skip, see + // HttpFindQueryParamsSchema) ride through VERBATIM. Folding them + // into QueryAST names is owned by the protocol's `findData` + // normalizer alone (#3795): this route used to carry its own copy + // of that fold, with the OPPOSITE precedence on three of the five + // alias pairs — two readers of one prose contract, the exact + // condition #3713 described. One fold, one answer; a second copy + // here could only agree by inspection. + // // Spec: returns FindDataResponse = { object, records, total?, hasMore? } - const result = await actionExec.callData(deps, 'query', { object: objectName, query: normalized }, _context.dataDriver, _context.environmentId, _context.executionContext); + const result = await actionExec.callData(deps, 'query', { object: objectName, query: { ...query } }, _context.dataDriver, _context.environmentId, _context.executionContext); return { handled: true, response: deps.success(result) }; } diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index e17970c372..eafebb7978 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -1326,9 +1326,12 @@ describe('HttpDispatcher', () => { ); expect(result.handled).toBe(true); - // top → limit and skip → offset are normalized by the dispatcher + // Wire params ride through VERBATIM (#3795): the dispatcher used to + // carry its own top→limit / skip→offset fold next to findData's — + // two copies of one precedence, disagreeing on three alias pairs. + // Folding is owned by the protocol normalizer alone now. expect(mockProtocol.findData).toHaveBeenCalledWith( - { object: 'task', query: { populate: 'assignee,project', limit: '10', offset: '0' } } + { object: 'task', query: { populate: 'assignee,project', top: '10', skip: '0' } } ); }); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 8cdb670d59..7510cba68d 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -472,6 +472,8 @@ "QUERY_CURSOR_REMOVED (const)", "QUERY_DISTINCT_REMOVED (const)", "QueryAST (type)", + "QueryAliasConflict (interface)", + "QueryAliasSlot (interface)", "QueryFilter (type)", "QueryFilterSchema (const)", "QueryInput (type)", @@ -479,6 +481,7 @@ "RAW_FILE_VALUES_CONTEXT_KEY (const)", "RECORD_SURFACE_PAGE_THRESHOLD (const)", "REFERENCE_VALUE_TYPES (const)", + "RPC_QUERY_ALIAS_SLOTS (const)", "RangeOperatorSchema (const)", "RecordFlow (type)", "RecordFlowContainer (type)", @@ -596,6 +599,7 @@ "deriveRecordSurface (function)", "effectiveOperationsArray (function)", "fieldForm (const)", + "foldQueryAliasSlots (function)", "formatUnknownAuthoringKey (function)", "hasDynamicTokens (function)", "hookForm (const)", diff --git a/packages/spec/src/data/data-engine.test.ts b/packages/spec/src/data/data-engine.test.ts index 063a737386..14003c03e8 100644 --- a/packages/spec/src/data/data-engine.test.ts +++ b/packages/spec/src/data/data-engine.test.ts @@ -463,7 +463,7 @@ describe('DataEngineFindRequestSchema', () => { expect(request.query?.where).toBeDefined(); }); - it('should accept find with legacy params (backward compat)', () => { + it('folds legacy params into their canonical keys and drops the aliases (#3795)', () => { const request = DataEngineFindRequestSchema.parse({ method: 'find', object: 'account', @@ -476,10 +476,69 @@ describe('DataEngineFindRequestSchema', () => { }, }); - expect(request.query?.filter).toEqual({ status: 'active' }); - expect(request.query?.select).toEqual(['id', 'name']); - expect(request.query?.skip).toBe(10); - expect(request.query?.populate).toEqual(['owner']); + // Legacy input stays accepted (backward compat) — but the parsed output + // speaks canonical QueryAST only, so no consumer re-implements precedence. + expect(request.query?.where).toEqual({ status: 'active' }); + expect(request.query?.fields).toEqual(['id', 'name']); + expect(request.query?.orderBy).toEqual([{ field: 'name', order: 'asc' }]); + expect(request.query?.offset).toBe(10); + expect(request.query?.expand).toEqual({ owner: { object: 'owner' } }); + for (const alias of ['filter', 'select', 'sort', 'skip', 'populate']) { + expect(request.query && alias in request.query).toBe(false); + } + }); + + it('lowers the record sort spellings while folding (#3795)', () => { + const named = DataEngineFindRequestSchema.parse({ + method: 'find', + object: 'account', + query: { sort: { name: 'desc' } }, + }); + expect(named.query?.orderBy).toEqual([{ field: 'name', order: 'desc' }]); + + const numeric = DataEngineFindRequestSchema.parse({ + method: 'find', + object: 'account', + query: { sort: { created_at: 1, name: -1 } }, + }); + expect(numeric.query?.orderBy).toEqual([ + { field: 'created_at', order: 'asc' }, + { field: 'name', order: 'desc' }, + ]); + }); + + it('tolerates a redundant identical spelling, refuses a conflicting one (#3795)', () => { + const redundant = DataEngineFindRequestSchema.parse({ + method: 'find', + object: 'account', + query: { fields: ['id'], select: ['id'] }, + }); + expect(redundant.query?.fields).toEqual(['id']); + expect(redundant.query && 'select' in redundant.query).toBe(false); + + const conflict = DataEngineFindRequestSchema.safeParse({ + method: 'find', + object: 'account', + query: { fields: ['id'], select: ['name'] }, + }); + expect(conflict.success).toBe(false); + if (!conflict.success) { + expect(conflict.error.issues[0].message).toMatch(/Send exactly one/); + } + }); + + it('refuses a conflicting value on every alias pair (#3795)', () => { + const cases: Array> = [ + { filter: { a: 1 }, where: { b: 2 } }, + { select: ['a'], fields: ['b'] }, + { sort: [{ field: 'a', order: 'asc' }], orderBy: [{ field: 'b', order: 'asc' }] }, + { skip: 1, offset: 2 }, + { populate: ['a'], expand: { b: { object: 'b' } } }, + ]; + for (const query of cases) { + const result = DataEngineFindRequestSchema.safeParse({ method: 'find', object: 'account', query }); + expect(result.success, `${Object.keys(query).join('+')} must be refused`).toBe(false); + } }); }); @@ -496,7 +555,7 @@ describe('DataEngineFindOneRequestSchema', () => { expect(request.method).toBe('findOne'); }); - it('should accept find one with legacy params (backward compat)', () => { + it('folds legacy params into canonical keys (backward compat input, #3795)', () => { const request = DataEngineFindOneRequestSchema.parse({ method: 'findOne', object: 'account', @@ -506,7 +565,9 @@ describe('DataEngineFindOneRequestSchema', () => { }, }); - expect(request.query?.filter).toEqual({ id: '123' }); + expect(request.query?.where).toEqual({ id: '123' }); + expect(request.query?.fields).toEqual(['id', 'name']); + expect(request.query && 'filter' in request.query).toBe(false); }); }); @@ -561,7 +622,7 @@ describe('DataEngineUpdateRequestSchema', () => { expect(request.id).toBe('123'); }); - it('should accept update with legacy filter (backward compat)', () => { + it('folds a legacy filter into where (backward compat input, #3795)', () => { const request = DataEngineUpdateRequestSchema.parse({ method: 'update', object: 'account', @@ -572,7 +633,8 @@ describe('DataEngineUpdateRequestSchema', () => { }, }); - expect(request.options?.filter).toEqual({ category: 'premium' }); + expect(request.options?.where).toEqual({ category: 'premium' }); + expect(request.options && 'filter' in request.options).toBe(false); expect(request.options?.multi).toBe(true); }); @@ -603,7 +665,7 @@ describe('DataEngineDeleteRequestSchema', () => { expect(request.id).toBe('123'); }); - it('should accept delete with legacy filter (backward compat)', () => { + it('folds a legacy filter into where (backward compat input, #3795)', () => { const request = DataEngineDeleteRequestSchema.parse({ method: 'delete', object: 'account', @@ -613,7 +675,8 @@ describe('DataEngineDeleteRequestSchema', () => { }, }); - expect(request.options?.filter).toEqual({ status: 'archived' }); + expect(request.options?.where).toEqual({ status: 'archived' }); + expect(request.options && 'filter' in request.options).toBe(false); expect(request.options?.multi).toBe(true); }); @@ -654,7 +717,7 @@ describe('DataEngineCountRequestSchema', () => { expect(request.query?.where).toBeDefined(); }); - it('should accept count with legacy filter (backward compat)', () => { + it('folds a legacy filter into where (backward compat input, #3795)', () => { const request = DataEngineCountRequestSchema.parse({ method: 'count', object: 'account', @@ -663,7 +726,17 @@ describe('DataEngineCountRequestSchema', () => { }, }); - expect(request.query?.filter).toEqual({ status: 'active' }); + expect(request.query?.where).toEqual({ status: 'active' }); + expect(request.query && 'filter' in request.query).toBe(false); + }); + + it('refuses filter + where with different values (#3795)', () => { + const result = DataEngineCountRequestSchema.safeParse({ + method: 'count', + object: 'account', + query: { filter: { a: 1 }, where: { b: 2 } }, + }); + expect(result.success).toBe(false); }); }); diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index 34cb16a371..f3d1c4c453 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -383,24 +383,186 @@ export const DataEngineContractSchema = lazySchema(() => z.object({ * separate microservice or plugin. */ +/** + * One RPC query-options slot: the canonical QueryAST key plus the deprecated + * alias spellings that fold into it. + */ +export interface QueryAliasSlot { + /** Canonical QueryAST key the slot's value lands on. */ + canonical: string; + /** Accepted alias spellings, in report order. */ + aliases: readonly string[]; +} + +/** + * A slot whose spellings arrived with different values — irreconcilable, + * reported instead of silently resolved (see {@link foldQueryAliasSlots}). + */ +export interface QueryAliasConflict { + /** Canonical key of the slot the spellings collided on. */ + canonical: string; + /** Every spelling present on the input, canonical first when present. */ + spellings: string[]; +} + +/** + * The five alias pairs the RPC query surface accepts (#3795) — the ONE place + * the alias → canonical mapping is declared. The schema transform below folds + * parsed input by this table, and the protocol normalizer + * (`metadata-protocol`) folds raw wire input by the same table, extended with + * the wire-only spellings `filters` / `$filter` / `$expand` that no schema + * declares. Before this table existed the precedence lived in prose only, so + * every reader re-implemented it — the #3713 condition — and the two readers + * disagreed on three of the five pairs, four of them backwards. + */ +export const RPC_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = [ + { canonical: 'where', aliases: ['filter'] }, + { canonical: 'fields', aliases: ['select'] }, + { canonical: 'orderBy', aliases: ['sort'] }, + { canonical: 'offset', aliases: ['skip'] }, + { canonical: 'expand', aliases: ['populate'] }, +]; + +/** + * Fold alias spellings into their canonical slot key, in place. + * + * Per slot: an alias alone moves to the canonical key; redundant IDENTICAL + * spellings (by JSON value) collapse into the canonical key; different values + * for one slot are irreconcilable — merging would invent an intent the caller + * never expressed, and picking a winner IS the silent drop (#4181) — so the + * slot is reported via `onConflict` and left unfolded. Alias keys are always + * deleted on a successful fold, so downstream readers see canonical keys only. + * + * An explicit `null` spelling is a WITHDRAWAL, not a value: a null alias is + * deleted without folding (the `??` / `!= null` guards this fold replaced + * treated it as absent), and a null canonical stays put for the slot's own + * value handling to answer — the filter slot rejects it (#4181), the sort + * slot ignores it (#4226) — so folding never manufactures a conflict out of + * "this key intentionally carries nothing". + * + * Values are moved verbatim — a folded value may still carry the alias's + * legacy SHAPE (e.g. `sort`'s `{field: 'asc'}` record form), which the caller + * lowers after folding. + * + * Returns the spelling each folded slot's value arrived under + * (canonical key → spelling), so a later rejection can quote the parameter + * the caller actually wrote (#4226). + */ +export function foldQueryAliasSlots( + options: Record, + slots: readonly QueryAliasSlot[], + onConflict: (conflict: QueryAliasConflict) => void, +): Record { + const arrivedAs: Record = {}; + for (const slot of slots) { + const spellings = [slot.canonical, ...slot.aliases]; + const present = spellings.filter((s) => options[s] != null); + if (present.length > 1) { + const distinct = new Set(present.map((s) => JSON.stringify(options[s]))); + if (distinct.size > 1) { + // Left as-is: the caller either throws (wire) or fails the parse + // (schema transform), so the unfolded state is never observed. + onConflict({ canonical: slot.canonical, spellings: present }); + continue; + } + } + if (present.length > 0) { + const value = options[present[0]]; + options[slot.canonical] = value; + arrivedAs[slot.canonical] = present[0]; + } + for (const spelling of spellings) { + if (spelling !== slot.canonical) delete options[spelling]; + } + } + return arrivedAs; +} + +/** The `where` slot alone — for RPC options that accept only the `filter` alias. */ +const RPC_WHERE_SLOT: readonly QueryAliasSlot[] = RPC_QUERY_ALIAS_SLOTS.filter( + (slot) => slot.canonical === 'where', +); + +function aliasConflictIssue(conflict: QueryAliasConflict): { + code: 'custom'; + path: string[]; + message: string; +} { + return { + code: 'custom', + path: [conflict.canonical], + message: + `Conflicting query parameters: ${conflict.spellings.map((s) => `'${s}'`).join(', ')} ` + + `are spellings of the same parameter (canonical '${conflict.canonical}') and were ` + + 'given different values. Send exactly one.', + }; +} + /** * RPC backward-compatibility mixin — shared `@deprecated filter` field. - * When both `filter` and `where` are present, the protocol/engine ignores - * `filter` in favor of `where`; only one should be provided. + * The parse transform folds `filter` into `where` and drops it; both spellings + * with different values fail the parse (see `RpcQueryOptionsSchema`). */ const RpcLegacyFilterMixin = { /** @deprecated Use `where` */ filter: DataEngineFilterSchema.optional(), }; +/** + * Parse-time fold for options that accept only the `filter` alias + * ({@link RpcLegacyFilterMixin}): `filter` lands on `where` and is dropped + * from the parsed output, so `filter` is absent from the inferred type and a + * TS consumer reading it fails to compile instead of silently reading + * `undefined` (the #3742 / #3764 shape, one layer down). + */ +function foldRpcLegacyFilter( + input: T, + ctx: z.core.$RefinementCtx, +): Omit { + const bag: Record = { ...input }; + foldQueryAliasSlots(bag, RPC_WHERE_SLOT, (conflict) => ctx.addIssue(aliasConflictIssue(conflict))); + return bag as Omit; +} + +/** + * Parse-time fold for the full RPC query options: each legacy alias lands on + * its canonical key (with the alias's legacy value shape lowered to the + * canonical one) and is dropped from the parsed output. + */ +function foldRpcQueryOptions(input: object, ctx: z.core.$RefinementCtx): EngineQueryOptions { + const bag = { ...(input as Record) }; + foldQueryAliasSlots(bag, RPC_QUERY_ALIAS_SLOTS, (conflict) => ctx.addIssue(aliasConflictIssue(conflict))); + // A folded `sort` may carry the record spellings `DataEngineSortSchema` + // allows; canonical `orderBy` declares `SortNode[]` only, so lower them. + if (bag.orderBy !== undefined && bag.orderBy !== null && !Array.isArray(bag.orderBy)) { + bag.orderBy = Object.entries(bag.orderBy as Record).map( + ([field, order]) => ({ field, order: order === 'asc' || order === 1 ? 'asc' : 'desc' }), + ); + } + // A folded `populate` is a relation-name list; canonical `expand` is a + // `{name: QueryAST}` record. + if (Array.isArray(bag.expand)) { + bag.expand = Object.fromEntries( + (bag.expand as string[]).map((rel) => [rel, { object: rel }]), + ); + } + return bag as EngineQueryOptions; +} + /** * RPC query options that accept BOTH new (where/fields/orderBy) and * legacy (filter/select/sort/skip/populate) parameter names. - * - * **Precedence:** When both legacy and new keys are present for the same - * concern, the protocol normalizer uses the new key (`where` > `filter`, - * `fields` > `select`, `orderBy` > `sort`, `offset` > `skip`, - * `expand` > `populate`). Callers should not mix vocabularies. + * + * **One slot, one value (#3795):** each legacy alias is folded into its + * canonical key at parse — `filter`→`where`, `select`→`fields`, + * `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand` + * ({@link RPC_QUERY_ALIAS_SLOTS}) — and the alias is dropped from the parsed + * output, so consumers only ever read canonical QueryAST keys. Sending both + * spellings with the SAME value is redundant and tolerated; sending DIFFERENT + * values for one slot is irreconcilable — picking either would silently drop + * the other (#4181) — and fails the parse. The protocol normalizer applies + * the same table to raw wire input, so mixed vocabularies resolve identically + * on every path. */ const RpcQueryOptionsSchema = EngineQueryOptionsSchema.extend({ ...RpcLegacyFilterMixin, @@ -412,7 +574,7 @@ const RpcQueryOptionsSchema = EngineQueryOptionsSchema.extend({ skip: z.number().int().min(0).optional(), /** @deprecated Use `expand` */ populate: z.array(z.string()).optional(), -}); +}).transform((options, ctx) => foldRpcQueryOptions(options, ctx)); export const DataEngineFindRequestSchema = lazySchema(() => z.object({ method: z.literal('find'), @@ -438,26 +600,30 @@ export const DataEngineUpdateRequestSchema = lazySchema(() => z.object({ object: z.string(), data: z.record(z.string(), z.unknown()), id: z.union([z.string(), z.number()]).optional().describe('ID for single update, or use where in options'), - options: EngineUpdateOptionsSchema.extend(RpcLegacyFilterMixin).optional() + options: EngineUpdateOptionsSchema.extend(RpcLegacyFilterMixin) + .transform((options, ctx) => foldRpcLegacyFilter(options, ctx)).optional() })); export const DataEngineDeleteRequestSchema = lazySchema(() => z.object({ method: z.literal('delete'), object: z.string(), id: z.union([z.string(), z.number()]).optional().describe('ID for single delete, or use where in options'), - options: EngineDeleteOptionsSchema.extend(RpcLegacyFilterMixin).optional() + options: EngineDeleteOptionsSchema.extend(RpcLegacyFilterMixin) + .transform((options, ctx) => foldRpcLegacyFilter(options, ctx)).optional() })); export const DataEngineCountRequestSchema = lazySchema(() => z.object({ method: z.literal('count'), object: z.string(), - query: EngineCountOptionsSchema.extend(RpcLegacyFilterMixin).optional() + query: EngineCountOptionsSchema.extend(RpcLegacyFilterMixin) + .transform((options, ctx) => foldRpcLegacyFilter(options, ctx)).optional() })); export const DataEngineAggregateRequestSchema = lazySchema(() => z.object({ method: z.literal('aggregate'), object: z.string(), query: EngineAggregateOptionsSchema.extend(RpcLegacyFilterMixin) + .transform((options, ctx) => foldRpcLegacyFilter(options, ctx)) })); /**