diff --git a/.changeset/import-runner-canonical-query-ast.md b/.changeset/import-runner-canonical-query-ast.md new file mode 100644 index 0000000000..06b036abfd --- /dev/null +++ b/.changeset/import-runner-canonical-query-ast.md @@ -0,0 +1,13 @@ +--- +"@objectstack/rest": minor +--- + +`import-runner.ts` builds its three server-side `findData` requests in the CANONICAL QueryAST, and the helper that carried them is typed against the declared contract instead of `any`. + +`FindDataRequestSchema` declares `query: QuerySchema.optional()`, and `QuerySchema` declares `where` / `limit` / `offset` / `fields` / `orderBy` / `expand` — it declares neither `$filter` nor `$top`. The normalizer's own table calls those two "the wire-only spellings no schema declares". The reference resolver, the duplicate probe and the id recheck each built a literal in that undeclared dialect, and nothing reddened because the helper they went through took `query: any`: the literals were type-checked by nothing at all, so the undeclared keys cost no diagnostic. Reverting one of them to `$filter` now costs `TS2353 … '$filter' does not exist in type 'QueryInput'`; on the pre-change file the identical revert cost zero errors. + +- **The three literals.** `$filter` → `where`, `$top` → `limit`, plus the `object` the declared query requires. No behaviour change on the two `rest-server.ts` call paths (`POST /data/:object/import` and the async import-job worker), which hand `runImport` the real `ObjectStackProtocolImplementation`: that normalizer folds `$filter` onto `where` and `$top` onto `limit` by the spec's own `RPC_QUERY_ALIAS_SLOTS`, moving the value verbatim, so both dialects reach `engine.find` as the same option bag. +- **The erasure vehicle.** `findArgsBase` now takes a `FindDataRequest` rather than a bare `any` query, so the request-level `object` is compiled too and the `object: ''` placeholder every caller had to override is gone. This is the durable half: rewriting the literals while leaving the parameter `any` would leave the next author in this file with no diagnostic at all. +- **The pin.** `rest-server-canonical-query-ast.test.ts` censuses the PACKAGE rather than one file. `import-runner.ts` has no HTTP door — every query in it is server-built — so its census rejects a wire spelling anywhere in the file, not only inside a `query:` slot. That whole-file rule is the one that finds this class: these three literals were arguments to a helper and were never in a `query:` slot to begin with. + +⚠️ Implementor-visible: `ImportProtocolLike` is exported, its `findData(args: any)` never declared which dialect the runner sends, and the runner now sends the canonical one. An implementation that reads `args.query.$filter` / `args.query.$top` directly — rather than through the protocol normalizer — receives `undefined` and must be updated to read `where` / `limit`. diff --git a/.changeset/plugin-auth-admin-import-canonical-query.md b/.changeset/plugin-auth-admin-import-canonical-query.md new file mode 100644 index 0000000000..ad0377e53d --- /dev/null +++ b/.changeset/plugin-auth-admin-import-canonical-query.md @@ -0,0 +1,12 @@ +--- +"@objectstack/plugin-auth": patch +--- + +`runAdminImportUsers`'s hand-written `ImportProtocolLike` reads the CANONICAL QueryAST (`where` / `limit`) — the payload `@objectstack/rest`'s import runner sends as of this same release — instead of the wire-only `$filter` / `$top`. + +`POST /api/v1/auth/admin/import-users` reuses the shared import runner but swaps in an identity-specific protocol, because an identity write is `auth.api.createUser` and not an engine insert. That protocol is hand-written, so it never passes through `ObjectStackProtocolImplementation` — the normalizer that folds `$filter` onto `where` and `$top` onto `limit` for a caller arriving off the HTTP door. It has to read the canonical keys itself. + +- **A mismatch here does not produce a missing filter, it produces an unbounded one.** `const where = args?.query?.$filter ?? {}` turns an unread key into an empty filter, and an empty filter constrains nothing: the upsert duplicate probe stops discriminating, `findExisting` matches rows it was given no key for, and an admin import updates the WRONG user. Both halves are measured in `admin-import-users.test.ts` — the email-match case reported `updated: 2` where one of the two rows was new, and the phone-match case sent a probe carrying no `where` at all. +- **One dialect, and no default behind it.** The two reads are now `args.query.where` and `args.query.limit`, with no `??`. A default here would not be tolerance for an older caller — this handle is fed by the runner, never off the wire — it is precisely the lenient fallback that converts a spelling mismatch into a silent match-everything. A request that arrives without a `query` now costs a loud `TypeError` instead. + +⚠️ No published version shipped the mismatch. The runner's rewrite and this adapter land in the same release, and `@objectstack/plugin-auth` depends on `@objectstack/rest` at an exact workspace version, so the two cannot be installed apart. What this entry records is why they move together — and what the same mismatch costs any OTHER hand-written `ImportProtocolLike`, which the `@objectstack/rest` entry calls out for implementors. diff --git a/packages/plugins/plugin-auth/src/admin-import-users.ts b/packages/plugins/plugin-auth/src/admin-import-users.ts index cc79147d8a..a557f3ca47 100644 --- a/packages/plugins/plugin-auth/src/admin-import-users.ts +++ b/packages/plugins/plugin-auth/src/admin-import-users.ts @@ -349,10 +349,23 @@ export async function runAdminImportUsers( } const protocol: ImportProtocolLike = { - // findExisting path: `{ $filter, $top }` against sys_user. + // [#16638] findExisting path: the CANONICAL QueryAST against sys_user. + // `runImport` hands this handle a `FindDataRequest`, and the `QuerySchema` + // that request declares carries `where` / `limit` — it declares neither + // `$filter` nor `$top`. Those two are wire-only spellings the protocol + // normalizer folds for a caller off the HTTP door; this protocol is + // hand-written and never passes through that normalizer, so it must read + // the canonical keys itself. + // + // Two dialects would be the lenient `??` alias Prime Directive #12 forbids, + // and the default is not a harmless belt either: a `where` that falls back + // to `{}` stops constraining anything, so the duplicate probe matches rows + // it was given no key for and the upsert updates the WRONG user. One + // dialect, read straight — a request that arrives without a `query` is a + // caller defect and costs a loud TypeError, not a silent match-everything. async findData(args: any) { - const where = args?.query?.$filter ?? {}; - const limit = args?.query?.$top ?? 2; + const where = args.query.where; + const limit = args.query.limit; return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any); }, diff --git a/packages/rest/src/import-runner-bulk.test.ts b/packages/rest/src/import-runner-bulk.test.ts index 78c42ef647..b5845c8242 100644 --- a/packages/rest/src/import-runner-bulk.test.ts +++ b/packages/rest/src/import-runner-bulk.test.ts @@ -148,8 +148,9 @@ describe('runImport — bulk create batching (framework#2678)', () => { })); const updateData = vi.fn(async (args: { id: string }) => ({ id: args.id })); // Row 1 ('existing') matches an existing record → update; the rest are creates. - const findData = vi.fn(async (args: { query?: { $filter?: { name?: string } } }) => - (args.query?.$filter?.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : [])); + // [#16638] Reads the CANONICAL `where` the runner sends, not `$filter`. + const findData = vi.fn(async (args: { query: { where: { name?: string } } }) => + (args.query.where.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : [])); const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData, createManyData }; const summary = await runImport({ diff --git a/packages/rest/src/import-runner-idempotency.test.ts b/packages/rest/src/import-runner-idempotency.test.ts index 000414bd9c..bc4e7ed800 100644 --- a/packages/rest/src/import-runner-idempotency.test.ts +++ b/packages/rest/src/import-runner-idempotency.test.ts @@ -49,8 +49,21 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) { store.push(rec); return rec; }); - const findData = vi.fn(async (args: { query?: { $filter?: Record } }) => { - const filter = args.query?.$filter ?? {}; + // [#16638] Reads the CANONICAL `where` the runner sends. ⛔ The `?? {}` this + // replaces is what made this whole file pass VACUOUSLY once the runner moved + // to `where`: with `$filter` undefined every recheck degraded to `{}`, which + // constrains nothing, so the recheck matched the entire store and the + // no-duplicate assertions below held without the probe discriminating at all. + // Reading `where` straight means an absent filter throws instead. + /** + * [#16638] Every filter this double actually APPLIED. Pinning the payload + * alone would still pass over a double that read the wrong key and defaulted + * to `{}` — which is the vacuity being closed here, so both are recorded. + */ + const appliedFilters: Array> = []; + const findData = vi.fn(async (args: { query: { where: Record; limit?: number } }) => { + const filter = args.query.where; + appliedFilters.push(filter); // Supports equality and { $in: [...] } — the id recheck (framework#3173) // queries by pre-assigned id $in, like the real SQL driver does. return store.filter((row) => Object.entries(filter).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); @@ -59,12 +72,45 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) { })); }); const p: ImportProtocolLike = { findData, createData, updateData: vi.fn(), createManyData }; - return { p, store, createManyData, createData }; + return { p, store, createManyData, createData, findData, appliedFilters }; +} + +/** One recorded `findData` probe, as the double above receives it. */ +type FindProbe = { query: { where: Record; limit?: number } }; + +/** + * ⭐ [#16638] Every probe the runner sends must NARROW — the assertion this + * file was missing, and the reason it stayed GREEN through a payload rewrite + * that broke it. While the double read `args.query.$filter`, a runner sending + * `where` left that read `undefined`, the `?? {}` default turned it into an + * empty filter, and an empty filter constrains NOTHING: every recheck matched + * the entire store, so each `store` / `created` expectation below held without + * the probe discriminating between one row and any other. Passing was not + * evidence. `{}` is the shape that has to be refused, so it is asserted + * against directly. + * + * `Object.keys` on an ABSENT `where` throws rather than reporting zero keys, + * and that is deliberate: a spelling drift must be loud here, not degrade into + * a probe that matches everything. + */ +function expectEveryProbeNarrowed( + calls: ReadonlyArray, + appliedFilters: ReadonlyArray>, +): void { + expect(calls.length).toBeGreaterThan(0); + for (const [args] of calls) { + expect(Object.keys(args.query.where)).not.toHaveLength(0); + } + // The payload half is the drift alarm; this is the vacuity half. The filter + // the double APPLIED must be the one it was handed — an equality a `?? {}` + // default breaks even while the runner's payload stays perfectly canonical. + expect(appliedFilters).toEqual(calls.map(([args]) => args.query.where)); + for (const filter of appliedFilters) expect(Object.keys(filter)).not.toHaveLength(0); } describe('runImport — idempotent retry with natural keys (framework#3149)', () => { it('upsert+matchFields: a transient retry after commit does not duplicate rows', async () => { - const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' }); + const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' }); const summary = await runImport({ ...baseOpts, p, writeMode: 'upsert', matchFields: ['name'], @@ -79,6 +125,10 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', () expect(store).toHaveLength(2); // no duplicates expect(summary.created).toBe(2); expect(summary.errors).toBe(0); + // ⭐ [#16638] …and every probe that produced those numbers actually + // constrained something. The natural-key probes carry the match field. + expectEveryProbeNarrowed(findData.mock.calls, appliedFilters); + expect(findData.mock.calls.map(([a]) => Object.keys(a.query.where))).toContainEqual(['name']); }); it('upsert+matchFields: a short createManyData return degrades and still does not duplicate', async () => { @@ -97,7 +147,7 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', () }); it('pure insert (no matchFields): pre-assigned ids make the retry exactly-once too (#3173)', async () => { - const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' }); + const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' }); const summary = await runImport({ ...baseOpts, p, writeMode: 'insert', matchFields: [], @@ -110,6 +160,18 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', () expect(store).toHaveLength(2); expect(summary.created).toBe(2); expect(summary.errors).toBe(0); + + // ⭐ [#16638] The recheck is the whole mechanism of #3173, so pin the + // payload it was handed rather than only the outcome: `id: { $in: [...] }` + // over exactly the ids the runner pre-assigned, bounded to that many rows. + // Read `where` / `limit`, the keys `FindDataRequest` declares — a drift + // back to `$filter` / `$top` reddens here before it reaches an implementor. + expectEveryProbeNarrowed(findData.mock.calls, appliedFilters); + const probes = findData.mock.calls.map(([a]) => a.query); + expect(probes).toHaveLength(1); + expect(Object.keys(probes[0].where)).toEqual(['id']); + expect([...probes[0].where.id.$in].sort()).toEqual(store.map((r) => r.id).sort()); + expect(probes[0].limit).toBe(store.length); }); it('pure insert: legitimate duplicate rows survive the retry intact (each copy has its own id) (#3173)', async () => { diff --git a/packages/rest/src/import-runner-selfref.test.ts b/packages/rest/src/import-runner-selfref.test.ts index 1ac369e2ae..84142e202a 100644 --- a/packages/rest/src/import-runner-selfref.test.ts +++ b/packages/rest/src/import-runner-selfref.test.ts @@ -42,8 +42,10 @@ function makeProtocol(seed: Array> = []) { return rec; }), })); - const findData = vi.fn(async (args: { query?: { $filter?: Record } }) => { - const filter = args.query?.$filter ?? {}; + // [#16638] Reads the CANONICAL `where` the runner sends. ⛔ No `?? {}`: an + // absent filter must throw here, never degrade into a match-everything probe. + const findData = vi.fn(async (args: { query: { where: Record } }) => { + const filter = args.query.where; return store.filter((row) => Object.entries(filter).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return row[k] === v; })); }); const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData: vi.fn(), createManyData }; diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 3a4895c733..32a731bb57 100644 --- a/packages/rest/src/import-runner.ts +++ b/packages/rest/src/import-runner.ts @@ -4,7 +4,7 @@ import { randomUUID } from 'node:crypto'; import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js'; import type { ExportFieldMeta } from './export-format.js'; import type { ValidationMessageTranslator } from '@objectstack/spec/system'; -import type { ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api'; +import type { FindDataRequest, ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api'; import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types'; import { isEngineDuplicateRecordEnvelope } from './error-response.js'; @@ -357,9 +357,29 @@ export function runImport(opts: RunImportOptions): Promise { : Array.isArray(r?.data) ? r.data : Array.isArray(r?.rows) ? r.rows : Array.isArray(r) ? r : []; - const findArgsBase = (query: any) => ({ - object: '', - query, + // [#16638] The server-scoped envelope for this file's `findData` calls, and + // the reason its parameter is TYPED. It used to be `query: any`, so the three + // call sites below were type-checked by nothing at all and their undeclared + // `$filter` / `$top` wire spellings cost no diagnostic — the same erasure + // #16337 found on `loadImportJob`'s `p: any` handle in `rest-server.ts`, + // whose signpost prescribes exactly this rewrite. Compiled against + // `FindDataRequest` every member is now held to the contract + // `FindDataRequestSchema` declares (`QuerySchema`: `where` / `limit` / + // `offset` / `fields` / `orderBy` / `expand`), so a wire alias is a compile + // error at the call site instead of a payload no schema has seen. It also + // retires the `object: ''` placeholder every caller had to override. + // + // ⚠️ The rewrite is a SPELLING change only: `@objectstack/metadata-protocol` + // folds `$filter`→`where` and `$top`→`limit` by the spec's own + // `RPC_QUERY_ALIAS_SLOTS` with the value moved verbatim, so all three calls + // reach `engine.find` with the same option bag as before — + // `rest-server-canonical-query-ast.test.ts` §3 measures that pair by pair. + // + // ⛔ 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 findArgsBase = (request: FindDataRequest) => ({ + ...request, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }); @@ -385,10 +405,10 @@ export function runImport(opts: RunImportOptions): Promise { let match: RefMatch = {}; for (const f of candidates) { try { - const r = await p.findData({ - ...findArgsBase({ $filter: { [f]: display }, $top: 2 }), + const r = await p.findData(findArgsBase({ object: referenceObject, - }); + query: { object: referenceObject, where: { [f]: display }, limit: 2 }, + })); const recs = findRows(r); if (recs.length === 0) continue; if (recs.length > 1) { match = { ambiguous: true, matchedField: f }; break; } @@ -428,7 +448,10 @@ export function runImport(opts: RunImportOptions): Promise { if (v === undefined || v === null || v === '') return 'blank'; filter[f] = v; } - const r = await p.findData({ ...findArgsBase({ $filter: filter, $top: 2 }), object: objectName }); + const r = await p.findData(findArgsBase({ + object: objectName, + query: { object: objectName, where: filter, limit: 2 }, + })); const recs = findRows(r); if (recs.length === 0) return 'none'; if (recs.length > 1) return 'ambiguous'; @@ -548,10 +571,10 @@ export function runImport(opts: RunImportOptions): Promise { const recheckByIds = async (chunk: Array>): Promise> => { const ids = chunk.map((r) => r.id).filter((v) => v != null && v !== ''); if (ids.length === 0) return new Map(); - const r = await p.findData({ - ...findArgsBase({ $filter: { id: { $in: ids } }, $top: ids.length }), + const r = await p.findData(findArgsBase({ object: objectName, - }); + query: { object: objectName, where: { id: { $in: ids } }, limit: ids.length }, + })); return new Map(findRows(r).map((rec: any) => [String(rec.id), rec])); }; const flushPendingCreates = async (): Promise => { 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 2f980ac619..846851ca33 100644 --- a/packages/rest/src/rest-server-canonical-query-ast.test.ts +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -4,6 +4,23 @@ * [#16337] Every server-built `findData` literal in `rest-server.ts` speaks the * CANONICAL QueryAST — the pin for the consumer half of #16066. * + * ⭐ [#16638] The census is now keyed to the PACKAGE's server-built query + * surface rather than to one file. `import-runner.ts` carried three more of + * these literals and they hid the same way the fourth one did: they were the + * argument to a `findArgsBase(query: any)` helper, so nothing type-checked them + * and `$filter` / `$top` cost no diagnostic. A pin keyed to ONE file cannot + * close a class that lives in a package, so §1 now runs per file, from a table + * that a new file is added to instead of a new test. + * + * ⚠️ The two files get DIFFERENT census rules, and the difference is not + * cosmetic. `rest-server.ts` is the HTTP door: it parses `filter` / `top` / + * `skip` / `sort` / `select` off the caller's own querystring, so a wire + * spelling outside a server-built `query:` literal is legitimate there. + * `import-runner.ts` has NO door — every query in it is server-built — so the + * wire dialect has nowhere legitimate to stand anywhere in the file, and its + * census says exactly that. That whole-file rule is the one that would have + * caught these three: they were never in a `query:` slot to begin with. + * * ## What this file is pinning, and why a type-check alone cannot * * #15866 typed 22 protocol-dispatch sites in `rest-server.ts` against their @@ -61,7 +78,33 @@ 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'); +const sourceOf = (file: string) => readFileSync(resolve(HERE, file), 'utf8'); + +const REST_SERVER = sourceOf('rest-server.ts'); +const IMPORT_RUNNER = sourceOf('import-runner.ts'); + +interface CensusEntry { + /** File name, relative to this test — the census reads package sources only. */ + file: string; + source: string; + /** + * Floor on the number of `query:` slots the file must still have. A file + * whose slots stopped matching would make every §1 assertion vacuously + * true, so each entry states what it expects to find. + */ + minQuerySlots: number; + /** + * Does a wire spelling have anywhere legitimate to stand in this file? + * `true` = no door, so the census covers the WHOLE file rather than only + * its `query:` literals (see the header note). + */ + noDoor: boolean; +} + +const CENSUS: CensusEntry[] = [ + { file: 'rest-server.ts', source: REST_SERVER, minQuerySlots: 5, noDoor: false }, + { file: 'import-runner.ts', source: IMPORT_RUNNER, minQuerySlots: 3, noDoor: true }, +]; // --------------------------------------------------------------------------- // §1 The source census — the section that reds on a re-introduced erasure @@ -80,9 +123,9 @@ const WIRE_DIALECT_KEYS = [ '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') +/** Every `query:` slot in one file, as `{ line, text }` (1-based lines). */ +function querySlots(source: string): { line: number; text: string }[] { + return source.split('\n') .map((text, i) => ({ line: i + 1, text })) .filter(({ text }) => /^\s*query:\s/.test(text)); } @@ -101,8 +144,8 @@ const CALLER_SUPPLIED_SLOT = 'query: req.query,'; * 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'); +function slotBody(source: string, 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; @@ -114,61 +157,154 @@ function slotBody(line: number): string { 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*\(/); - }); +/** + * [#16638] Comment-ONLY lines, dropped. Deliberately conservative: a trailing + * comment after code survives, so the whole-file scan below can only ever + * over-report (a loud failure someone fixes), never under-report (a silent + * pass). A string-aware stripper would be the alternative and it is the + * unsafe one here — `replace(/[`"']/g, '')` in `import-runner.ts` opens a + * quote state that no simple tokenizer closes, and everything after it would + * stop being scanned at all. + */ +function withoutCommentLines(source: string): string { + return source + .split('\n') + .filter((line) => !/^\s*(?:\/\/|\/\*|\*)/.test(line)) + .join('\n'); +} + +/** + * Wire-dialect keys in OBJECT-LITERAL KEY position anywhere in a source — the + * class-closing half. The preceding `{` or `,` is what separates a key from a + * type annotation: `const filter: Record` is preceded by `const` + * and is not a key, while `{ $filter: …` and a key on its own line after a + * trailing comma both are. + */ +function wireKeysAnywhere(source: string): string[] { + const text = withoutCommentLines(source); + const found: string[] = []; + for (const key of WIRE_DIALECT_KEYS) { + const asKey = new RegExp(`([{,])\\s*${key.replace('$', '\\$')}\\s*:`); + if (asKey.test(text)) found.push(key); + } + return found; +} + +for (const { file, source, minQuerySlots, noDoor } of CENSUS) { + describe(`[#16337][#16638] §1 no server-built \`query\` slot in ${file} 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(source); + // A control on the census itself: a file whose slots stopped matching + // would make every assertion below vacuously true. + expect(slots.length).toBeGreaterThanOrEqual(minQuerySlots); + + 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 server-built `query` literal carries an `as` cast', () => { + const cast = querySlots(source) + .filter(({ text }) => text.trim() !== CALLER_SUPPLIED_SLOT) + .filter(({ line }) => /\bas\s+(any|unknown|FindDataRequest)\b/.test(slotBody(source, line))); + expect(cast.map((c) => `line ${c.line}`)).toEqual([]); + }); - 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); + it('no server-built `query` literal spells a wire alias', () => { + const found: string[] = []; + for (const { line, text } of querySlots(source)) { + if (text.trim() === CALLER_SUPPLIED_SLOT) continue; + const body = slotBody(source, 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([]); + }); - const offenders = slots.filter(({ text }) => { - const value = text.trim(); - if (value === CALLER_SUPPLIED_SLOT) return false; - return !/^query:\s*\{/.test(value); + it.skipIf(!noDoor)('has no door, so NO wire spelling stands anywhere in the file', () => { + // ⭐ [#16638] The rule the `query:` census structurally could not + // reach. This file's three literals were arguments to a helper, not + // `query:` slots — a guard that enumerates slots does not find + // them; a guard that closes the class does. + expect( + wireKeysAnywhere(source), + 'every query in this file is server-built, so a wire alias has nowhere legitimate to stand', + ).toEqual([]); }); - 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 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([]); +describe('[#16638] §1 CONTROLS on the census instrument itself', () => { + it('the whole-file detector fires on a wire spelling, in every position it must', () => { + // Without this the empty result above is equally consistent with a + // detector that matches nothing. + expect(wireKeysAnywhere('const a = { $filter: { id: 1 } };')).toEqual(['$filter']); + expect(wireKeysAnywhere('const a = { where: 1,\n $top: 2 };')).toEqual(['$top']); + expect(wireKeysAnywhere('const a = { select: [] };')).toEqual(['select']); }); - 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 whole-file detector does NOT fire on a type annotation or a value reference', () => { + // The two shapes `import-runner.ts` actually contains. + expect(wireKeysAnywhere('const filter: Record = {};')).toEqual([]); + expect(wireKeysAnywhere('const a = { where: filter, limit: 2 };')).toEqual([]); }); - it('the three sites the card names are canonical, by name', () => { + it('dropping comment lines leaves the code that is being censused', () => { + // The stripper is the one step that could silently empty the input. + const stripped = withoutCommentLines(IMPORT_RUNNER); + expect(stripped).toContain('const findArgsBase = (request: FindDataRequest) => ({'); + expect(stripped.split('\n').length).toBeGreaterThan(400); + // And it really does drop prose: the header of the helper names the + // retired spellings, and that prose must not be censused. + expect(IMPORT_RUNNER).toContain('`$filter` / `$top` wire spellings cost no diagnostic'); + expect(stripped).not.toContain('wire spellings cost no diagnostic'); + }); +}); + +describe('[#16337] §1 the three sites rest-server.ts names are canonical, by name', () => { + it('names them', () => { // 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\(/); + expect(REST_SERVER).toContain("orderBy: [{ field: 'created_at', order: 'desc' }],"); + expect(REST_SERVER).toContain("orderBy: [{ field: displayFields[0], order: 'asc' }],"); + expect(REST_SERVER).toContain("fields: ['id', ...displayFields],"); + expect(REST_SERVER).toMatch(/expand: Object\.fromEntries\(/); + }); +}); + +describe('[#16638] §1 the three sites import-runner.ts names are canonical, and the helper still types them', () => { + it('the three literals are there, canonical, and carry the required `object`', () => { + expect(IMPORT_RUNNER).toContain('query: { object: referenceObject, where: { [f]: display }, limit: 2 },'); + expect(IMPORT_RUNNER).toContain('query: { object: objectName, where: filter, limit: 2 },'); + expect(IMPORT_RUNNER).toContain('query: { object: objectName, where: { id: { $in: ids } }, limit: ids.length },'); + }); + + it('the envelope helper is compiled against the declared contract, not `any`', () => { + // ⭐ The erasure vehicle this card retired. The whole-file rule above is + // what actually holds the ground — reverting this signature alone + // changes no spelling — but naming it here says which line is load + // bearing, and the second assertion closes the vector class-wide. + expect(IMPORT_RUNNER).toMatch(/const findArgsBase = \(request: FindDataRequest\) => \(\{/); + const erased = withoutCommentLines(IMPORT_RUNNER).match(/\(\s*(?:query|request)\s*:\s*any\b/g) ?? []; + expect(erased, 'a query-carrying parameter typed as any puts every literal handed to it back outside the compiler').toEqual([]); }); }); @@ -197,6 +333,8 @@ describe('[#16337] §2 the declared `FindDataRequest[\'query\']` contract', () = // 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 [#16638] `$filter` is not a declared QueryAST key + const dollarFilter: Query = { object: 'x', $filter: { id: '1' } }; // @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` @@ -209,7 +347,7 @@ describe('[#16337] §2 the declared `FindDataRequest[\'query\']` contract', () = 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); + expect([dollarTop, dollarFilter, wireFilters, wireSelect, wireSort, recordSort, commaExpand, noObject]).toHaveLength(8); }); }); @@ -298,6 +436,26 @@ const PAIRS: { site: string; wire: Record; canonical: Record p.site.startsWith('public reference picker'))!; + const outcome = await normalized(picker.canonical) as { refused?: { code?: string; status?: number } }; expect(outcome.refused).toEqual({ code: 'INVALID_FILTER', status: 400 }); }); });