diff --git a/.changeset/client-find-canonical-only-key-predicate.md b/.changeset/client-find-canonical-only-key-predicate.md new file mode 100644 index 0000000000..02476062c7 --- /dev/null +++ b/.changeset/client-find-canonical-only-key-predicate.md @@ -0,0 +1,56 @@ +--- +"@objectstack/client": patch +--- + +fix(client): `data.find({ limit })` reached the server as an empty query, and `QueryOptionsV2.expand` reached it as nothing at all (#6322) + +`data.find()` accepts two vocabularies — the canonical `QueryOptionsV2` +(`where` / `fields` / `orderBy` / `limit` / `offset` / `expand`) and the legacy +`QueryOptions` (`filter` / `select` / `sort` / `top` / `skip`) — and picked the +branch with a hand-written condition that named four keys: +`'where' in options || 'fields' in options || 'orderBy' in options || 'offset' in options`. +That condition was a second, independent statement of what `QueryOptionsV2` +declares, and it had fallen behind the interface twice. + +**`limit` was missing from it.** `client.data.find('task', { limit: 20 })` — a +canonical key as the only key, and the most natural spelling of "first 20" — +was not recognised as canonical, fell to the legacy branch, and that branch +reads only `top` / `skip` / `sort` / `select` / `filter` / `filters` / +`aggregations` / `groupBy`. Nothing there reads `limit`, so the value was +dropped between the call and the wire: the request went out with an **empty +query string**, the caller got the server's default page size, HTTP 200, no +warning. Its pagination twin `{ offset: 5 }` worked correctly, because `offset` +happened to be one of the four listed keys — one interface, two pagination +keys, opposite behaviour. + +**`expand` was missing too, and had no mapping either.** It is declared on +`QueryOptionsV2`, documented as the replacement for a legacy `populate` that +`QueryOptions` never had, and was carried by neither branch — not one character +of it reached the wire, on either of the two `find` implementations. + +**What changed.** The branch predicate is now derived from the interface rather +than restated beside it: the canonical-only key set is +`Exclude`, held as a +`Record<…, true>` that TypeScript rejects when a key is missing or extra. A key +added to `QueryOptionsV2` from now on is a compile error until it is listed, so +the next canonical key is covered on the day it is declared. Appending `limit` +to the old list would have been the third round of the same mistake. + +`expand` now maps onto the spelling the server actually accepts: +`?expand=`, which +`HttpFindQueryParamsSchema` declares for the GET list route and the protocol +normalizer splits on commas before folding each name into the engine's expand +map. The `Record` form contributes its keys — the same relation names the +server derives from the comma list. A **nested** per-relation query inside +`expand` has no spelling on a GET, so it is now refused with an error naming +the relation and the keys it could not carry, rather than trimmed away +silently; `data.query()` carries a QueryAST body and is where nested expand +detail belongs. + +Both `find` implementations — `ObjectStackClient.data.find` and +`ScopedProjectClient.data.find`, which were byte-identical copies of the same +defect — read the one shared predicate and the one shared `expand` mapping. + +No change to the five paired keys: canonical and legacy spellings of the same +query still produce byte-identical transport parameters, and that parity is now +pinned by a test table both implementations are driven through. diff --git a/packages/client/src/client.test.ts b/packages/client/src/client.test.ts index 5cd9660ed3..45e03217c4 100644 --- a/packages/client/src/client.test.ts +++ b/packages/client/src/client.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect, vi } from 'vitest'; // entered a tsc program (#5449). import { WELL_KNOWN_CAPABILITY_KEYS } from '@objectstack/spec/api'; import { ObjectStackClient, createQuery, createFilter } from './index'; +import type { QueryOptions, QueryOptionsV2 } from './index'; /** Helper: create a client with mocked fetch that returns the given response body */ function createMockClient(body: any, status = 200) { @@ -1226,50 +1227,181 @@ describe('ObjectStackClient.automation', () => { // QueryOptionsV2 (Canonical Query Syntax) Tests // ========================================== -describe('QueryOptionsV2 — canonical find()', () => { - it('should accept canonical field names (where, fields, orderBy, limit, offset)', async () => { - const { client, fetchMock } = createMockClient({ - success: true, - data: { object: 'account', records: [], total: 0 } - }); +/** + * `data.find()` transport parameters — ONE expectation table, BOTH copies. + * + * `find` is implemented twice: `ObjectStackClient.data.find` and + * `ScopedProjectClient.data.find`. They are two faces of ONE wire contract + * (the scoped one differs only in the URL prefix) and were byte-identical + * copies of the same normalization — including the same defect. Every row + * below is therefore driven through BOTH and compared against the SAME + * expected query string, so a future edit that lands on only one of them goes + * red here instead of shipping a fork. + * + * The expectations are EXACT full query strings, not `toContain` substrings: + * the defect this suite was written for (#6322) was a param that never + * appeared at all, and a substring assertion on the params that DID appear + * stays green through exactly that. + * + * Seeded from the measurements in #6322. + */ +describe('data.find() — canonical/legacy transport parameters (both copies)', () => { + /** The query string a call put on the wire (`''` when it sent none). */ + function queryOf(url: string): string { + const q = url.indexOf('?'); + return q === -1 ? '' : url.slice(q + 1); + } - await client.data.find('account', { - where: { status: 'active' }, - fields: ['name', 'email'], - orderBy: ['-created_at'], - limit: 10, - offset: 5, + /** Drive the SAME options through both `find` implementations. */ + async function driveBoth(options: QueryOptions | QueryOptionsV2): Promise<{ direct: string; scoped: string }> { + const body = { success: true, data: { object: 'task', records: [], total: 0 } }; + + const a = createMockClient(body); + await a.client.data.find('task', options); + const direct = queryOf(a.fetchMock.mock.calls[0][0] as string); + + const b = createMockClient(body); + await b.client.project('env-1').data.find('task', options); + const scoped = queryOf(b.fetchMock.mock.calls[0][0] as string); + + return { direct, scoped }; + } + + interface TransportRow { + options: QueryOptions | QueryOptionsV2; + /** The exact query string both copies must emit. */ + wire: string; + } + + /** + * A key declared on `QueryOptionsV2` and on NO legacy `QueryOptions` + * spelling — recomputed here from the two exported interfaces rather than + * restated, so it tracks them. + */ + type CanonicalOnlyKey = Exclude; + + /** + * ONE ROW PER CANONICAL-ONLY KEY, DRIVEN ALONE. This is the shape of the + * #6322 defect: `find('task', { limit: 20 })` — a canonical key as the + * ONLY key — was not recognised as canonical vocabulary, fell to the + * legacy branch, which reads no `limit`, and reached the server with an + * EMPTY query string. HTTP 200, server default page size, no warning. + * Its pagination twin `{ offset: 5 }` worked, because `offset` happened to + * be in the hand-written sniff list and `limit` did not. + * + * `Record` is the anti-rot device: a new key added to + * `QueryOptionsV2` makes this object a COMPILE error until someone states + * what that key puts on the wire — which is the question the old + * hand-maintained sniff list let two keys (`limit`, `expand`) slip past. + */ + const SINGLE_CANONICAL_KEY: Record = { + where: { options: { where: { contact_id: 'c1' } }, wire: 'contact_id=c1' }, + fields: { options: { fields: ['id', 'amount'] }, wire: 'select=id%2Camount' }, + orderBy: { options: { orderBy: ['-created_at'] }, wire: 'sort=-created_at' }, + limit: { options: { limit: 20 }, wire: 'top=20' }, + offset: { options: { offset: 5 }, wire: 'skip=5' }, + expand: { options: { expand: ['contact'] }, wire: 'expand=contact' }, + }; + + /** The five paired keys, canonical spelling. */ + const CANONICAL_FULL: QueryOptionsV2 = { + where: { contact_id: 'c1' }, + fields: ['id', 'amount'], + orderBy: ['-created_at'], + limit: 20, + offset: 5, + }; + + /** The same query, legacy spelling. Must reach the wire byte-identically. */ + const LEGACY_FULL: QueryOptions = { + filter: { contact_id: 'c1' }, + select: ['id', 'amount'], + sort: ['-created_at'], + top: 20, + skip: 5, + }; + + const ROWS: Record = { + 'canonical: where + fields + orderBy + limit + offset': { + options: CANONICAL_FULL, + wire: 'top=20&skip=5&sort=-created_at&select=id%2Camount&contact_id=c1', + }, + 'legacy: filter + select + sort + top + skip': { + options: LEGACY_FULL, + wire: 'top=20&skip=5&sort=-created_at&select=id%2Camount&contact_id=c1', + }, + ...Object.fromEntries( + Object.entries(SINGLE_CANONICAL_KEY).map(([key, row]) => [`canonical single key: { ${key} }`, row]), + ), + // The legacy twins of the two pagination keys — pinned alongside so a + // change that fixes one vocabulary by breaking the other goes red. + 'legacy single key: { top }': { options: { top: 20 }, wire: 'top=20' }, + 'legacy single key: { skip }': { options: { skip: 5 }, wire: 'skip=5' }, + 'canonical: where + limit': { + options: { where: { contact_id: 'c1' }, limit: 20 }, + wire: 'top=20&contact_id=c1', + }, + 'canonical: where + expand': { + options: { where: { contact_id: 'c1' }, expand: ['contact'] }, + wire: 'contact_id=c1&expand=contact', + }, + // `expand` reaches the wire as `expand=` — the one spelling `HttpFindQueryParamsSchema` declares for + // the GET list route and the protocol normalizer splits on commas. + 'canonical: expand as a name array': { + options: { expand: ['contact', 'owner'] }, + wire: 'expand=contact%2Cowner', + }, + // The `Record` form contributes its KEYS — the same relation names the + // server derives from the comma list. + 'canonical: expand as a relation map': { + options: { expand: { contact: {}, owner: {} } }, + wire: 'expand=contact%2Cowner', + }, + 'no options at all': { options: {}, wire: '' }, + }; + + for (const [name, row] of Object.entries(ROWS)) { + it(`${name} → \`${row.wire}\` on both copies`, async () => { + const { direct, scoped } = await driveBoth(row.options); + expect(direct).toBe(row.wire); + expect(scoped).toBe(row.wire); }); + } - const url = fetchMock.mock.calls[0][0] as string; - // V2 canonical options are normalized to HTTP transport params - expect(url).toContain('top=10'); - expect(url).toContain('skip=5'); - expect(url).toContain('select=name%2Cemail'); - expect(url).toContain('sort=-created_at'); - // where → filter as JSON - expect(url).toContain('status=active'); + it('canonical and legacy spellings of one query are byte-identical on the wire', async () => { + const canonical = await driveBoth(CANONICAL_FULL); + const legacy = await driveBoth(LEGACY_FULL); + expect(canonical.direct).toBe(legacy.direct); + expect(canonical.scoped).toBe(legacy.scoped); }); - it('should still accept legacy field names (filter, select, sort, top, skip)', async () => { - const { client, fetchMock } = createMockClient({ - success: true, - data: { object: 'account', records: [], total: 0 } - }); + /** + * A nested per-relation query inside `expand` has no spelling on a GET, so + * it is REFUSED rather than trimmed away — trimming would send a wider + * read than the caller asked for and say nothing. + * + * This is a client-side pre-flight guard, not an HTTP rejection, so there + * is no ADR-0112 `code`/`status` envelope to assert. The two independent + * bits asserted instead: the message names the offending relation AND the + * nested keys it could not carry, and NO request was issued — so a + * "refusal" that fired after the read had already gone out, or one that + * resolved instead of throwing, both go red. + */ + it('refuses a nested per-relation expand on both copies, before any request', async () => { + const nested: QueryOptionsV2 = { expand: { contact: { fields: ['name'] } } }; - await client.data.find('account', { - filter: { industry: 'Tech' }, - select: ['name'], - sort: ['-revenue'], - top: 20, - skip: 0, - }); + const a = createMockClient({ success: true, data: { object: 'task', records: [] } }); + await expect(a.client.data.find('task', nested)).rejects.toThrow( + /expand\['contact'\] carries a nested query \(fields\)/, + ); + expect(a.fetchMock).not.toHaveBeenCalled(); - const url = fetchMock.mock.calls[0][0] as string; - expect(url).toContain('top=20'); - expect(url).toContain('select=name'); - expect(url).toContain('sort=-revenue'); - expect(url).toContain('industry=Tech'); + const b = createMockClient({ success: true, data: { object: 'task', records: [] } }); + await expect(b.client.project('env-1').data.find('task', nested)).rejects.toThrow( + /expand\['contact'\] carries a nested query \(fields\)/, + ); + expect(b.fetchMock).not.toHaveBeenCalled(); }); }); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index d2f819c3e8..083256a677 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -190,6 +190,118 @@ export interface QueryOptionsV2 { groupBy?: string[]; } +/** + * [#6322] A key that exists on {@link QueryOptionsV2} and on NO spelling of the + * legacy {@link QueryOptions} — computed by the type system, not restated by + * hand. Presence of any one of them is what tells `data.find()` the caller + * wrote canonical vocabulary. + * + * `aggregations` / `groupBy` are excluded automatically because both options + * types declare them: shared vocabulary cannot discriminate between the two. + */ +type QueryOptionsV2OnlyKey = Exclude; + +/** + * Every canonical-only key, exhaustively. + * + * WHY A `Record` AND NOT AN ARRAY OF STRINGS. + * `data.find()` used to sniff the branch with a hand-written inline condition + * (`'where' in options || 'fields' in options || 'orderBy' in options || + * 'offset' in options`), duplicated verbatim in both `find` copies. A + * hand-written list is a second, independent statement of what + * `QueryOptionsV2` declares, and it had already fallen behind that declaration + * TWICE: + * + * - `limit` was never in it, so `find('task', { limit: 20 })` — the most + * natural canonical spelling of "first 20" — fell to the legacy branch, + * which reads only `top`/`skip`/`sort`/`select`/`filter`/`filters`/ + * `aggregations`/`groupBy`. Nothing there reads `limit`, so the value was + * dropped between the call and the wire: HTTP 200, server default page + * size, no warning. Its pagination twin `offset` WAS in the list, so one + * interface shipped two pagination keys with opposite behaviour. + * - `expand` was never in it either (see {@link canonicalExpandParam}). + * + * Appending the missing key would have been the third round of the same + * mistake. This shape cannot fall behind: TypeScript rejects the object if a + * canonical-only key is MISSING and rejects it if a key that is not + * canonical-only is present, so the next key added to `QueryOptionsV2` is a + * compile error here until it is listed — and both `find` copies pick it up + * from this one definition on the same commit. + */ +const QUERY_OPTIONS_V2_ONLY_KEYS: Record = { + where: true, + fields: true, + orderBy: true, + limit: true, + offset: true, + expand: true, +}; + +/** + * Does this options bag speak canonical {@link QueryOptionsV2} vocabulary? + * + * ONE definition read by both `data.find()` implementations + * (`ObjectStackClient` and `ScopedProjectClient`), which are two faces of one + * wire contract and were byte-identical copies of the old inline condition. + */ +function isCanonicalQueryOptions(options: QueryOptions | QueryOptionsV2): options is QueryOptionsV2 { + return Object.keys(QUERY_OPTIONS_V2_ONLY_KEYS).some((key) => key in options); +} + +/** + * `QueryOptionsV2.expand` → the `?expand=` transport spelling, or `undefined` + * when there is nothing to expand. + * + * THE ACCEPTED SPELLING, verified against the server rather than invented: + * `HttpFindQueryParamsSchema` (packages/spec/src/api/protocol.zod.ts) declares + * `expand` on the GET list route as a "Comma-separated list of + * lookup/master_detail field names to expand", and the protocol normalizer + * (packages/metadata-protocol/src/protocol.ts, `findData`) splits that string + * on commas and folds each name into `{ [name]: { object: name } }` before the + * engine batch-loads it. So a comma-joined name list is not one encoding among + * several — it is the only shape this route reads. + * + * Until #6322 `expand` was declared on `QueryOptionsV2`, documented as the + * replacement for a legacy `populate` that `QueryOptions` never had, and mapped + * on NEITHER branch: not one character of it reached the wire. + * + * WHY A NESTED PER-RELATION QUERY IS REFUSED RATHER THAN TRIMMED. The `Record` + * form's KEYS are relation names — exactly what the server derives from the + * comma list, so they map losslessly. Its VALUES are nested QueryASTs, and a + * query string has no spelling for them: trimming them away would deliver a + * wider read than the caller asked for and say nothing, which is the same + * silent-drop defect this function exists to close (and the one the engine + * itself refused inside `expand` in #4371). `data.query()` carries a QueryAST + * body and is where nested expand detail belongs. + */ +function canonicalExpandParam(expand: QueryOptionsV2['expand']): string | undefined { + if (expand == null) return undefined; + + let names: string[]; + if (Array.isArray(expand)) { + names = expand.map((name) => String(name).trim()).filter(Boolean); + } else if (typeof expand === 'object') { + for (const [relation, nested] of Object.entries(expand)) { + const nestedKeys = nested != null && typeof nested === 'object' && !Array.isArray(nested) + ? Object.keys(nested as Record) + : []; + if (nestedKeys.length > 0) { + throw new Error( + `data.find(): expand['${relation}'] carries a nested query (${nestedKeys.slice().sort().join(', ')}), ` + + 'but the list route accepts only `expand=` — a nested per-relation ' + + 'query has no transport spelling on a GET, so it would be dropped rather than applied. ' + + 'Pass relation names only, or use data.query() with a QueryAST `expand` for nested detail.', + ); + } + } + names = Object.keys(expand).map((name) => name.trim()).filter(Boolean); + } else { + return undefined; + } + + return names.length > 0 ? names.join(',') : undefined; +} + export interface PaginatedResult { /** Spec-compliant: array of matching records */ records: T[]; @@ -4083,16 +4195,24 @@ export class ObjectStackClient { const queryParams = new URLSearchParams(); // ── Normalize V2 canonical options → HTTP transport params ─── - // Detect V2 options by presence of canonical-only keys. + // Detect V2 options by presence of canonical-only keys. The predicate + // is derived from QueryOptionsV2 itself and SHARED with the copy of + // this method on ScopedProjectClient — see QUERY_OPTIONS_V2_ONLY_KEYS + // for why an inline hand-written key list is not allowed here (#6322). const v2 = options as QueryOptionsV2; const normalizedOptions: QueryOptions = {} as QueryOptions; - if ('where' in options || 'fields' in options || 'orderBy' in options || 'offset' in options) { + let expandParam: string | undefined; + if (isCanonicalQueryOptions(options)) { // V2 canonical options detected — map to legacy HTTP transport keys if (v2.where) normalizedOptions.filter = v2.where as any; if (v2.fields) normalizedOptions.select = v2.fields; if (v2.orderBy) normalizedOptions.sort = v2.orderBy as any; if (v2.limit != null) normalizedOptions.top = v2.limit; if (v2.offset != null) normalizedOptions.skip = v2.offset; + // `expand` has no legacy QueryOptions counterpart to normalize INTO + // (QueryOptions never had `populate`), so it goes straight to its + // own transport param below. + expandParam = canonicalExpandParam(v2.expand); if (v2.aggregations) normalizedOptions.aggregations = v2.aggregations; if (v2.groupBy) normalizedOptions.groupBy = v2.groupBy; } else { @@ -4149,6 +4269,11 @@ export class ObjectStackClient { queryParams.set('groupBy', normalizedOptions.groupBy.join(',')); } + // 6. Handle Expand (canonical-only — see canonicalExpandParam) + if (expandParam) { + queryParams.set('expand', expandParam); + } + const res = await this.fetch(`${this.baseUrl}${route}/${object}?${queryParams.toString()}`); return this.unwrapResponse>(res); }, @@ -4761,14 +4886,19 @@ export class ScopedProjectClient { find: async (object: string, options: QueryOptions | QueryOptionsV2 = {}): Promise> => { const queryParams = new URLSearchParams(); + // Same normalization as ObjectStackClient.data.find — one shared + // predicate and one shared expand mapping, so the two copies cannot + // diverge on which vocabulary a bag speaks (#6322). const v2 = options as QueryOptionsV2; const normalizedOptions: QueryOptions = {} as QueryOptions; - if ('where' in options || 'fields' in options || 'orderBy' in options || 'offset' in options) { + let expandParam: string | undefined; + if (isCanonicalQueryOptions(options)) { if (v2.where) normalizedOptions.filter = v2.where as any; if (v2.fields) normalizedOptions.select = v2.fields; if (v2.orderBy) normalizedOptions.sort = v2.orderBy as any; if (v2.limit != null) normalizedOptions.top = v2.limit; if (v2.offset != null) normalizedOptions.skip = v2.offset; + expandParam = canonicalExpandParam(v2.expand); if (v2.aggregations) normalizedOptions.aggregations = v2.aggregations; if (v2.groupBy) normalizedOptions.groupBy = v2.groupBy; } else { @@ -4806,6 +4936,9 @@ export class ScopedProjectClient { if (normalizedOptions.groupBy) { queryParams.set('groupBy', normalizedOptions.groupBy.join(',')); } + if (expandParam) { + queryParams.set('expand', expandParam); + } const qs = queryParams.toString(); const res = await this.parent._fetch(this.url(`/data/${object}${qs ? `?${qs}` : ''}`));