Skip to content

Commit 9ca49eb

Browse files
claude[bot]os-project-managerclaude
authored
fix(rest): import-runner builds the canonical QueryAST through a typed findData envelope (#16950)
* fix(rest): import-runner builds canonical QueryAST, through a typed envelope Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 * chore(rest): changeset for the canonical QueryAST rewrite in import-runner Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 * fix(plugin-auth): read the canonical QueryAST in the admin-import protocol `runImport` now sends `where` / `limit`, and it takes an INJECTED `ImportProtocolLike`. The wire-alias folding the runner's rewrite relied on lives in `ObjectStackProtocolImplementation`; a hand-written protocol never passes through it, so `admin-import-users.ts` read `args.query.$filter` and got `undefined`. The failure mode is not a missing filter but an unbounded one: `?? {}` turns the unread key into an empty filter, the upsert duplicate probe stops discriminating, and an admin import updates the wrong user. Both halves were red on this branch (`admin-import-users.test.ts:560` and `:592`). - The adapter reads `args.query.where` / `args.query.limit`, with no `??` behind either. One dialect, and an absent `query` is a loud TypeError rather than a silent match-everything. - The three `import-runner` test doubles read `where` too. Two were red (`import-runner-selfref.test.ts`, `import-runner-bulk.test.ts`); the third was GREEN FOR THE WRONG REASON — its degraded `{}` matched the whole store, so every no-duplicate assertion held without the probe discriminating. - `import-runner-idempotency.test.ts` gains the assertion that closes that: every probe must narrow, and the id recheck is pinned to the `id: { $in: [...] }` over exactly the pre-assigned ids, bounded by `limit`. `ImportProtocolLike.findData(args: any)` is deliberately untouched — narrowing a published extension point is a contract decision and has its own card. Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <pm@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 53ec0b1 commit 9ca49eb

8 files changed

Lines changed: 363 additions & 75 deletions

‎.changeset/import-runner-canonical-query-ast.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/rest": minor
3+
---
4+
5+
`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`.
6+
7+
`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.
8+
9+
- **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.
10+
- **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.
11+
- **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.
12+
13+
⚠️ 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`.

‎.changeset/plugin-auth-admin-import-canonical-query.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
`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`.
6+
7+
`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.
8+
9+
- **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.
10+
- **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.
11+
12+
⚠️ 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.

‎packages/plugins/plugin-auth/src/admin-import-users.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,23 @@ export async function runAdminImportUsers(
349349
}
350350

351351
const protocol: ImportProtocolLike = {
352-
// findExisting path: `{ $filter, $top }` against sys_user.
352+
// [#16638] findExisting path: the CANONICAL QueryAST against sys_user.
353+
// `runImport` hands this handle a `FindDataRequest`, and the `QuerySchema`
354+
// that request declares carries `where` / `limit` — it declares neither
355+
// `$filter` nor `$top`. Those two are wire-only spellings the protocol
356+
// normalizer folds for a caller off the HTTP door; this protocol is
357+
// hand-written and never passes through that normalizer, so it must read
358+
// the canonical keys itself.
359+
//
360+
// Two dialects would be the lenient `??` alias Prime Directive #12 forbids,
361+
// and the default is not a harmless belt either: a `where` that falls back
362+
// to `{}` stops constraining anything, so the duplicate probe matches rows
363+
// it was given no key for and the upsert updates the WRONG user. One
364+
// dialect, read straight — a request that arrives without a `query` is a
365+
// caller defect and costs a loud TypeError, not a silent match-everything.
353366
async findData(args: any) {
354-
const where = args?.query?.$filter ?? {};
355-
const limit = args?.query?.$top ?? 2;
367+
const where = args.query.where;
368+
const limit = args.query.limit;
356369
return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any);
357370
},
358371

‎packages/rest/src/import-runner-bulk.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,9 @@ describe('runImport — bulk create batching (framework#2678)', () => {
148148
}));
149149
const updateData = vi.fn(async (args: { id: string }) => ({ id: args.id }));
150150
// Row 1 ('existing') matches an existing record → update; the rest are creates.
151-
const findData = vi.fn(async (args: { query?: { $filter?: { name?: string } } }) =>
152-
(args.query?.$filter?.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : []));
151+
// [#16638] Reads the CANONICAL `where` the runner sends, not `$filter`.
152+
const findData = vi.fn(async (args: { query: { where: { name?: string } } }) =>
153+
(args.query.where.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : []));
153154
const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData, createManyData };
154155

155156
const summary = await runImport({

‎packages/rest/src/import-runner-idempotency.test.ts‎

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,21 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) {
4949
store.push(rec);
5050
return rec;
5151
});
52-
const findData = vi.fn(async (args: { query?: { $filter?: Record<string, any> } }) => {
53-
const filter = args.query?.$filter ?? {};
52+
// [#16638] Reads the CANONICAL `where` the runner sends. ⛔ The `?? {}` this
53+
// replaces is what made this whole file pass VACUOUSLY once the runner moved
54+
// to `where`: with `$filter` undefined every recheck degraded to `{}`, which
55+
// constrains nothing, so the recheck matched the entire store and the
56+
// no-duplicate assertions below held without the probe discriminating at all.
57+
// Reading `where` straight means an absent filter throws instead.
58+
/**
59+
* [#16638] Every filter this double actually APPLIED. Pinning the payload
60+
* alone would still pass over a double that read the wrong key and defaulted
61+
* to `{}` — which is the vacuity being closed here, so both are recorded.
62+
*/
63+
const appliedFilters: Array<Record<string, any>> = [];
64+
const findData = vi.fn(async (args: { query: { where: Record<string, any>; limit?: number } }) => {
65+
const filter = args.query.where;
66+
appliedFilters.push(filter);
5467
// Supports equality and { $in: [...] } — the id recheck (framework#3173)
5568
// queries by pre-assigned id $in, like the real SQL driver does.
5669
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' } = {}) {
5972
}));
6073
});
6174
const p: ImportProtocolLike = { findData, createData, updateData: vi.fn(), createManyData };
62-
return { p, store, createManyData, createData };
75+
return { p, store, createManyData, createData, findData, appliedFilters };
76+
}
77+
78+
/** One recorded `findData` probe, as the double above receives it. */
79+
type FindProbe = { query: { where: Record<string, any>; limit?: number } };
80+
81+
/**
82+
* ⭐ [#16638] Every probe the runner sends must NARROW — the assertion this
83+
* file was missing, and the reason it stayed GREEN through a payload rewrite
84+
* that broke it. While the double read `args.query.$filter`, a runner sending
85+
* `where` left that read `undefined`, the `?? {}` default turned it into an
86+
* empty filter, and an empty filter constrains NOTHING: every recheck matched
87+
* the entire store, so each `store` / `created` expectation below held without
88+
* the probe discriminating between one row and any other. Passing was not
89+
* evidence. `{}` is the shape that has to be refused, so it is asserted
90+
* against directly.
91+
*
92+
* `Object.keys` on an ABSENT `where` throws rather than reporting zero keys,
93+
* and that is deliberate: a spelling drift must be loud here, not degrade into
94+
* a probe that matches everything.
95+
*/
96+
function expectEveryProbeNarrowed(
97+
calls: ReadonlyArray<readonly [FindProbe]>,
98+
appliedFilters: ReadonlyArray<Record<string, any>>,
99+
): void {
100+
expect(calls.length).toBeGreaterThan(0);
101+
for (const [args] of calls) {
102+
expect(Object.keys(args.query.where)).not.toHaveLength(0);
103+
}
104+
// The payload half is the drift alarm; this is the vacuity half. The filter
105+
// the double APPLIED must be the one it was handed — an equality a `?? {}`
106+
// default breaks even while the runner's payload stays perfectly canonical.
107+
expect(appliedFilters).toEqual(calls.map(([args]) => args.query.where));
108+
for (const filter of appliedFilters) expect(Object.keys(filter)).not.toHaveLength(0);
63109
}
64110

65111
describe('runImport — idempotent retry with natural keys (framework#3149)', () => {
66112
it('upsert+matchFields: a transient retry after commit does not duplicate rows', async () => {
67-
const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' });
113+
const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' });
68114

69115
const summary = await runImport({
70116
...baseOpts, p, writeMode: 'upsert', matchFields: ['name'],
@@ -79,6 +125,10 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', ()
79125
expect(store).toHaveLength(2); // no duplicates
80126
expect(summary.created).toBe(2);
81127
expect(summary.errors).toBe(0);
128+
// ⭐ [#16638] …and every probe that produced those numbers actually
129+
// constrained something. The natural-key probes carry the match field.
130+
expectEveryProbeNarrowed(findData.mock.calls, appliedFilters);
131+
expect(findData.mock.calls.map(([a]) => Object.keys(a.query.where))).toContainEqual(['name']);
82132
});
83133

84134
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)', ()
97147
});
98148

99149
it('pure insert (no matchFields): pre-assigned ids make the retry exactly-once too (#3173)', async () => {
100-
const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' });
150+
const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' });
101151

102152
const summary = await runImport({
103153
...baseOpts, p, writeMode: 'insert', matchFields: [],
@@ -110,6 +160,18 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', ()
110160
expect(store).toHaveLength(2);
111161
expect(summary.created).toBe(2);
112162
expect(summary.errors).toBe(0);
163+
164+
// ⭐ [#16638] The recheck is the whole mechanism of #3173, so pin the
165+
// payload it was handed rather than only the outcome: `id: { $in: [...] }`
166+
// over exactly the ids the runner pre-assigned, bounded to that many rows.
167+
// Read `where` / `limit`, the keys `FindDataRequest` declares — a drift
168+
// back to `$filter` / `$top` reddens here before it reaches an implementor.
169+
expectEveryProbeNarrowed(findData.mock.calls, appliedFilters);
170+
const probes = findData.mock.calls.map(([a]) => a.query);
171+
expect(probes).toHaveLength(1);
172+
expect(Object.keys(probes[0].where)).toEqual(['id']);
173+
expect([...probes[0].where.id.$in].sort()).toEqual(store.map((r) => r.id).sort());
174+
expect(probes[0].limit).toBe(store.length);
113175
});
114176

115177
it('pure insert: legitimate duplicate rows survive the retry intact (each copy has its own id) (#3173)', async () => {

‎packages/rest/src/import-runner-selfref.test.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,10 @@ function makeProtocol(seed: Array<Record<string, any>> = []) {
4242
return rec;
4343
}),
4444
}));
45-
const findData = vi.fn(async (args: { query?: { $filter?: Record<string, any> } }) => {
46-
const filter = args.query?.$filter ?? {};
45+
// [#16638] Reads the CANONICAL `where` the runner sends. ⛔ No `?? {}`: an
46+
// absent filter must throw here, never degrade into a match-everything probe.
47+
const findData = vi.fn(async (args: { query: { where: Record<string, any> } }) => {
48+
const filter = args.query.where;
4749
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; }));
4850
});
4951
const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData: vi.fn(), createManyData };

‎packages/rest/src/import-runner.ts‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { randomUUID } from 'node:crypto';
44
import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js';
55
import type { ExportFieldMeta } from './export-format.js';
66
import type { ValidationMessageTranslator } from '@objectstack/spec/system';
7-
import type { ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api';
7+
import type { FindDataRequest, ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api';
88
import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core';
99
import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types';
1010
import { isEngineDuplicateRecordEnvelope } from './error-response.js';
@@ -357,9 +357,29 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
357357
: Array.isArray(r?.data) ? r.data
358358
: Array.isArray(r?.rows) ? r.rows
359359
: Array.isArray(r) ? r : [];
360-
const findArgsBase = (query: any) => ({
361-
object: '',
362-
query,
360+
// [#16638] The server-scoped envelope for this file's `findData` calls, and
361+
// the reason its parameter is TYPED. It used to be `query: any`, so the three
362+
// call sites below were type-checked by nothing at all and their undeclared
363+
// `$filter` / `$top` wire spellings cost no diagnostic — the same erasure
364+
// #16337 found on `loadImportJob`'s `p: any` handle in `rest-server.ts`,
365+
// whose signpost prescribes exactly this rewrite. Compiled against
366+
// `FindDataRequest` every member is now held to the contract
367+
// `FindDataRequestSchema` declares (`QuerySchema`: `where` / `limit` /
368+
// `offset` / `fields` / `orderBy` / `expand`), so a wire alias is a compile
369+
// error at the call site instead of a payload no schema has seen. It also
370+
// retires the `object: ''` placeholder every caller had to override.
371+
//
372+
// ⚠️ The rewrite is a SPELLING change only: `@objectstack/metadata-protocol`
373+
// folds `$filter`→`where` and `$top`→`limit` by the spec's own
374+
// `RPC_QUERY_ALIAS_SLOTS` with the value moved verbatim, so all three calls
375+
// reach `engine.find` with the same option bag as before —
376+
// `rest-server-canonical-query-ast.test.ts` §3 measures that pair by pair.
377+
//
378+
// ⛔ Server-built means server-built: the wire aliases stay accepted at the
379+
// HTTP door for CALLERS. Declaring them there is #16066's spec half and is
380+
// not this file's business.
381+
const findArgsBase = (request: FindDataRequest) => ({
382+
...request,
363383
...(environmentId ? { environmentId } : {}),
364384
...(context ? { context } : {}),
365385
});
@@ -385,10 +405,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
385405
let match: RefMatch = {};
386406
for (const f of candidates) {
387407
try {
388-
const r = await p.findData({
389-
...findArgsBase({ $filter: { [f]: display }, $top: 2 }),
408+
const r = await p.findData(findArgsBase({
390409
object: referenceObject,
391-
});
410+
query: { object: referenceObject, where: { [f]: display }, limit: 2 },
411+
}));
392412
const recs = findRows(r);
393413
if (recs.length === 0) continue;
394414
if (recs.length > 1) { match = { ambiguous: true, matchedField: f }; break; }
@@ -428,7 +448,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
428448
if (v === undefined || v === null || v === '') return 'blank';
429449
filter[f] = v;
430450
}
431-
const r = await p.findData({ ...findArgsBase({ $filter: filter, $top: 2 }), object: objectName });
451+
const r = await p.findData(findArgsBase({
452+
object: objectName,
453+
query: { object: objectName, where: filter, limit: 2 },
454+
}));
432455
const recs = findRows(r);
433456
if (recs.length === 0) return 'none';
434457
if (recs.length > 1) return 'ambiguous';
@@ -548,10 +571,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
548571
const recheckByIds = async (chunk: Array<Record<string, any>>): Promise<Map<string, any>> => {
549572
const ids = chunk.map((r) => r.id).filter((v) => v != null && v !== '');
550573
if (ids.length === 0) return new Map();
551-
const r = await p.findData({
552-
...findArgsBase({ $filter: { id: { $in: ids } }, $top: ids.length }),
574+
const r = await p.findData(findArgsBase({
553575
object: objectName,
554-
});
576+
query: { object: objectName, where: { id: { $in: ids } }, limit: ids.length },
577+
}));
555578
return new Map(findRows(r).map((rec: any) => [String(rec.id), rec]));
556579
};
557580
const flushPendingCreates = async (): Promise<void> => {

0 commit comments

Comments
 (0)