From 18c787d5e93b22218e6b945fc9ea152f408423c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 13:59:30 +0000 Subject: [PATCH 1/5] wip: canonical QueryAST rewrite + pin (in progress) --- .../rest-server-canonical-query-ast.test.ts | 415 ++++++++++++++++++ packages/rest/src/rest-server.ts | 142 +++--- 2 files changed, 508 insertions(+), 49 deletions(-) create mode 100644 packages/rest/src/rest-server-canonical-query-ast.test.ts diff --git a/packages/rest/src/rest-server-canonical-query-ast.test.ts b/packages/rest/src/rest-server-canonical-query-ast.test.ts new file mode 100644 index 0000000000..c574c71cf8 --- /dev/null +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -0,0 +1,415 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16337] Every server-built `findData` literal in `rest-server.ts` speaks the + * CANONICAL QueryAST — the pin for the consumer half of #16066. + * + * ## What this file is pinning, and why a type-check alone cannot + * + * #15866 typed 22 protocol-dispatch sites in `rest-server.ts` against their + * declared spec contracts. Three `findData` literals could not join, because + * they built their `query` in the UNDECLARED wire dialect (`$filter`, `$top`, + * `$skip`, `$orderby`, `$expand`, `filters`, `select`, `sort`), so they were + * routed through a `wireDialectQuery` helper that cast the slot. #16337 + * rewrote all of them as QueryAST and retired the helper. + * + * ⛔ A type-level check cannot hold that ground on its own, and that is the + * whole reason this file reads SOURCE. The erasure it replaces was a cast, and + * a cast compiles: re-introducing `wireDialectQuery`, writing `as any`, or + * routing one more literal through an `any`-typed protocol handle each leaves + * `tsc --noEmit` at exit 0. §1 is therefore the load-bearing section, and §2 is + * the type-level half that says the canonical shapes are the ones the contract + * actually declares. + * + * ⚠️ The FOURTH literal is the reason §1 is phrased over the whole file rather + * than over three named sites. `loadImportJob` built `{ $filter, $top }` and + * handed it to a `p: any` handle — type-checked by nothing, named by no card, + * and invisible to any pin keyed to the three known sites. A guard that closes + * the CLASS finds it; a guard that enumerates instances does not. + * + * ## §3 is the equivalence receipt + * + * The rewrite is a spelling change and that is a claim, so it is measured: + * every before/after pair is driven through the REAL + * `ObjectStackProtocolImplementation` normalizer and the option bags it hands + * `engine.find` are asserted EQUAL. `RPC_QUERY_ALIAS_SLOTS` is what makes them + * equal (an alias folds onto its canonical key with the value moved verbatim); + * if that table ever stops making them equal, this section is where it is + * reported rather than in production. + * + * ⛔ What §3 is NOT: a claim that either dialect is SERVED. The public picker's + * pair is asserted equal by both REFUSING — its `where` carries + * `ViewFilterRule` rows, which the ingress declines with `400 INVALID_FILTER` + * before and after this card alike. Equality is the assertion; the verdict on + * either side is the ingress's, and repairing it is a separate card. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import type { FindDataRequest } from '@objectstack/spec/api'; +import { RestServer } from './rest-server.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const SOURCE = readFileSync(resolve(HERE, 'rest-server.ts'), 'utf8'); + +// --------------------------------------------------------------------------- +// §1 The source census — the section that reds on a re-introduced erasure +// --------------------------------------------------------------------------- + +/** + * The undeclared spellings `@objectstack/metadata-protocol`'s normalizer folds + * onto a canonical QueryAST key. `RPC_QUERY_ALIAS_SLOTS` (the spec's own table) + * declares the bare ones; `WIRE_QUERY_ALIAS_SLOTS` / `WIRE_DOLLAR_ALIASES` add + * the wire-only ones. ⛔ Not a copy of either table for the door to consult — + * it is the list of spellings a SERVER-BUILT literal must not use, and the two + * tables stay the authority on what a CALLER may send. + */ +const WIRE_DIALECT_KEYS = [ + '$filter', '$top', '$skip', '$orderby', '$select', '$expand', '$search', '$searchFields', '$count', + 'filters', 'filter', 'select', 'sort', 'skip', 'top', 'populate', +] as const; + +/** Every `query:` slot in the file, as `{ line, text }` (1-based lines). */ +function querySlots(): { line: number; text: string }[] { + return SOURCE.split('\n') + .map((text, i) => ({ line: i + 1, text })) + .filter(({ text }) => /^\s*query:\s/.test(text)); +} + +/** + * The ONE `query:` slot in this file that is allowed to carry the wire dialect, + * and the reason: `GET /data/:object` forwards the CALLER's own querystring + * bag. That is caller input, not a server-built literal — the wire dialect is + * exactly what the door exists to accept, and declaring those aliases is + * #16066's spec half. + */ +const CALLER_SUPPLIED_SLOT = 'query: req.query,'; + +/** + * The body of a server-built `query:` object literal, from its opening brace to + * the line that closes it at the same indentation. Read from source so a slot + * added later is covered without editing this file. + */ +function slotBody(line: number): string { + const lines = SOURCE.split('\n'); + const open = lines[line - 1]; + const indent = (open.match(/^\s*/) ?? [''])[0]; + if (/^\s*query:\s*\{.*\},?\s*$/.test(open)) return open; + const out = [open]; + for (let i = line; i < lines.length; i++) { + out.push(lines[i]); + if (lines[i] === `${indent}},` || lines[i] === `${indent}}`) return out.join('\n'); + } + throw new Error(`unterminated query literal at line ${line}`); +} + +describe('[#16337] §1 no server-built `query` slot in rest-server.ts is erased or wire-spelled', () => { + it('the `wireDialectQuery` helper is gone — no declaration, no call', () => { + // The retirement note in the file's prose may NAME the helper; what may + // not survive is a declaration or a call. Both spellings are checked so + // "it is mentioned in a comment" cannot be mistaken for either. + expect(SOURCE).not.toMatch(/(?:const|function)\s+wireDialectQuery\b/); + expect(SOURCE).not.toMatch(/wireDialectQuery\s*\(/); + }); + + it('every `query:` slot is an inline object literal or the caller-supplied bag — never a call or a cast', () => { + const slots = querySlots(); + // A control on the census itself: a file whose slots stopped matching + // would make every assertion below vacuously true. + expect(slots.length).toBeGreaterThanOrEqual(5); + + const offenders = slots.filter(({ text }) => { + const value = text.trim(); + if (value === CALLER_SUPPLIED_SLOT) return false; + return !/^query:\s*\{/.test(value); + }); + expect( + offenders.map((o) => `line ${o.line}: ${o.text.trim()}`), + 'a `query` slot that is not an inline object literal is the erasure #16337 retired', + ).toEqual([]); + }); + + it('no `query` slot carries an `as` cast', () => { + const cast = querySlots().filter(({ line }) => /\bas\s+(any|unknown|FindDataRequest)\b/.test(slotBody(line))); + expect(cast.map((c) => `line ${c.line}`)).toEqual([]); + }); + + it('no server-built `query` literal spells a wire alias', () => { + const found: string[] = []; + for (const { line, text } of querySlots()) { + if (text.trim() === CALLER_SUPPLIED_SLOT) continue; + const body = slotBody(line); + for (const key of WIRE_DIALECT_KEYS) { + // Key POSITION only: `where: filter` names a local called + // `filter` and is not a `filter:` key. The escape covers `$`. + const asKey = new RegExp(`(^|[\\s{,])${key.replace('$', '\\$')}\\s*:`, 'm'); + if (asKey.test(body)) found.push(`line ${line}: ${key}`); + } + } + expect(found, 'a server-built literal must speak the declared QueryAST, not the wire dialect').toEqual([]); + }); + + it('the three sites the card names are canonical, by name', () => { + // Belt to §1's braces: the class-wide assertions above would still pass + // over a file that had lost these literals entirely. + expect(SOURCE).toContain("orderBy: [{ field: 'created_at', order: 'desc' }],"); + expect(SOURCE).toContain("orderBy: [{ field: displayFields[0], order: 'asc' }],"); + expect(SOURCE).toContain("fields: ['id', ...displayFields],"); + expect(SOURCE).toMatch(/expand: Object\.fromEntries\(/); + }); +}); + +// --------------------------------------------------------------------------- +// §2 The type-level half — what the declared contract actually admits +// --------------------------------------------------------------------------- + +type Query = NonNullable; + +describe('[#16337] §2 the declared `FindDataRequest[\'query\']` contract', () => { + it('admits the canonical AST and refuses every wire spelling (compile-time)', () => { + const canonical: Query = { + object: 'sys_import_job', + where: { status: 'queued' }, + orderBy: [{ field: 'created_at', order: 'desc' }], + limit: 50, + offset: 0, + fields: ['id', 'status'], + expand: { owner_id: { object: 'owner_id' } }, + }; + expect(canonical.limit).toBe(50); + + // Each directive below is LIVE: `tsconfig.test.json` compiles this + // layer, and an unused `@ts-expect-error` is TS2578 there. So these are + // assertions, not decoration — if the wire dialect were ever declared + // on `QuerySchema`, this block reds rather than going quiet. + // @ts-expect-error `$top` is not a declared QueryAST key + const dollarTop: Query = { object: 'x', $top: 5 }; + // @ts-expect-error `filters` is not a declared QueryAST key + const wireFilters: Query = { object: 'x', filters: [] }; + // @ts-expect-error `select` is the alias; the declared key is `fields` + const wireSelect: Query = { object: 'x', select: ['id'] }; + // @ts-expect-error `sort` is the alias; the declared key is `orderBy` + const wireSort: Query = { object: 'x', sort: [{ field: 'a', order: 'asc' }] }; + // @ts-expect-error the `{field: direction}` record is not `SortNode[]` + const recordSort: Query = { object: 'x', orderBy: { created_at: 'desc' } }; + // @ts-expect-error a comma list is not `Record` + const commaExpand: Query = { object: 'x', expand: 'owner_id' }; + // @ts-expect-error `object` is REQUIRED on the declared query + const noObject: Query = { limit: 1 }; + expect([dollarTop, wireFilters, wireSelect, wireSort, recordSort, commaExpand, noObject]).toHaveLength(7); + }); +}); + +// --------------------------------------------------------------------------- +// §3 The equivalence receipt — driven through the REAL normalizer +// --------------------------------------------------------------------------- + +const OBJECT_FIXTURE = { + name: 'sys_import_job', + nameField: 'id', + searchableFields: ['object_name', 'status'], + fields: { + id: { name: 'id', type: 'text' }, + status: { name: 'status', type: 'text' }, + object_name: { name: 'object_name', type: 'text' }, + created_at: { name: 'created_at', type: 'datetime' }, + owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' }, + }, +}; + +/** + * The option bag the REAL normalizer hands `engine.find` for one query — or the + * refusal it raises instead. Both outcomes are returned rather than thrown so a + * pair that refuses on BOTH sides is still comparable: equality is what §3 + * asserts, never that either side is served. + */ +async function normalized(query: unknown): Promise { + let seen: unknown = undefined; + const engine = { + registry: { getObject: (n: string) => (n === OBJECT_FIXTURE.name ? OBJECT_FIXTURE : undefined) }, + find: async (_o: string, options: unknown) => { seen = options; return []; }, + aggregate: async () => [], + count: async () => 0, + }; + const protocol = new ObjectStackProtocolImplementation(engine as never); + try { + await protocol.findData({ object: OBJECT_FIXTURE.name, query } as never); + return { served: seen }; + } catch (error) { + const e = error as { code?: string; status?: number }; + return { refused: { code: e.code, status: e.status } }; + } +} + +/** + * The wire literals as #15866 left them, frozen. ⛔ Not a spec of what the door + * may send — a historical record, kept only so §3 has a BEFORE to compare the + * canonical rewrite against. + */ +const PAIRS: { site: string; wire: Record; canonical: Record }[] = [ + { + site: 'import-job loader (loadImportJob)', + wire: { $filter: { id: 'job_1' }, $top: 1 }, + canonical: { object: 'sys_import_job', where: { id: 'job_1' }, limit: 1 }, + }, + { + site: 'import-job listing (GET /data/import/jobs)', + wire: { $filter: { status: 'queued' }, $orderby: { created_at: 'desc' }, $top: 50, $skip: 10 }, + canonical: { + object: 'sys_import_job', + where: { status: 'queued' }, + orderBy: [{ field: 'created_at', order: 'desc' }], + limit: 50, + offset: 10, + }, + }, + { + site: 'export chunk loop (GET /data/:object/export)', + wire: { + $filter: { status: 'done' }, + $search: 'acme', + $searchFields: ['object_name'], + $orderby: { created_at: 'desc' }, + $expand: 'owner_id', + $top: 500, + $skip: 0, + }, + canonical: { + object: 'sys_import_job', + where: { status: 'done' }, + search: 'acme', + searchFields: ['object_name'], + orderBy: [{ field: 'created_at', order: 'desc' }], + expand: { owner_id: { object: 'owner_id' } }, + limit: 500, + offset: 0, + }, + }, + { + site: 'public reference picker (GET /forms/:slug/lookup/:field)', + wire: { + limit: 10, + offset: 0, + filters: [{ field: 'status', operator: 'equals', value: 'done' }], + select: ['id', 'object_name'], + sort: [{ field: 'object_name', order: 'asc' }], + }, + canonical: { + object: 'sys_import_job', + limit: 10, + offset: 0, + where: [{ field: 'status', operator: 'equals', value: 'done' }], + fields: ['id', 'object_name'], + orderBy: [{ field: 'object_name', order: 'asc' }], + }, + }, +]; + +describe('[#16337] §3 the rewrite moves nothing — driven through the real normalizer', () => { + it.each(PAIRS)('$site: wire and canonical reach the engine identically', async ({ wire, canonical }) => { + const before = await normalized(wire); + const after = await normalized(canonical); + expect(after).toEqual(before); + }); + + it('CONTROL: the instrument can tell two option bags apart', () => { + // Without this, every row above would also pass against an instrument + // that returned a constant. + return Promise.all([ + normalized({ object: 'sys_import_job', limit: 1 }), + normalized({ object: 'sys_import_job', limit: 2 }), + ]).then(([one, two]) => expect(one).not.toEqual(two)); + }); + + it('CONTROL: the picker pair is equal by REFUSING, and the refusal is the ingress\'s', async () => { + // Stated rather than left implicit: this pair's equality is not evidence + // that the picker query is served. Both sides carry `ViewFilterRule` + // rows on the filter slot, which is not a `FilterCondition`. + const outcome = await normalized(PAIRS[3].canonical) as { refused?: { code?: string; status?: number } }; + expect(outcome.refused).toEqual({ code: 'INVALID_FILTER', status: 400 }); + }); +}); + +// --------------------------------------------------------------------------- +// §4 The literals the doors ACTUALLY build, read off the mounted routes +// --------------------------------------------------------------------------- + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function mockRes() { + const res: Record = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: unknown) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.write = vi.fn(() => true); + res.end = vi.fn(() => res); + return res; +} + +/** Mount the real routes over a protocol that records every `findData` call. */ +function mountedRoutes() { + const findData = vi.fn().mockResolvedValue({ object: 'sys_import_job', records: [] }); + const protocol = { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '' } }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([]), + getMetaItem: vi.fn().mockResolvedValue(undefined), + findData, + }; + const rest = new RestServer(mockServer() as never, protocol as never, { api: { requireAuth: false } } as never); + (rest as unknown as { resolveExecCtx: () => Promise }).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = (method: string, suffix: string) => { + const found = rest.getRoutes().find((r) => r.method === method && r.path.endsWith(suffix)); + if (!found) throw new Error(`route ${method} …${suffix} is not mounted`); + return found as unknown as { handler: (req: unknown, res: unknown) => Promise }; + }; + return { findData, route }; +} + +/** Declared QueryAST keys — anything else on a server-built literal is a wire alias. */ +const DECLARED_QUERY_KEYS = new Set([ + 'object', 'fields', 'where', 'search', 'searchFields', 'orderBy', 'limit', 'offset', + 'top', 'aggregations', 'groupBy', 'having', 'expand', +]); + +describe('[#16337] §4 the mounted doors hand `findData` canonical keys', () => { + it('GET /data/import/jobs builds a canonical query', async () => { + const { findData, route } = mountedRoutes(); + const jobs = route('GET', '/data/import/jobs'); + await jobs.handler({ params: {}, query: { status: 'queued', limit: '7', offset: '3' } }, mockRes()); + + expect(findData).toHaveBeenCalledTimes(1); + const query = findData.mock.calls[0][0].query as Record; + expect(Object.keys(query).filter((k) => !DECLARED_QUERY_KEYS.has(k))).toEqual([]); + expect(query.where).toEqual({ status: 'queued' }); + expect(query.orderBy).toEqual([{ field: 'created_at', order: 'desc' }]); + expect(query.limit).toBe(7); + expect(query.offset).toBe(3); + }); + + it('GET /data/:object/export builds a canonical query for its first chunk', async () => { + const { findData, route } = mountedRoutes(); + const exportRoute = route('GET', '/data/:object/export'); + await exportRoute.handler( + { params: { object: 'sys_import_job' }, query: { format: 'json', limit: '10', filter: '{"status":"done"}', search: 'acme' } }, + mockRes(), + ); + + expect(findData).toHaveBeenCalledTimes(1); + const query = findData.mock.calls[0][0].query as Record; + expect(Object.keys(query).filter((k) => !DECLARED_QUERY_KEYS.has(k))).toEqual([]); + expect(query.where).toEqual({ status: 'done' }); + expect(query.search).toBe('acme'); + expect(query.limit).toBe(10); + expect(query.offset).toBe(0); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 53ec32739e..c7d9b27510 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -262,39 +262,33 @@ type TransportScopedMetaRequest = R & { environmentId?: string }; * What the compiler regains here is the KEY SET — an undeclared member (TS2353) * and a missing required member (TS2739/TS2741) — not the value types of keys * read off the request bag. - */ -type ServerScopedDataRequest = R & { environmentId?: string; context?: unknown }; - -/** - * [#15866] The ONE thing restoring the data doors' compile-time check could not - * type honestly, isolated behind a name so what stays erased is countable and - * greppable instead of diffuse — and so the next person meets the reason rather - * than a bare cast. * - * `FindDataRequest.query` declares the AST (`QuerySchema`). But - * `@objectstack/metadata-protocol`'s `findData` ingress accepts TWO dialects - * through that one slot: the AST, and the WIRE dialect its normalizer folds — - * the bare transport spellings and the OData `$` forms (`$top`→`top`→`limit`, - * `$orderby`→`orderBy`, `filter`/`filters`/`$filter`→`where`, …). That second - * set is deliberately undeclared: the normalizer's own table calls them "the - * wire-only spellings no schema declares", and its sibling hint table is - * documented as never accepting input precisely so a second de-facto contract - * does not grow (Prime Directive #12). + * ⭐ [#16337] The one slot this alias could NOT cover is now covered too, and + * the helper that covered for it is gone. #15866 left three server-built + * `findData` literals — the import-job listing, the export chunk loop and the + * public reference picker — speaking the UNDECLARED wire dialect (`$filter`, + * `$top`, `$skip`, `$orderby`, `$expand`, `filters`, `select`, `sort`) and + * routed them through a `wireDialectQuery` helper that cast the `query` member + * to `FindDataRequest['query']`. All three now build the CANONICAL QueryAST + * (`object`, `where`, `orderBy`, `limit`, `offset`, `fields`, `expand`), so the + * `query` slot compiles against the declared contract like every other member + * and the helper has been retired with this card. * - * Three server-built literals in this file speak that wire dialect (the - * import-job listing, the export chunk loop, the public picker). ⛔ The two - * repairs this card forbids are exactly the two that would make them compile: - * widening `QuerySchema` to admit `$`-forms, and dropping back to a runtime - * `safeParse`. So the honest move is neither — it is to keep the erasure, make - * it one slot wide instead of one call wide, and hand the gap back: the - * declared-vs-shipped mismatch on this slot is a CONTRACT question, filed - * separately, not something this door may settle by itself. + * ⚠️ The rewrite is a spelling change ONLY, and that is measurable rather than + * asserted: `@objectstack/metadata-protocol`'s `findData` folds every alias + * spelling onto the canonical key by the spec's own table + * (`RPC_QUERY_ALIAS_SLOTS`) and moves the value verbatim, so both dialects + * reach `engine.find` as the same option bag. `rest-server-canonical-query-ast + * .test.ts` drives the before/after pairs through the real normalizer and + * asserts that equality, so a future edit that changes the option bag while + * still compiling reddens there. * - * ⚠️ What is NOT erased at those three sites, and was before: the method name, - * the arity, and every other member of the request literal. + * ⛔ Server-built means server-built: the wire aliases stay accepted at the + * HTTP door for CALLERS. Declaring them there is #16066's spec half and is not + * this file's business. */ -const wireDialectQuery = (query: Record): FindDataRequest['query'] => - query as FindDataRequest['query']; +type ServerScopedDataRequest = R & { environmentId?: string; context?: unknown }; + import { buildFieldMetaMap, referenceFieldNames, @@ -8970,12 +8964,20 @@ export class RestServer { // Shared loader: fetch one job row by id. Used by the read routes, the // cancel route, and the background worker's durable cancellation checks. const loadImportJob = async (p: any, jobId: string, environmentId?: string, context?: any): Promise => { - const r = await p.findData({ + // [#16337] The FOURTH server-built `findData` literal in this file, + // and the one the card's three did not name — because nothing could + // see it: `p` is `any`, so this call was type-checked by nothing at + // all and its `$filter` / `$top` wire spellings cost no diagnostic. + // Annotating the literal is what puts it back under the same + // compiler check as its three siblings; canonicalising it is the + // same mechanical rewrite (`$filter`→`where`, `$top`→`limit`). + const jobLoadRequest: ServerScopedDataRequest = { object: IMPORT_JOB_OBJECT, - query: { $filter: { id: jobId }, $top: 1 }, + query: { object: IMPORT_JOB_OBJECT, where: { id: jobId }, limit: 1 }, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), - }); + }; + const r = await p.findData(jobLoadRequest); const rows = Array.isArray(r?.records) ? r.records : Array.isArray(r?.data) ? r.data : Array.isArray(r?.rows) ? r.rows @@ -9340,7 +9342,19 @@ export class RestServer { const offset = Math.max(0, Number(q.offset) || 0); const jobsListRequest: ServerScopedDataRequest = { object: IMPORT_JOB_OBJECT, - query: wireDialectQuery({ $filter: filter, $orderby: { created_at: 'desc' }, $top: limit, $skip: offset }), + // [#16337] Canonical QueryAST, not the wire dialect this + // literal used to speak (`$filter` / `$orderby` / `$top` / + // `$skip`). The normalizer folds those onto exactly these + // keys and the record sort form onto exactly this node + // list, so the option bag reaching `engine.find` is + // unchanged — pinned in `rest-server-canonical-query-ast.test.ts`. + query: { + object: IMPORT_JOB_OBJECT, + where: filter, + orderBy: [{ field: 'created_at', order: 'desc' }], + limit, + offset, + }, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }; @@ -9644,15 +9658,30 @@ export class RestServer { const take = Math.min(chunkSize, limit - exported); const findArgs: ServerScopedDataRequest = { object: objectName, - query: wireDialectQuery({ - ...(filter ? { $filter: filter } : {}), - ...(search ? { $search: search } : {}), - ...(search && searchFields ? { $searchFields: searchFields } : {}), - ...(orderby ? { $orderby: orderby } : {}), - ...(expandFields.length > 0 ? { $expand: expandFields.join(',') } : {}), - $top: take, - $skip: skip, - }), + // [#16337] Canonical QueryAST. `expand` is spelled as + // the relation map the AST declares rather than as the + // comma list `$expand` accepted: the normalizer lowers + // that list to `{name: {object: name}}`, which is what + // this builds directly — same map, one fewer dialect. + // (The nested `object` naming the RELATION rather than + // its target is the normalizer's own lowering, kept + // byte-identical here on purpose.) + query: { + object: objectName, + ...(filter ? { where: filter } : {}), + ...(search ? { search } : {}), + ...(search && searchFields ? { searchFields } : {}), + ...(orderby ? { orderBy: orderby } : {}), + ...(expandFields.length > 0 + ? { + expand: Object.fromEntries( + expandFields.map((rel): [string, { object: string }] => [rel, { object: rel }]), + ), + } + : {}), + limit: take, + offset: skip, + }, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }; @@ -10512,13 +10541,28 @@ export class RestServer { const pickerRequest: ServerScopedDataRequest = { object: referenceTo, - // [#15866] `filters` is a WIRE-only spelling the normalizer folds to - // `where`, and no schema declares it — see {@link wireDialectQuery}. - query: wireDialectQuery({ + // [#16337] Canonical QueryAST: `filters` → `where`, + // `select` → `fields`, `sort` → `orderBy`. The normalizer + // folds each of those aliases onto exactly these keys and + // moves the value verbatim, so this is a spelling change + // and nothing else. + // + // ⚠️ The VALUE on `where` is unchanged and is NOT a + // `FilterCondition`: `filters` carries `ViewFilterRule` + // rows (`{field, operator, value}` objects, the dialect + // `FormFieldPublicPickerSchema.filter` declares) composed + // with the route's own search row, and the ingress refuses + // a non-empty one with `400 INVALID_FILTER` — measured, and + // filed separately. ⛔ Not repaired here: this card retypes + // the SPELLING of these literals and moves no behaviour. + // `FilterCondition`'s `[key: string]: any` index signature + // is why the array still compiles against the slot. + query: { + object: referenceTo, limit: maxResults, offset: 0, - filters, - select: ['id', ...displayFields], + where: filters, + fields: ['id', ...displayFields], // [#7485] Ordering is FIXED — first display field, // ascending. This used to read `picker.sort`, a key // `FormFieldPublicPickerSchema` (#7467) deliberately @@ -10528,8 +10572,8 @@ export class RestServer { // permanently-maintained public key on an // UNAUTHENTICATED surface. A pre-schema stored row // still carrying `sort` is IGNORED, not an error. - sort: [{ field: displayFields[0], order: 'asc' }], - }), + orderBy: [{ field: displayFields[0], order: 'asc' }], + }, ...(environmentId ? { environmentId } : {}), context, }; From 2e6f6c29822ab33eb79c2c554b16a48c93ae2b84 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 14:05:34 +0000 Subject: [PATCH 2/5] wip: fixture triage for canonical spelling --- .../src/public-form-lookup-picker.test.ts | 23 ++++++++++++++----- .../rest-server-canonical-query-ast.test.ts | 6 +++-- .../rest-server-closed-query-params.test.ts | 12 ++++++---- packages/rest/src/rest.test.ts | 14 +++++++---- 4 files changed, 37 insertions(+), 18 deletions(-) diff --git a/packages/rest/src/public-form-lookup-picker.test.ts b/packages/rest/src/public-form-lookup-picker.test.ts index 40c0878532..be0f6f344d 100644 --- a/packages/rest/src/public-form-lookup-picker.test.ts +++ b/packages/rest/src/public-form-lookup-picker.test.ts @@ -203,16 +203,27 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo // the declared object override, the declared cap, the declared filter // rows ahead of the visitor's search predicate, id + displayFields // projection, offset pinned to 0 (no anonymous pagination). + // + // [#16337] The KEYS are the canonical QueryAST ones (`where` / `fields` + // / `orderBy`); until then the route spelled them `filters` / `select` / + // `sort`, wire aliases the normalizer folds onto exactly these. The + // VALUES are byte-identical across that rewrite, which is the point — + // and note what `where` carries: `ViewFilterRule` rows, the dialect + // `FormFieldPublicPickerSchema.filter` declares, NOT a + // `FilterCondition`. `findData` is stubbed in this suite, so it never + // meets the ingress's verdict on that value; the real normalizer + // refuses it. Filed separately — ⛔ do not "repair" it by editing this + // expectation. expect(findData).toHaveBeenCalledTimes(1); const call = findData.mock.calls[0][0]; expect(call.object).toBe('sys_user'); expect(call.query.limit).toBe(10); expect(call.query.offset).toBe(0); - expect(call.query.select).toEqual(['id', 'name', 'email']); + expect(call.query.fields).toEqual(['id', 'name', 'email']); // [#7485] Ordering is fixed, not authorable: first display field, // ascending. The route's `picker.sort ??` read is retired. - expect(call.query.sort).toEqual([{ field: 'name', order: 'asc' }]); - expect(call.query.filters).toEqual([ + expect(call.query.orderBy).toEqual([{ field: 'name', order: 'asc' }]); + expect(call.query.where).toEqual([ { field: 'is_active', operator: 'equals', value: true }, { field: 'name', operator: 'contains', value: 'ad' }, ]); @@ -299,7 +310,7 @@ describe('#7485 publicPicker.sort is retired — not declarable, and not read', // The stored `{ field: 'email', order: 'desc' }` reaches `findData` // nowhere: the fixed default is the only ordering the route composes. expect(findData).toHaveBeenCalledTimes(1); - expect(findData.mock.calls[0][0].query.sort).toEqual([{ field: 'name', order: 'asc' }]); + expect(findData.mock.calls[0][0].query.orderBy).toEqual([{ field: 'name', order: 'asc' }]); }); it('…and the fixed sort tracks displayFields[0], including the no-displayFields default', async () => { @@ -310,7 +321,7 @@ describe('#7485 publicPicker.sort is retired — not declarable, and not read', const stored = await persistedBody(studioForm([{ field: 'owner', publicPicker: { object: 'sys_user' } }])); const { findData, lookup } = routesOver(stored, []); await lookup.handler({ params: { slug: 'contact', field: 'owner' }, query: {} } as any, mockRes()); - expect(findData.mock.calls[0][0].query.sort).toEqual([{ field: 'name', order: 'asc' }]); + expect(findData.mock.calls[0][0].query.orderBy).toEqual([{ field: 'name', order: 'asc' }]); const stored2 = await persistedBody(studioForm([{ field: 'owner', @@ -318,7 +329,7 @@ describe('#7485 publicPicker.sort is retired — not declarable, and not read', }])); const second = routesOver(stored2, []); await second.lookup.handler({ params: { slug: 'contact', field: 'owner' }, query: {} } as any, mockRes()); - expect(second.findData.mock.calls[0][0].query.sort).toEqual([{ field: 'email', order: 'asc' }]); + expect(second.findData.mock.calls[0][0].query.orderBy).toEqual([{ field: 'email', order: 'asc' }]); }); }); diff --git a/packages/rest/src/rest-server-canonical-query-ast.test.ts b/packages/rest/src/rest-server-canonical-query-ast.test.ts index c574c71cf8..4d2828b62b 100644 --- a/packages/rest/src/rest-server-canonical-query-ast.test.ts +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -132,8 +132,10 @@ describe('[#16337] §1 no server-built `query` slot in rest-server.ts is erased ).toEqual([]); }); - it('no `query` slot carries an `as` cast', () => { - const cast = querySlots().filter(({ line }) => /\bas\s+(any|unknown|FindDataRequest)\b/.test(slotBody(line))); + it('no server-built `query` literal carries an `as` cast', () => { + const cast = querySlots() + .filter(({ text }) => text.trim() !== CALLER_SUPPLIED_SLOT) + .filter(({ line }) => /\bas\s+(any|unknown|FindDataRequest)\b/.test(slotBody(line))); expect(cast.map((c) => `line ${c.line}`)).toEqual([]); }); diff --git a/packages/rest/src/rest-server-closed-query-params.test.ts b/packages/rest/src/rest-server-closed-query-params.test.ts index 86637d939c..3079cb0fd2 100644 --- a/packages/rest/src/rest-server-closed-query-params.test.ts +++ b/packages/rest/src/rest-server-closed-query-params.test.ts @@ -303,18 +303,20 @@ describe('#7606 §2 — GET /data/:object/export', () => { // is what makes this a preservation pin: `limit` silently dropped // exports the whole table with a perfectly ordinary 200. - // `limit` is the binding cap here (25 < 100), so $top proves IT arrived. + // `limit` is the binding cap here (25 < 100), so the query's `limit` + // proves IT arrived. (Spelled `$top` until #16337 canonicalised the + // door's literal; the assertion is unchanged in what it measures.) const capped = boot(); const byLimit = await capped.exportRows({ format: 'csv', limit: '25', page: '100' }); expect(byLimit.status).toBe(200); - expect((capped.findData.mock.calls[0][0] as any)?.query?.$top).toBe(25); + expect((capped.findData.mock.calls[0][0] as any)?.query?.limit).toBe(25); - // `page` is the binding cap here (50 < 200), so $top proves IT arrived - // — a default chunk would have read 500. + // `page` is the binding cap here (50 < 200), so the query's `limit` + // proves IT arrived — a default chunk would have read 500. const chunked = boot(); const byPage = await chunked.exportRows({ format: 'csv', limit: '200', page: '50' }); expect(byPage.status).toBe(200); - expect((chunked.findData.mock.calls[0][0] as any)?.query?.$top).toBe(50); + expect((chunked.findData.mock.calls[0][0] as any)?.query?.limit).toBe(50); }); it('PRESERVATION: the row-selection axes still narrow the export', async () => { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 5e6487b087..00ea430f4a 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -1119,8 +1119,8 @@ describe('RestServer', () => { // Always return full chunks so the loop is bounded only by `limit`. protocol.findData.mockImplementation(async ({ query }: any) => { - const take = query?.$top ?? 0; - return { data: Array.from({ length: take }, (_v, i) => ({ id: String((query?.$skip ?? 0) + i) })) }; + const take = query?.limit ?? 0; + return { data: Array.from({ length: take }, (_v, i) => ({ id: String((query?.offset ?? 0) + i) })) }; }); const { res, chunks } = makeRes(); @@ -1214,7 +1214,7 @@ describe('RestServer', () => { expect(lines[0]).toBe('1,写代码,是,高,2026-06-30,张三'); }); - it('injects $expand for reference fields into the findData query', async () => { + it('injects expand for reference fields into the findData query', async () => { const p = protocolWithSchema([RAW_TASK_ROW]); const rest = new RestServer(server as any, p as any, ANON_API as any); (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); @@ -1226,7 +1226,11 @@ describe('RestServer', () => { expect(p.findData).toHaveBeenCalled(); const firstQuery = p.findData.mock.calls[0][0].query; - expect(firstQuery.$expand).toBe('owner'); + // [#16337] The door builds the canonical relation MAP the QueryAST + // declares. It used to build the comma list `$expand` accepted, which + // the normalizer lowered to exactly this map — same value, one fewer + // dialect between the door and the engine. + expect(firstQuery.expand).toEqual({ owner: { object: 'owner' } }); }); it('formats values readably in JSON, leaving unknown keys untouched', async () => { @@ -1326,7 +1330,7 @@ describe('RestServer', () => { p.getMetaItem = vi.fn().mockResolvedValue({ type: 'object', name: 'task', item: NAMED_SCHEMA }); // First page returns one row, subsequent pages are empty (ends the stream). p.findData = vi.fn(async ({ query }: any) => - (query?.$skip ?? 0) === 0 ? { data: [{ id: '1', title: 'x', done: true }] } : { data: [] }, + (query?.offset ?? 0) === 0 ? { data: [{ id: '1', title: 'x', done: true }] } : { data: [] }, ); // i18nServiceProvider is the 14th constructor arg (after server, protocol, From caee1210d19a626aee5e8fd94aafe3efc58c4940 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 14:11:17 +0000 Subject: [PATCH 3/5] chore: re-anchor system-context census after line shift --- content/docs/permissions/system-context.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index f61cbb4b79..1a0895460a 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -64,7 +64,7 @@ not on any flag. ## How the flag is set `isSystem` is **server-constructed and never client-supplied**. Inbound HTTP -cannot set it (`packages/rest/src/rest-server.ts:1858`, `:1887`), and neither +cannot set it (`packages/rest/src/rest-server.ts:1852`, `:1881`), and neither can an action body (`packages/runtime/src/domains/actions.ts:414`). It is written by internal callers only, as an option on the engine call: @@ -103,7 +103,7 @@ that silently does not happen. | 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:250` | | 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` | | 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` | -| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1890` | +| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1884` | ### 2. Write pipeline and data integrity @@ -158,7 +158,7 @@ The largest single consumer — **17 of the 106 sites**. |:--|:---|:---|:---|:---| | 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:138` | | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` | -| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:5520`, `:6977`, `:7225`, `:7656`, `:7849` | +| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:5514`, `:6971`, `:7219`, `:7650`, `:7843` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` | | 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:552`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | @@ -200,7 +200,7 @@ assuming `isSystem` covers it is a documented source of bugs. | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1590` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` | -| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1858`, `:1887`; `domains/actions.ts:414` | +| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1852`, `:1881`; `domains/actions.ts:414` | --- From afbf7a4e0b1f097d08b59cc0e8db4008e5c7235a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 14:12:18 +0000 Subject: [PATCH 4/5] chore: changeset --- .changeset/rest-server-canonical-query-ast.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .changeset/rest-server-canonical-query-ast.md diff --git a/.changeset/rest-server-canonical-query-ast.md b/.changeset/rest-server-canonical-query-ast.md new file mode 100644 index 0000000000..4ada0882ea --- /dev/null +++ b/.changeset/rest-server-canonical-query-ast.md @@ -0,0 +1,13 @@ +--- +"@objectstack/rest": patch +--- + +The REST server's own `findData` calls now build the canonical QueryAST instead of an undeclared wire dialect, and the helper that erased the type on that one slot is gone. + +Four server-built query literals in `rest-server.ts` — the import-job loader, the import-job listing, the export chunk loop and the public reference picker — spelled their query in transport aliases (`$filter`, `$top`, `$skip`, `$orderby`, `$expand`, plus the bare `filters` / `select` / `sort`). None of those spellings is declared by `QuerySchema`, so three of them were routed through a `wireDialectQuery` helper that cast the `query` member to `FindDataRequest['query']`, and the fourth escaped the compiler entirely because its protocol handle was typed `any`. All four now spell `object` / `where` / `orderBy` / `limit` / `offset` / `fields` / `expand`, so the slot compiles against the declared contract like every other member of the request, and the helper is retired. + +**No behaviour moves, and that is measured rather than asserted.** `@objectstack/metadata-protocol`'s `findData` folds every alias onto its canonical key by the spec's own table (`RPC_QUERY_ALIAS_SLOTS`) and moves the value verbatim, so both spellings reach `engine.find` as the same option bag. `rest-server-canonical-query-ast.test.ts` drives all four before/after pairs through the real normalizer and asserts that equality, and reads the source to keep the erasure retired — a cast compiles, so a type-check alone could not hold this ground. + +**Nothing is removed from the published surface.** `wireDialectQuery` was a module-local `const` in `rest-server.ts`: it carried no `export` keyword, `packages/rest/src/index.ts` never named it, and it appeared in no other file in the tree. Deleting it moves no exported symbol, which is why this is a patch. + +**What this change deliberately does NOT do:** it does not touch how the HTTP door treats a *caller's* query. The wire aliases stay accepted on `GET /data/:object` exactly as before — declaring them in the spec's alias table is a separate piece of work — and `GET /data/:object` still forwards the caller's own querystring bag untouched. From c0ae12d76bc705f4a98d8233c78f6356ef43d5fc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 14:40:55 +0000 Subject: [PATCH 5/5] docs: cite #16581 for the picker filter-shape defect --- packages/rest/src/public-form-lookup-picker.test.ts | 2 +- packages/rest/src/rest-server-canonical-query-ast.test.ts | 2 +- packages/rest/src/rest-server.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rest/src/public-form-lookup-picker.test.ts b/packages/rest/src/public-form-lookup-picker.test.ts index be0f6f344d..53e3929cad 100644 --- a/packages/rest/src/public-form-lookup-picker.test.ts +++ b/packages/rest/src/public-form-lookup-picker.test.ts @@ -212,7 +212,7 @@ describe('#7467 a spec-valid stored form carrying a publicPicker reaches the loo // `FormFieldPublicPickerSchema.filter` declares, NOT a // `FilterCondition`. `findData` is stubbed in this suite, so it never // meets the ingress's verdict on that value; the real normalizer - // refuses it. Filed separately — ⛔ do not "repair" it by editing this + // refuses it (#16581) — ⛔ do not "repair" it by editing this // expectation. expect(findData).toHaveBeenCalledTimes(1); const call = findData.mock.calls[0][0]; diff --git a/packages/rest/src/rest-server-canonical-query-ast.test.ts b/packages/rest/src/rest-server-canonical-query-ast.test.ts index 4d2828b62b..96c828b78d 100644 --- a/packages/rest/src/rest-server-canonical-query-ast.test.ts +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -41,7 +41,7 @@ * pair is asserted equal by both REFUSING — its `where` carries * `ViewFilterRule` rows, which the ingress declines with `400 INVALID_FILTER` * before and after this card alike. Equality is the assertion; the verdict on - * either side is the ingress's, and repairing it is a separate card. + * either side is the ingress's, and repairing it is #16581. */ import { describe, it, expect, vi } from 'vitest'; diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index c7d9b27510..2bc1172e13 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -10553,7 +10553,7 @@ export class RestServer { // `FormFieldPublicPickerSchema.filter` declares) composed // with the route's own search row, and the ingress refuses // a non-empty one with `400 INVALID_FILTER` — measured, and - // filed separately. ⛔ Not repaired here: this card retypes + // filed as #16581. ⛔ Not repaired here: this card retypes // the SPELLING of these literals and moves no behaviour. // `FilterCondition`'s `[key: string]: any` index signature // is why the array still compiles against the slot.