diff --git a/.changeset/import-protocol-typed-args.md b/.changeset/import-protocol-typed-args.md new file mode 100644 index 0000000000..ba2c88d924 --- /dev/null +++ b/.changeset/import-protocol-typed-args.md @@ -0,0 +1,53 @@ +--- +"@objectstack/rest": minor +--- + +refactor(rest)!: `ImportProtocolLike` declares the request each of its three required members receives, instead of `args: any` (#16952) + +The exported extension point `runImport` accepts a protocol through now states its own contract. + +**FROM** — every required member erased its parameter, so the interface declared nothing about the request it would hand an implementor: + +```ts +export interface ImportProtocolLike { + findData(args: any): Promise; + createData(args: any): Promise; + updateData(args: any): Promise; +} +``` + +**TO** — each member names the declared spec request, wrapped in the server-scoped envelope the runner adds (`ImportProtocolRequest`, exported alongside): + +```ts +export type ImportProtocolRequest = R & { context?: any; environmentId?: string }; + +export interface ImportProtocolLike { + findData(args: ImportProtocolRequest): Promise; + createData(args: ImportProtocolRequest): Promise; + updateData(args: ImportProtocolRequest): Promise; +} +``` + +**Why this is breaking-ish, and released as `minor`.** This is a narrowing of a published surface: an implementor that compiles today may stop compiling. Nothing about the values the runner sends changes — the request objects are byte-for-byte the ones #16638 already made canonical — so no runtime behaviour moves. What changes is that the compiler now holds an implementor to the same `QuerySchema` the runner is held to: `where` / `limit` / `offset` / `fields` / `orderBy` / `expand` are declared, and the wire spellings `$filter` / `$top` are not. + +**Migration for implementors.** If your `findData` / `createData` / `updateData` reads a wire alias, it will now fail to compile — that diagnostic is the point of this change, and the fix is to read the canonical key: + +```ts +// before — compiles, and silently degrades to match-everything when `$filter` is absent +async findData(args: any) { + const where = args?.query?.$filter ?? {}; + const limit = args?.query?.$top ?? 2; +} + +// after — drop your own annotation and let the declaration type the parameter +async findData(args) { + const where = args.query!.where; + const limit = args.query!.limit; +} +``` + +⛔ An implementor that keeps an explicit `args: any` annotation of its own opts back out: the annotation wins over the contextual type, and the contract reaches nothing. Leave the parameter unannotated, or name `ImportProtocolRequest` explicitly. + +⚠️ The `?? {}` shape in the "before" is the mechanism that made a dialect mismatch silent rather than loud: an unrecognised query does not throw, it degrades into a filter that constrains nothing, so a duplicate probe stops discriminating and an upsert updates the wrong record. Prefer a read that throws. + + diff --git a/packages/rest/src/import-runner-bulk.test.ts b/packages/rest/src/import-runner-bulk.test.ts index b5845c8242..333e8fce18 100644 --- a/packages/rest/src/import-runner-bulk.test.ts +++ b/packages/rest/src/import-runner-bulk.test.ts @@ -9,6 +9,18 @@ import { describe, it, expect, vi } from 'vitest'; import { runImport, type ImportProtocolLike } from './import-runner'; + +/** + * [#16952] The doubles below are annotated FROM the exported declaration + * (`ImportProtocolLike`), never from a hand-written restatement of the shape + * the runner happens to send. A local parameter annotation was one of the + * three non-authoritative places this card converged: it froze a dialect no + * compiler held anyone to, so it kept compiling — and kept passing — after the + * runner moved to another one. ⛔ Never widen these back to an inline object + * type; that re-opens the seam. + */ +type FindArgs = Parameters[0]; +type CreateArgs = Parameters[0]; import type { ExportFieldMeta } from './export-format.js'; const metaMap = new Map([ @@ -85,9 +97,9 @@ describe('runImport — bulk create batching (framework#2678)', () => { const createManyData = vi.fn(async () => { throw new Error('CHECK constraint failed'); }); - const createData = vi.fn(async (args: { data: { name: string } }) => { + const createData = vi.fn(async (args: CreateArgs) => { if (args.data.name === 'r1') throw new Error('CHECK constraint failed: name'); - return { id: `id_${args.data.name}`, record: { id: `id_${args.data.name}` } }; + return { id: `id_${String(args.data.name)}`, record: { id: `id_${String(args.data.name)}` } }; }); const p: ImportProtocolLike = { findData: vi.fn(async () => []), @@ -107,7 +119,7 @@ describe('runImport — bulk create batching (framework#2678)', () => { }); it('falls back to one createData call per row when the protocol has no createManyData', async () => { - const createData = vi.fn(async (args: { data: { name: string } }) => ({ id: `id_${args.data.name}` })); + const createData = vi.fn(async (args: CreateArgs) => ({ id: `id_${String(args.data.name)}` })); const p: ImportProtocolLike = { findData: vi.fn(async () => []), createData, @@ -123,10 +135,10 @@ describe('runImport — bulk create batching (framework#2678)', () => { it('retries a transient createData failure on the no-createManyData fallback path (#3150)', async () => { let attempts = 0; - const createData = vi.fn(async (args: { data: { name: string } }) => { + const createData = vi.fn(async (args: CreateArgs) => { attempts++; if (attempts === 1) throw new Error('fetch failed'); // one transient blip, then succeeds - return { id: `id_${args.data.name}` }; + return { id: `id_${String(args.data.name)}` }; }); const p: ImportProtocolLike = { findData: vi.fn(async () => []), @@ -149,8 +161,8 @@ 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. // [#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 findData = vi.fn(async (args: FindArgs) => + (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-cancel.test.ts b/packages/rest/src/import-runner-cancel.test.ts index 9c591c372f..b7045e15f7 100644 --- a/packages/rest/src/import-runner-cancel.test.ts +++ b/packages/rest/src/import-runner-cancel.test.ts @@ -14,6 +14,17 @@ import { describe, it, expect, vi } from 'vitest'; import { runImport, type ImportProtocolLike } from './import-runner'; + +/** + * [#16952] The doubles below are annotated FROM the exported declaration + * (`ImportProtocolLike`), never from a hand-written restatement of the shape + * the runner happens to send. A local parameter annotation was one of the + * three non-authoritative places this card converged: it froze a dialect no + * compiler held anyone to, so it kept compiling — and kept passing — after the + * runner moved to another one. ⛔ Never widen these back to an inline object + * type; that re-opens the seam. + */ +type CreateArgs = Parameters[0]; import type { ExportFieldMeta } from './export-format.js'; const metaMap = new Map([ @@ -40,7 +51,7 @@ function rowsOf(n: number): Array> { function syncProtocol(): ImportProtocolLike { return { findData: vi.fn(async () => []), - createData: vi.fn(async (args: { data: { name: string } }) => ({ id: `id_${args.data.name}` })), + createData: vi.fn(async (args: CreateArgs) => ({ id: `id_${String(args.data.name)}` })), updateData: vi.fn(async () => ({})), createManyData: vi.fn(async (args: { records: any[] }) => ({ records: args.records.map((r) => ({ id: `id_${r.name}`, ...r })), diff --git a/packages/rest/src/import-runner-historical-readonly-insert.test.ts b/packages/rest/src/import-runner-historical-readonly-insert.test.ts index 7faa6dbbb0..43dd0fca08 100644 --- a/packages/rest/src/import-runner-historical-readonly-insert.test.ts +++ b/packages/rest/src/import-runner-historical-readonly-insert.test.ts @@ -99,10 +99,14 @@ async function makeRealProtocol() { const impl = new ObjectStackProtocolImplementation(engine as any); // `runImport` needs find/create only for an insert-mode run; delegate both to // the real implementation so the ingress is genuinely on the path. + // [#16952] The bridge is annotated FROM the exported declaration, so the + // request this test hands the REAL implementation is the one the contract + // declares — `args: any` here would have made the bridge itself another + // place the dialect was only observed. const p: ImportProtocolLike = { - findData: (args: any) => impl.findData(args as any) as any, - createData: (args: any) => impl.createData(args as any) as any, - updateData: (args: any) => impl.updateData(args as any) as any, + findData: (args) => impl.findData(args) as any, + createData: (args) => impl.createData(args) as any, + updateData: (args) => impl.updateData(args) as any, }; return { p, inserted, logger }; } diff --git a/packages/rest/src/import-runner-historical.test.ts b/packages/rest/src/import-runner-historical.test.ts index b26512eec0..4f17e92dea 100644 --- a/packages/rest/src/import-runner-historical.test.ts +++ b/packages/rest/src/import-runner-historical.test.ts @@ -15,6 +15,17 @@ import { describe, it, expect, vi } from 'vitest'; import { runImport, type ImportProtocolLike } from './import-runner'; + +/** + * [#16952] Annotated FROM the exported declaration (`ImportProtocolLike`). + * These doubles used to say `args: any`, which is the erasure this card + * retired at the declaration — an implementor that re-states `any` on its own + * parameter opts back out of the contract, because the annotation wins over + * the contextual type. + */ +type CreateArgs = Parameters[0]; +type UpdateArgs = Parameters[0]; +type CreateManyArgs = Parameters>[0]; import type { ExportFieldMeta } from './export-format.js'; const metaMap = new Map([['name', { name: 'name', type: 'text' }]]); @@ -37,15 +48,15 @@ function makeProvider() { let idc = 0; const p: ImportProtocolLike = { findData: vi.fn(async () => []), - createData: vi.fn(async (args: any) => { + createData: vi.fn(async (args: CreateArgs) => { contexts.push(args.context); return { id: `d${++idc}`, ...args.data }; }), - updateData: vi.fn(async (args: any) => { + updateData: vi.fn(async (args: UpdateArgs) => { contexts.push(args.context); return { id: args.id, ...args.data }; }), - createManyData: vi.fn(async (args: any) => { + createManyData: vi.fn(async (args: CreateManyArgs) => { contexts.push(args.context); return { records: args.records.map((r: any) => ({ id: `d${++idc}`, ...r })) }; }), diff --git a/packages/rest/src/import-runner-idempotency.test.ts b/packages/rest/src/import-runner-idempotency.test.ts index bc4e7ed800..1e4434f281 100644 --- a/packages/rest/src/import-runner-idempotency.test.ts +++ b/packages/rest/src/import-runner-idempotency.test.ts @@ -12,6 +12,18 @@ import { describe, it, expect, vi } from 'vitest'; import { runImport, type ImportProtocolLike } from './import-runner'; import type { ExportFieldMeta } from './export-format.js'; +/** + * [#16952] The doubles below are annotated FROM the exported declaration + * (`ImportProtocolLike`), never from a hand-written restatement of the shape + * the runner happens to send. A local parameter annotation was one of the + * three non-authoritative places this card converged: it froze a dialect no + * compiler held anyone to, so it kept compiling — and kept passing — after the + * runner moved to another one. ⛔ Never widen these back to an inline object + * type; that re-opens the seam. + */ +type FindArgs = Parameters[0]; +type CreateArgs = Parameters[0]; + const metaMap = new Map([['name', { name: 'name', type: 'text' }]]); const baseOpts = { @@ -44,7 +56,7 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) { if (calls === 1 && opts.firstCall === 'shortReturn') return { records: [] }; // committed, bad count return { records: recs }; }); - const createData = vi.fn(async (args: { data: { name: string } }) => { + const createData = vi.fn(async (args: CreateArgs) => { const rec = { id: `id-${++idc}`, ...args.data }; store.push(rec); return rec; @@ -61,8 +73,11 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) { * 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; + const findData = vi.fn(async (args: FindArgs) => { + // Both slots are OPTIONAL on the declared contract, and the `!`s say so + // while keeping the refusal: an absent one throws here exactly as it did + // before, rather than degrading into a match-everything probe. + 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. @@ -75,8 +90,11 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) { return { p, store, createManyData, createData, findData, appliedFilters }; } -/** One recorded `findData` probe, as the double above receives it. */ -type FindProbe = { query: { where: Record; limit?: number } }; +/** + * One recorded `findData` probe — the DECLARED parameter type, not a + * restatement of it. [#16952] + */ +type FindProbe = FindArgs; /** * ⭐ [#16638] Every probe the runner sends must NARROW — the assertion this @@ -99,12 +117,12 @@ function expectEveryProbeNarrowed( ): void { expect(calls.length).toBeGreaterThan(0); for (const [args] of calls) { - expect(Object.keys(args.query.where)).not.toHaveLength(0); + 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)); + expect(appliedFilters).toEqual(calls.map(([args]) => args.query!.where!)); for (const filter of appliedFilters) expect(Object.keys(filter)).not.toHaveLength(0); } @@ -128,7 +146,7 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', () // ⭐ [#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']); + 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 () => { @@ -167,10 +185,10 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', () // 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); + 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(Object.keys(probes[0].where!)).toEqual(['id']); + expect([...(probes[0].where!.id as { $in: string[] }).$in].sort()).toEqual(store.map((r) => r.id).sort()); expect(probes[0].limit).toBe(store.length); }); diff --git a/packages/rest/src/import-runner-selfref.test.ts b/packages/rest/src/import-runner-selfref.test.ts index 84142e202a..54bdca8ff4 100644 --- a/packages/rest/src/import-runner-selfref.test.ts +++ b/packages/rest/src/import-runner-selfref.test.ts @@ -11,6 +11,17 @@ import { describe, it, expect, vi } from 'vitest'; import { runImport, type ImportProtocolLike } from './import-runner'; + +/** + * [#16952] The doubles below are annotated FROM the exported declaration + * (`ImportProtocolLike`), never from a hand-written restatement of the shape + * the runner happens to send. A local parameter annotation was one of the + * three non-authoritative places this card converged: it froze a dialect no + * compiler held anyone to, so it kept compiling — and kept passing — after the + * runner moved to another one. ⛔ Never widen these back to an inline object + * type; that re-opens the seam. + */ +type FindArgs = Parameters[0]; import type { ExportFieldMeta } from './export-format.js'; // `parent` is a lookup back to this same object (a category tree). @@ -44,8 +55,8 @@ function makeProtocol(seed: Array> = []) { })); // [#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; + const findData = vi.fn(async (args: FindArgs) => { + 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-unique-violation-row.test.ts b/packages/rest/src/import-runner-unique-violation-row.test.ts index f41ff14466..189e7da4b9 100644 --- a/packages/rest/src/import-runner-unique-violation-row.test.ts +++ b/packages/rest/src/import-runner-unique-violation-row.test.ts @@ -35,6 +35,17 @@ import { describe, it, expect, vi } from 'vitest'; import { DuplicateRecordError } from '@objectstack/objectql'; import { uniqueViolationColumn } from '@objectstack/types'; import { runImport, type ImportProtocolLike } from './import-runner'; + +/** + * [#16952] The doubles below are annotated FROM the exported declaration + * (`ImportProtocolLike`), never from a hand-written restatement of the shape + * the runner happens to send. A local parameter annotation was one of the + * three non-authoritative places this card converged: it froze a dialect no + * compiler held anyone to, so it kept compiling — and kept passing — after the + * runner moved to another one. ⛔ Never widen these back to an inline object + * type; that re-opens the seam. + */ +type CreateArgs = Parameters[0]; import { mapDataError } from './error-response.js'; import type { ExportFieldMeta } from './export-format.js'; @@ -73,7 +84,7 @@ const envelope = () => new DuplicateRecordError('task', sqliteRaw(), uniqueViola function protocolWith(overrides: Partial): ImportProtocolLike { return { findData: vi.fn(async () => []), - createData: vi.fn(async (args: { data: { name: string } }) => ({ id: `id_${args.data.name}` })), + createData: vi.fn(async (args: CreateArgs) => ({ id: `id_${String(args.data.name)}` })), updateData: vi.fn(), ...overrides, }; @@ -91,9 +102,9 @@ function expectNothingLeaked(summary: unknown): void { describe('[#14723] §1 — the per-row `createData` path', () => { it('a `DuplicateRecordError` row reports `UNIQUE_VIOLATION`, and nothing of the driver', async () => { const p = protocolWith({ - createData: vi.fn(async (args: { data: { name: string } }) => { + createData: vi.fn(async (args: CreateArgs) => { if (args.data.name === 'r1') throw envelope(); - return { id: `id_${args.data.name}` }; + return { id: `id_${String(args.data.name)}` }; }), }); @@ -111,9 +122,9 @@ describe('[#14723] §1 — the per-row `createData` path', () => { describe('[#14723] §2 — the batched `createManyData` path, degraded to per-row writes', () => { it('the conflicting row alone reports `UNIQUE_VIOLATION`; its siblings are created', async () => { const createManyData = vi.fn(async () => { throw envelope(); }); - const createData = vi.fn(async (args: { data: { name: string } }) => { + const createData = vi.fn(async (args: CreateArgs) => { if (args.data.name === 'r1') throw envelope(); - return { id: `id_${args.data.name}` }; + return { id: `id_${String(args.data.name)}` }; }); const p = protocolWith({ createData, createManyData }); diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 32a731bb57..47f875f0dc 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 { FindDataRequest, ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api'; +import type { CreateDataRequest, FindDataRequest, UpdateDataRequest, 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'; @@ -91,18 +91,56 @@ export interface ImportRunSummary extends ImportProgress { undoLog?: ImportUndoLog; } -/** Minimal protocol surface the runner needs (find / create / update). */ +/** + * The envelope `runImport` wraps every declared request in before dispatch: the + * spec request type itself, plus the two server-side members the runner threads + * onto it. Exported so an implementor can NAME what it receives. + * + * ⚠️ `rest-server.ts` declares a structurally identical local alias for the + * door's own dispatch sites (`ServerScopedDataRequest`). The two are not + * converged here because that file is held by a sibling change; the difference + * is the `context` slot, which stays `any` on this side because that is what + * this file's own members already spelled. + */ +export type ImportProtocolRequest = R & { context?: any; environmentId?: string }; + +/** + * Minimal protocol surface the runner needs (find / create / update). + * + * ⭐ [#16952] Every member states the request it is handed, and this + * declaration is the ONLY place the dialect is stated. It used to be + * `args: any` on all three required members, which is why the dialect ended up + * written down in three places that no compiler reads — a prose comment and a + * runtime read in `plugin-auth`'s hand-written implementor, and a local + * parameter annotation in a test double. An implementor had nothing to compile + * against and could only freeze on the spelling it happened to observe; when + * #16638 moved the runner's literals to the canonical QueryAST, the frozen read + * went `undefined` and its `?? {}` default degraded a duplicate probe into + * match-everything, so an admin user import updated the WRONG user. Nothing + * caught it, because the parameter was `any`. + * + * ⇒ Compiled against `FindDataRequest` / `CreateDataRequest` / + * `UpdateDataRequest`, a wire alias (`$filter`, `$top`, …) is a compile error + * in the IMPLEMENTOR rather than a payload no schema has seen. `QuerySchema` + * declares `where` / `limit` / `offset` / `fields` / `orderBy` / `expand`; it + * declares neither `$filter` nor `$top`, and declaring those at the HTTP door + * is #16066's spec half, not this interface's business. + * + * ⛔ An implementor that annotates its own parameter `any` opts back out of all + * of this — the annotation wins over the contextual type. Leave the parameter + * unannotated and let this declaration type it. + */ export interface ImportProtocolLike { - findData(args: any): Promise; - createData(args: any): Promise; - updateData(args: any): Promise; + findData(args: ImportProtocolRequest): Promise; + createData(args: ImportProtocolRequest): Promise; + updateData(args: ImportProtocolRequest): Promise; /** * Optional bulk-create primitive. When present, `runImport` batches * CREATE-resolved rows through it instead of one `createData` call per * row — see framework#2678. Must resolve to `{ records: any[] }` with one * record per input row, in the same order. */ - createManyData?(args: { object: string; records: any[]; context?: any; environmentId?: string }): Promise<{ records: any[] }>; + createManyData?(args: ImportProtocolRequest<{ object: string; records: any[] }>): Promise<{ records: any[] }>; /** * Optional partial-success bulk create (framework#3172). When present it is * preferred over `createManyData`: one outcome per input row, in order — a @@ -110,7 +148,7 @@ export interface ImportProtocolLike { * the whole-batch degradation that re-runs beforeInsert hooks on the good * rows. */ - insertManyData?(args: { object: string; records: any[]; context?: any; environmentId?: string }): Promise<{ outcomes: Array<{ ok: boolean; record?: any; error?: unknown }> }>; + insertManyData?(args: ImportProtocolRequest<{ object: string; records: any[] }>): Promise<{ outcomes: Array<{ ok: boolean; record?: any; error?: unknown }> }>; /** * Validate-only (#6037 — #4633 ruling D). The write path's verdict on a * candidate row, with nothing persisted. The dry run routes through THIS @@ -126,7 +164,7 @@ export interface ImportProtocolLike { * findings ITS write never produces — a false alarm dressed as coverage. * Such a dry run reports coercion + create/update/skip resolution only. */ - validateData?(args: ValidateDataRequest & { context?: any; environmentId?: string }): Promise; + validateData?(args: ImportProtocolRequest): Promise; } export interface RunImportOptions { diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts index cbb66d0d00..a88ece9c64 100644 --- a/packages/rest/src/index.ts +++ b/packages/rest/src/index.ts @@ -40,6 +40,7 @@ export type { ImportRunSummary, ImportUndoLog, ImportProtocolLike, + ImportProtocolRequest, RunImportOptions, } from './import-runner.js'; export { coerceRow } from './import-coerce.js'; 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 846851ca33..6e9062f1b6 100644 --- a/packages/rest/src/rest-server-canonical-query-ast.test.ts +++ b/packages/rest/src/rest-server-canonical-query-ast.test.ts @@ -76,6 +76,7 @@ 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'; +import type { ImportProtocolLike } from './import-runner.js'; const HERE = dirname(fileURLToPath(import.meta.url)); const sourceOf = (file: string) => readFileSync(resolve(HERE, file), 'utf8'); @@ -303,11 +304,57 @@ describe('[#16638] §1 the three sites import-runner.ts names are canonical, and // 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) ?? []; + const erased = withoutCommentLines(IMPORT_RUNNER).match(ERASED_PARAM) ?? []; expect(erased, 'a query-carrying parameter typed as any puts every literal handed to it back outside the compiler').toEqual([]); }); }); +// --------------------------------------------------------------------------- +// §1b [#16952] The EXPORTED extension point — the erasure the detector missed +// --------------------------------------------------------------------------- + +/** + * ⭐ [#16952] `args` joined `query` / `request` here because the erasure that + * survived #16638 was spelled with it: `ImportProtocolLike`'s three required + * members were `findData(args: any)` / `createData(args: any)` / + * `updateData(args: any)`, so the detector above swept the whole file and + * reported nothing while the EXPORTED extension point declared no dialect at + * all. A detector that enumerates parameter names it has already seen closes + * yesterday's instance; the name an implementor actually writes is `args`. + */ +const ERASED_PARAM = /\(\s*(?:args|query|request)\s*:\s*any\b/g; + +describe('[#16952] §1b the exported `ImportProtocolLike` declares what it is handed', () => { + it('no required member takes `any` — the three that did are named', () => { + // Source-read rather than type-read on purpose: §2 below cannot tell a + // reverted signature from a declared one that happens to admit + // everything, and `any` admits everything. + expect(IMPORT_RUNNER).toContain('findData(args: ImportProtocolRequest): Promise;'); + expect(IMPORT_RUNNER).toContain('createData(args: ImportProtocolRequest): Promise;'); + expect(IMPORT_RUNNER).toContain('updateData(args: ImportProtocolRequest): Promise;'); + }); + + it('the envelope is declared ONCE and every member of the interface uses it', () => { + // The interface used to spell `{ context?: any; environmentId?: string }` + // inline on each of its already-typed members. One declaration is the + // point of this card; three copies of it would be the same defect a + // level down. + expect(IMPORT_RUNNER).toMatch(/export type ImportProtocolRequest = R & \{ context\?: any; environmentId\?: string \};/); + const inlineEnvelopes = IMPORT_RUNNER.match(/context\?: any; environmentId\?: string/g) ?? []; + expect(inlineEnvelopes, 'the envelope is declared once, not re-spelled per member').toHaveLength(1); + }); + + it('CONTROL: the erasure detector fires on the exact spelling this card removed', () => { + // Without this the empty result above is equally consistent with a + // detector that never matches `args` at all — which is precisely how + // the erasure survived the previous card. + expect(' findData(args: any): Promise;'.match(ERASED_PARAM)).toHaveLength(1); + expect('const findArgsBase = (request: any) => ({'.match(ERASED_PARAM)).toHaveLength(1); + // …and does NOT fire on the declared form, so a green is a green. + expect(' findData(args: ImportProtocolRequest): Promise;'.match(ERASED_PARAM)).toBeNull(); + }); +}); + // --------------------------------------------------------------------------- // §2 The type-level half — what the declared contract actually admits // --------------------------------------------------------------------------- @@ -351,6 +398,68 @@ describe('[#16337] §2 the declared `FindDataRequest[\'query\']` contract', () = }); }); +/** + * ⭐ [#16952] The type-level half of the extension point itself. Every alias + * below is derived from the EXPORTED declaration with `Parameters<…>`, so it + * cannot drift into a fourth restatement of the dialect: revert + * `ImportProtocolLike.findData` to `any` and these become `any` too, every + * `@ts-expect-error` in the block goes unused, and `tsconfig.test.json` reds + * the whole file with TS2578. That is the ablation this section is written to + * lose. + */ +type FindArgs = Parameters[0]; +type CreateArgs = Parameters[0]; +type UpdateArgs = Parameters[0]; + +describe('[#16952] §2 the declared `ImportProtocolLike` parameter contract', () => { + it('admits what the runner sends, and refuses the wire dialect (compile-time)', () => { + // Exactly the three literals `import-runner.ts` builds, envelope included. + const find: FindArgs = { + object: 'sys_user', + query: { object: 'sys_user', where: { email: 'a@b.c' }, limit: 2 }, + context: { isSystem: true }, + environmentId: 'env_1', + }; + const create: CreateArgs = { object: 'task', data: { name: 'r0' }, context: {}, environmentId: 'env_1' }; + const update: UpdateArgs = { object: 'task', id: 'id_1', data: { name: 'r0' }, context: {} }; + expect([find.object, create.object, update.id]).toEqual(['sys_user', 'task', 'id_1']); + + // ⭐ The dialect an implementor used to freeze on, now refused at the + // extension point rather than observed from it. Each directive is LIVE + // — an unused `@ts-expect-error` is TS2578 under `tsconfig.test.json`. + // @ts-expect-error `$filter` is the wire spelling; the declared key is `where` + const wireFilter: FindArgs = { object: 'sys_user', query: { object: 'sys_user', $filter: { email: 'a@b.c' } } }; + // @ts-expect-error `$top` is the wire spelling; the declared key is `limit` + const wireTop: FindArgs = { object: 'sys_user', query: { object: 'sys_user', $top: 2 } }; + // @ts-expect-error `object` is REQUIRED on every declared request + const noObject: FindArgs = { query: { object: 'sys_user', where: {} } }; + // @ts-expect-error `data` is REQUIRED on a create + const noData: CreateArgs = { object: 'task' }; + // @ts-expect-error `id` is REQUIRED on an update + const noId: UpdateArgs = { object: 'task', data: { name: 'r0' } }; + expect([wireFilter, wireTop, noObject, noData, noId]).toHaveLength(5); + }); + + it('an implementor written against the declaration needs no annotation of its own', () => { + // ⭐ The whole point: leave the parameter unannotated and the contract + // types it. This is the shape `plugin-auth`'s hand-written implementor + // could not have while the declaration said `any`. + const probes: Array | undefined> = []; + const p: ImportProtocolLike = { + findData: async (args) => { probes.push(args.query?.where); return { records: [] }; }, + createData: async (args) => ({ id: String(args.data.name) }), + updateData: async (args) => ({ id: args.id }), + }; + return Promise.all([ + p.findData({ object: 'sys_user', query: { object: 'sys_user', where: { email: 'a@b.c' } } }), + p.createData({ object: 'task', data: { name: 'r0' } }), + p.updateData({ object: 'task', id: 'id_1', data: { name: 'r1' } }), + ]).then(() => { + expect(probes).toEqual([{ email: 'a@b.c' }]); + }); + }); +}); + // --------------------------------------------------------------------------- // §3 The equivalence receipt — driven through the REAL normalizer // ---------------------------------------------------------------------------