fix(client): derive data.find's canonical-key predicate from QueryOptionsV2, and map expand to the wire

.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<keyof QueryOptionsV2, keyof QueryOptions>`, 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=<comma-separated relation names>`, 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.

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<keyof QueryOptionsV2, keyof QueryOptions>;
+
+    /**
+     * 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<CanonicalOnlyKey, …>` 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<CanonicalOnlyKey, TransportRow> = {
+        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<string, TransportRow> = {
+        '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=<comma-separated relation
+        // names>` — 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();
     });
 });