Skip to content

Commit 58a03d2

Browse files
os-zhuangclaude
andauthored
feat(objectql): engine option bags are a closed contract — unknown keys throw instead of silently doing nothing (#4400)
* docs(spec): driver.find example shows canonical QueryAST keys, not wire spellings The @example taught filters/sort/top — keys a driver can never receive: the engine folds/rejects wire spellings before this layer (#4346, #4371), and QuerySchema declares where/orderBy/limit. Also dedupes the doubled @returns tag. Comment-only; no generated artifact reads @example (all 8 gates verified up to date). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(objectql,spec,metadata-protocol,service-queue): engine option bags are a closed contract — unknown keys throw (#4371 option 2) All six engine methods now reject non-null option keys they do not execute, naming the legal set per method; cursor/distinct quote their #4286 tombstone; null stays a withdrawal. Legal sets = schema keys + documented extras (searchFields — now DECLARED on EngineQueryOptionsSchema, it was read and sent all along; onFieldsDropped on update; driver pass-through keys on the methods whose bag reaches driver options — count/aggregate forward nothing, so they reject those too). A drift pin holds the sets equal to the schemas. Same sweep also closes what the option-2 caller survey turned up: - bag object no longer overrides the resolved AST object ({...query, object} order fix); protocol refuses a contradicting POST-body object (400 QUERY_OBJECT_MISMATCH) and strips its own vocabulary (object/count/ tombstones/non-aggregate having) off the engine bag - nested expand ASTs reject the four wire-only spellings like the top level - $search/$searchFields engine reads removed (protocol normalizes to bare) - DbQueueAdapter purge/purgeFailed passed {id} — purge deleted NOTHING (throw swallowed into warn), purgeFailed always threw; both now pass {where: {id}}, and the test fake stops accepting the signature the real engine rejects - 19 test bags carrying the dead 'filters' key cleaned up (two were intent-carrying: a locale multi-update and an oidc dogfood assert that both silently scanned unfiltered) - driver.find JSDoc example shows canonical QueryAST keys Closes #4371 (option 1 landed in #4387). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent d25a0ec commit 58a03d2

14 files changed

Lines changed: 552 additions & 36 deletions

File tree

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
"@objectstack/objectql": patch
3+
"@objectstack/spec": patch
4+
"@objectstack/metadata-protocol": patch
5+
"@objectstack/service-queue": patch
6+
---
7+
8+
fix(objectql,spec,metadata-protocol,service-queue): engine option bags are now a closed contract — unknown keys throw instead of silently doing nothing (#4371 option 2)
9+
10+
The engine declares `Engine*OptionsSchema` but never parses it at runtime, so
11+
any option key outside the contract — a typo (`orderby`), a retired key
12+
(`cursor`), a wire-protocol leftover (`object`, `count`), a key that only
13+
works on other methods (`tenantId` on `count`) — rode along and was silently
14+
ignored. All six methods now reject non-null unknown keys, naming the legal
15+
set; retired keys (`cursor`/`distinct`) quote their #4286 tombstone; `null`
16+
stays a withdrawal.
17+
18+
Per-method legal keys = the method's schema keys plus the documented extras:
19+
`searchFields` (now declared on `EngineQueryOptionsSchema` — it was read by
20+
the engine's `$search` expansion and sent by the protocol layer all along),
21+
`onFieldsDropped` on `update` (contract-declared write observability), and
22+
the driver pass-through keys (`transaction`, `tenantId`, `tenantIds`,
23+
`timezone`, `bypassTenantAudit`, `preserveAudit`) on `find`/`findOne`/
24+
`update`/`delete` — the methods whose bag actually reaches driver options.
25+
`count`/`aggregate` never forward their bag, so pass-through keys there are
26+
rejected rather than accepted-and-ignored. A drift pin holds the sets equal
27+
to the schemas.
28+
29+
Also closed in the same sweep:
30+
31+
- A bag-level `object` key used to OVERRIDE the resolved object on the query
32+
AST (`{ object, ...query }` spread order), splitting `ast.object` from the
33+
table actually queried. The AST now keeps the resolved name; a direct call
34+
passing `object` is rejected, and the protocol layer refuses a POST-body
35+
`object` that contradicts the route (400 `QUERY_OBJECT_MISMATCH`) instead
36+
of picking a winner.
37+
- `findData` no longer leaks protocol-layer vocabulary (`object`, `count`,
38+
`joins`, `windowFunctions`, `cursor`, `distinct`, non-aggregate `having`)
39+
onto the engine bag.
40+
- Nested expand ASTs (`expand: { rel: { sort } }`) reject the four wire-only
41+
spellings exactly like the top-level bag (#4371 option 1 did the top level).
42+
- The engine's OData-spelling reads (`$search`/`$searchFields`) are gone —
43+
the protocol normalizes to the bare keys; a direct call passing them now
44+
throws instead of half-working on one method.
45+
- `DbQueueAdapter.purge`/`purgeFailed` passed `{ id }` — a key the engine
46+
never read, so purge deleted NOTHING (each delete threw into a warn-level
47+
catch) and purgeFailed always threw. Both now pass `{ where: { id } }`;
48+
the test fake's `delete` no longer accepts the signature the real engine
49+
rejects.
50+
51+
Migration for direct engine callers (wire/HTTP callers are unaffected): pass
52+
only the keys your method's `Engine*OptionsSchema` declares (plus the extras
53+
above). Anything else previously did nothing — delete it, or move it to the
54+
layer that owns it.

content/docs/references/data/data-engine.mdx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,7 @@ QueryAST-aligned query options for IDataEngine.find() operations
548548
| **top** | `number` | optional | |
549549
| **cursor** | `any` | optional | [REMOVED] `query.cursor` was removed in @objectstack/spec 17 (#4286, ADR-0049) — no driver ever implemented keyset pagination, so the cursor was accepted and ignored and every page came back identical (a caller looping "until hasMore is false" never terminates). Delete the key; `QueryBuilder.cursor()` was removed with it. Express the keyset as an ordinary `where` predicate on your sort key — `where: { created_at: { $gt: last.created_at } }` with the matching `orderBy` — which every driver executes with canonicalised comparands. A first-class cursor, if ever built, will be a response-minted opaque token, not this caller-built record. |
550550
| **search** | `{ query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }` | optional | |
551+
| **searchFields** | `string[]` | optional | |
551552
| **expand** | `Record<string, { object: string; fields?: string[]; where?: any; search?: string \| { query: string; fields?: string[]; fuzzy: boolean; operator: Enum<'and' \| 'or'>; … }; … }>` | optional | |
552553
| **distinct** | `any` | optional | [REMOVED] `query.distinct` was removed in @objectstack/spec 17 (#4286, ADR-0049 / ADR-0078) — no driver ever rendered SELECT DISTINCT; the flag's only observable effect was MIS-WIRED: the REST list path treated a distinct query as not countable and silently degraded `total`/`hasMore` to a page-local estimate while still returning duplicate rows. Delete the key; `QueryBuilder.distinct()` was removed with it, and the count suppression is gone (`total` is truthful again). For unique values of one column use the SQL/memory drivers' `distinct(object, field)` door; for unique combinations, `groupBy`; for a deduplicated count, the `count_distinct` aggregation. |
553554

packages/metadata-protocol/src/protocol.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4434,6 +4434,35 @@ export class ObjectStackProtocolImplementation implements
44344434
};
44354435
}
44364436

4437+
// [#4371 option 2] Strip the PROTOCOL-layer keys off the bag before it
4438+
// becomes the engine option bag — the engine now rejects option keys
4439+
// it does not execute, and these belong to this layer, not to it:
4440+
// - `object`: the POST-body convenience copy of the route object
4441+
// (reserved above so it is not read as a field filter). It used to
4442+
// ride the spread into the engine AST and OVERRIDE the resolved
4443+
// object, splitting `ast.object` from the table actually queried —
4444+
// a mismatch is refused, never resolved by picking a winner.
4445+
// - `count`: a response-shape flag this method consumed above.
4446+
// - QueryAST tombstones (`cursor`/`joins`/`windowFunctions`/
4447+
// `distinct`): reserved at the wire gate so they are not read as
4448+
// field filters; on the wire they stay ignored-with-tombstone-docs
4449+
// (schema parse is where they 400), and they must not leak to the
4450+
// engine as junk.
4451+
// - `having`: consumed by the aggregate branch above; the find path
4452+
// cannot serve it.
4453+
if (options.object != null && options.object !== request.object) {
4454+
const err: any = new Error(
4455+
`Conflicting object: the route addresses '${request.object}' but the query body ` +
4456+
`says '${options.object}'. The body 'object' key is a convenience copy of the ` +
4457+
'route object and must match it.',
4458+
);
4459+
err.status = 400;
4460+
err.code = 'QUERY_OBJECT_MISMATCH';
4461+
throw err;
4462+
}
4463+
for (const k of ['object', 'count', 'joins', 'windowFunctions', 'cursor', 'distinct', 'having']) {
4464+
delete options[k];
4465+
}
44374466
const records = await this.engine.find(request.object, options);
44384467
// Pagination metadata. When a `limit` is present the response is a single
44394468
// page, so `records.length` is the page size — NOT the match total. Run a
Lines changed: 233 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #4371 option (2) — an engine option-bag key the engine does not execute is
5+
* REJECTED at the entry point instead of silently ignored.
6+
*
7+
* The engine declares a query contract (`Engine*OptionsSchema`) but never
8+
* parses it at runtime, so before this gate ANY key outside the contract —
9+
* a typo (`orderby`), a retired key (`cursor`), a wire-protocol leftover
10+
* (`object`, `count`), a key that only works on OTHER methods (`tenantId` on
11+
* `count`) — rode along and was dropped without a trace. The per-method legal
12+
* sets mirror the schemas plus the documented extras (`searchFields`,
13+
* `onFieldsDropped`, the driver pass-through keys); the drift pin at the
14+
* bottom keeps them from falling out of step with the spec.
15+
*/
16+
17+
import { describe, it, expect, beforeEach } from 'vitest';
18+
import {
19+
EngineQueryOptionsSchema,
20+
EngineUpdateOptionsSchema,
21+
EngineDeleteOptionsSchema,
22+
EngineCountOptionsSchema,
23+
EngineAggregateOptionsSchema,
24+
} from '@objectstack/spec/data';
25+
import { ObjectQL, ENGINE_OPTION_KEY_SETS } from './engine.js';
26+
27+
const task = {
28+
name: 'task',
29+
label: 'Task',
30+
fields: {
31+
id: { name: 'id', type: 'text' as const, primaryKey: true },
32+
title: { name: 'title', type: 'text' as const },
33+
status: { name: 'status', type: 'text' as const },
34+
owner: { name: 'owner', type: 'lookup' as const, reference: 'person' },
35+
},
36+
};
37+
const person = {
38+
name: 'person',
39+
label: 'Person',
40+
fields: {
41+
id: { name: 'id', type: 'text' as const, primaryKey: true },
42+
name: { name: 'name', type: 'text' as const },
43+
},
44+
};
45+
46+
function makeDriver() {
47+
const stores = new Map<string, Map<string, Record<string, unknown>>>();
48+
const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; };
49+
const finds: any[] = [];
50+
let nextId = 0;
51+
const matches = (row: any, where: any): boolean => {
52+
if (!where || typeof where !== 'object') return true;
53+
for (const [k, v] of Object.entries(where)) {
54+
if (k === '$and') return (v as any[]).every((w) => matches(row, w));
55+
if (k.startsWith('$')) continue;
56+
const exp = (v && typeof v === 'object' && '$in' in (v as any)) ? (v as any).$in : v;
57+
if (Array.isArray(exp)) { if (!exp.includes(row[k])) return false; }
58+
else if ((row[k] ?? null) !== (exp ?? null)) return false;
59+
}
60+
return true;
61+
};
62+
const run = (o: string, ast: any) => {
63+
let rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where));
64+
if (Array.isArray(ast?.orderBy) && ast.orderBy.length > 0) {
65+
rows = [...rows].sort((a: any, b: any) => {
66+
for (const { field, order } of ast.orderBy) {
67+
const cmp = String(a?.[field] ?? '').localeCompare(String(b?.[field] ?? ''));
68+
if (cmp !== 0) return order === 'desc' ? -cmp : cmp;
69+
}
70+
return 0;
71+
});
72+
}
73+
return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows;
74+
};
75+
const driver: any = {
76+
name: 'memory', version: '0.0.0', supports: {},
77+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
78+
async find(o: string, ast: any) { finds.push(ast); return run(o, ast); },
79+
async findOne(o: string, ast: any) { finds.push(ast); return run(o, ast)[0] ?? null; },
80+
findStream() { throw new Error('ns'); },
81+
async create(o: string, data: any) { nextId += 1; const id = data.id ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row; },
82+
async update(o: string, id: string, data: any) { const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error('nf'); const up = { ...cur, ...data, id }; s.set(id, up); return up; },
83+
async updateMany(o: string, ast: any, data: any) { let n = 0; for (const [id, row] of storeFor(o)) { if (!matches(row, ast?.where)) continue; storeFor(o).set(id, { ...row, ...data, id }); n += 1; } return n; },
84+
async delete(o: string, id: string) { return storeFor(o).delete(id); },
85+
async deleteMany(o: string, ast: any) { let n = 0; for (const [id, row] of [...storeFor(o)]) { if (!matches(row, ast?.where)) continue; storeFor(o).delete(id); n += 1; } return n; },
86+
async count(o: string, ast: any) { return run(o, ast).length; },
87+
async aggregate(o: string, ast: any) { return [{ n: run(o, ast).length }]; },
88+
async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); },
89+
async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {},
90+
};
91+
return { driver, stores, finds };
92+
}
93+
94+
describe('unknown engine option keys are rejected (#4371 option 2)', () => {
95+
let engine: ObjectQL;
96+
let finds: any[];
97+
let a: any;
98+
99+
beforeEach(async () => {
100+
engine = new ObjectQL();
101+
const mem = makeDriver();
102+
finds = mem.finds;
103+
engine.registerDriver(mem.driver, true);
104+
await engine.init();
105+
engine.registry.registerObject(task as any);
106+
engine.registry.registerObject(person as any);
107+
await engine.insert('person', { id: 'p1', name: 'P' });
108+
a = await engine.insert('task', { title: 'A', status: 'open', owner: 'p1' });
109+
await engine.insert('task', { title: 'B', status: 'done', owner: 'p1' });
110+
finds.length = 0;
111+
});
112+
113+
// ── each method refuses a key it does not execute ───────────────────
114+
115+
it.each([
116+
['find', () => engine.find('task', { bogus: 1 } as any)],
117+
['findOne', () => engine.findOne('task', { bogus: 1 } as any)],
118+
['update', () => engine.update('task', { title: 'Z' }, { where: { id: 'x' }, bogus: 1 } as any)],
119+
['delete', () => engine.delete('task', { where: { id: 'x' }, bogus: 1 } as any)],
120+
['count', () => engine.count('task', { bogus: 1 } as any)],
121+
['aggregate', () => engine.aggregate('task', { aggregations: [{ function: 'count', alias: 'n' }], bogus: 1 } as any)],
122+
])('%s rejects an unknown option, naming the legal set', async (method, call) => {
123+
await expect(call()).rejects.toThrow(
124+
new RegExp(`${method}\\('task'\\) does not recognise option 'bogus'.*Legal keys for ${method}:`, 's'),
125+
);
126+
});
127+
128+
it('a typo like `orderby` is caught, not silently unsorted', async () => {
129+
await expect(engine.find('task', { orderby: [{ field: 'title', order: 'asc' }] } as any))
130+
.rejects.toThrow(/'orderby'/);
131+
expect(finds).toHaveLength(0);
132+
});
133+
134+
it('several unknown keys are reported in ONE rejection', async () => {
135+
await expect(engine.find('task', { bogus: 1, extra: 2 } as any))
136+
.rejects.toThrow(/options 'bogus'; 'extra'/);
137+
});
138+
139+
// ── retired keys quote their tombstone ──────────────────────────────
140+
141+
it.each(['cursor', 'distinct'])('%s is rejected with its #4286 tombstone', async (key) => {
142+
await expect(engine.find('task', { [key]: 'x' } as any))
143+
.rejects.toThrow(/#4286, ADR-0049/);
144+
});
145+
146+
// ── null stays a withdrawal ─────────────────────────────────────────
147+
148+
it('a null-valued unknown key is a withdrawal, not a rejection', async () => {
149+
const rows = await engine.find('task', { bogus: null } as any);
150+
expect(rows).toHaveLength(2);
151+
});
152+
153+
// ── the documented extras stay legal where they work ────────────────
154+
155+
it('driver pass-through keys (tenantId, bypassTenantAudit) pass on find/update/delete', async () => {
156+
await engine.find('task', { tenantId: 't1', bypassTenantAudit: true } as any);
157+
await engine.update('task', { title: 'A2' }, { where: { id: a.id }, tenantId: 't1' } as any);
158+
await engine.delete('task', { where: { id: a.id }, bypassTenantAudit: true } as any);
159+
});
160+
161+
it.each([
162+
['count', () => engine.count('task', { tenantId: 't1' } as any)],
163+
['aggregate', () => engine.aggregate('task', { aggregations: [{ function: 'count', alias: 'n' }], tenantId: 't1' } as any)],
164+
])('%s rejects driver pass-through keys — its bag never reaches the driver options', async (_m, call) => {
165+
await expect(call()).rejects.toThrow(/'tenantId'/);
166+
});
167+
168+
it('onFieldsDropped passes on update (contract-declared write observability)', async () => {
169+
await engine.update('task', { title: 'A3' }, { where: { id: a.id }, onFieldsDropped: () => {} } as any);
170+
});
171+
172+
it('searchFields passes on find (read by the $search expansion)', async () => {
173+
// Legality pin only — the $or/$contains expansion itself is covered by
174+
// the search-filter tests; this mock driver does not evaluate it.
175+
await expect(engine.find('task', { search: 'A', searchFields: ['title'] } as any))
176+
.resolves.toBeDefined();
177+
});
178+
179+
it('the OData spellings $search/$searchFields are unknown keys — wire-only, protocol folds them', async () => {
180+
await expect(engine.find('task', { $search: 'A' } as any)).rejects.toThrow(/'\$search'/);
181+
});
182+
183+
it('a stray `object` key is refused — it used to OVERRIDE the resolved object on the AST', async () => {
184+
await expect(engine.find('task', { object: 'person' } as any)).rejects.toThrow(/'object'/);
185+
expect(finds).toHaveLength(0);
186+
});
187+
188+
// ── expand nested ASTs reject wire spellings too ────────────────────
189+
190+
it("expand: { owner: { sort } } is refused — nested wire spellings were silently dropped", async () => {
191+
await expect(
192+
engine.find('task', { expand: { owner: { object: 'person', sort: { name: 'asc' } } } } as any),
193+
).rejects.toThrow(/expand\['owner'\] on 'task' does not accept 'sort'.*Pass 'orderBy'/s);
194+
});
195+
196+
it('expand with canonical nested keys works', async () => {
197+
const rows = await engine.find('task', {
198+
where: { id: a.id },
199+
expand: { owner: { object: 'person', fields: ['id', 'name'] } },
200+
} as any);
201+
expect(rows[0].owner).toMatchObject({ id: 'p1', name: 'P' });
202+
});
203+
204+
// ── drift pin: legal sets stay glued to the spec schemas ────────────
205+
206+
it('each legal set covers its schema shape (minus tombstones) and only the documented extras', () => {
207+
const TOMBSTONES = new Set(['cursor', 'distinct']);
208+
const PASSTHROUGH = ['transaction', 'tenantId', 'tenantIds', 'timezone', 'bypassTenantAudit', 'preserveAudit'];
209+
const expectSetMatches = (
210+
setName: string,
211+
schema: { shape: Record<string, unknown> },
212+
extras: string[],
213+
) => {
214+
const legal = ENGINE_OPTION_KEY_SETS[setName];
215+
const schemaKeys = Object.keys(schema.shape).filter((k) => !TOMBSTONES.has(k));
216+
for (const k of schemaKeys) {
217+
// `top` folds into `limit` before the gate and never reaches it.
218+
if (k === 'top') continue;
219+
expect(legal.has(k), `${setName} must accept schema key '${k}'`).toBe(true);
220+
}
221+
const documented = new Set([...schemaKeys, ...extras]);
222+
for (const k of legal) {
223+
expect(documented.has(k), `${setName} accepts '${k}' which neither the schema nor the documented extras declare`).toBe(true);
224+
}
225+
};
226+
expectSetMatches('find', EngineQueryOptionsSchema as any, PASSTHROUGH);
227+
expectSetMatches('findOne', EngineQueryOptionsSchema as any, PASSTHROUGH);
228+
expectSetMatches('update', EngineUpdateOptionsSchema as any, ['onFieldsDropped', ...PASSTHROUGH]);
229+
expectSetMatches('delete', EngineDeleteOptionsSchema as any, PASSTHROUGH);
230+
expectSetMatches('count', EngineCountOptionsSchema as any, []);
231+
expectSetMatches('aggregate', EngineAggregateOptionsSchema as any, []);
232+
});
233+
});

packages/objectql/src/engine-validation-locale.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ describe('engine write path — validation messages honour ExecutionContext.loca
130130
ql.update(
131131
'mes_settlement',
132132
{ quota_hours: -3 },
133-
{ multi: true, filters: [['name', '=', 'S1']], context: zhCN } as any,
133+
{ multi: true, where: { name: 'S1' }, context: zhCN } as any,
134134
),
135135
);
136136
expect(msg).toBe('定额工时必须大于或等于 0');

0 commit comments

Comments
 (0)