Skip to content

Commit c39d713

Browse files
os-zhuangclaude
andauthored
fix(objectql): a direct engine call carrying sort/select/skip/populate now throws instead of silently dropping it (#4371) (#4387)
The engine folds filter→where and top→limit itself (#4346); the other four pairs in RPC_QUERY_ALIAS_SLOTS fold at the RPC/protocol layer only, because their value shapes need lowering that belongs there. A direct engine.find() never crosses that layer, so the alias key rode the AST verbatim, drivers read only the canonical name, and the request succeeded with the parameter silently discarded — three shipped instances fixed in #4370, and a fourth in the engine's own seedAutonumber (select — its projection never applied, and the surrounding catch would have swallowed the new rejection into 'seed from 0', i.e. duplicate autonumbers; now passes fields). find/findOne now reject a non-null wire-only spelling with an error naming the canonical key, the layer that owns the fold, and the target shape. Folding them here instead would re-implement the RPC layer's shape lowering per reader — the #3795 condition — so rejecting keeps one fold, one owner. The where-only methods (update/delete/count/aggregate) are deliberately unchanged: their contracts honour no sort/projection/pagination in either spelling, so 'pass orderBy instead' would redirect to a key they silently ignore too — that is #4371 option (2), scoped separately. Call-site survey (this repo + cloud): zero direct callers pass these keys today; every wire ingress folds at the protocol layer (pinned end-to-end). Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 31e0be9 commit c39d713

3 files changed

Lines changed: 300 additions & 4 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): a direct engine call carrying `sort`/`select`/`skip`/`populate` now throws instead of silently dropping the parameter (#4371)
6+
7+
The engine folds `filter``where` and `top``limit` itself (#4346); the other
8+
four pairs in `RPC_QUERY_ALIAS_SLOTS` fold at the RPC/protocol layer only,
9+
because their value shapes need lowering (`sort`'s `{field: 'asc'}` record
10+
form, `populate`'s name list) that belongs there. A **direct** `engine.find()`
11+
/ `findOne()` never crosses that layer, so one of those keys used to ride the
12+
AST verbatim, drivers read only the canonical name, and the request succeeded
13+
with the parameter discarded — `sort` + `limit` ("the latest N") silently
14+
returning an arbitrary N. Three shipped instances were fixed in #4370, and a
15+
fourth sat in the engine's own autonumber seeding (`select` — now `fields`).
16+
17+
`find`/`findOne` now reject a non-null wire-only spelling with an error naming
18+
the canonical key and shape, e.g.:
19+
20+
> `find('task') does not accept 'sort': 'sort' is a wire spelling of
21+
> 'orderBy', folded by the RPC/protocol layer — a direct engine call bypasses
22+
> that fold, so the value would be silently dropped, not applied. Pass
23+
> 'orderBy' (SortNode[]: [{ field, order: 'asc' | 'desc' }]) instead.`
24+
25+
Migration for direct engine callers (HTTP/RPC callers are unaffected — the
26+
wire fold is unchanged): `select: [...]``fields: [...]`;
27+
`sort: {f: 'asc'}` or `sort: [{field, order}]``orderBy: [{field, order}]`;
28+
`skip: n``offset: n`; `populate: ['rel']``expand: {rel: {object: 'rel'}}`.
29+
An explicit `null` under a wire spelling remains a withdrawal (ignored), and
30+
the where-only methods (`update`/`delete`/`count`/`aggregate`) are unchanged —
31+
their contracts honour no sort/projection/pagination in either spelling
32+
(unknown-key enforcement there is #4371's follow-up scope).
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #4371 — a direct engine call carrying a wire-only alias spelling
5+
* (`sort`/`select`/`skip`/`populate`) is REJECTED, not silently dropped.
6+
*
7+
* Regression: #4346 folds `filter`→`where` and `top`→`limit` on every engine
8+
* entry point, but the other four pairs in `RPC_QUERY_ALIAS_SLOTS` fold at the
9+
* RPC/protocol layer only (their value shapes need lowering that belongs
10+
* there). A direct `engine.find()` never crosses that layer, so the alias key
11+
* rode the AST verbatim, drivers read only the canonical name, and the request
12+
* SUCCEEDED with the parameter discarded — `sort` + `limit` ("the latest N")
13+
* silently returning an arbitrary N. Three shipped instances were fixed in
14+
* #4370; a fourth sat in the engine's own `seedAutonumber`. The engine now
15+
* throws at the boundary, naming the canonical key and shape, while the
16+
* RPC/wire path keeps folding exactly as before — the fold must NOT move.
17+
*/
18+
19+
import { describe, it, expect, beforeEach } from 'vitest';
20+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
21+
import { ObjectQL } from './engine.js';
22+
23+
const task = {
24+
name: 'task',
25+
label: 'Task',
26+
fields: {
27+
id: { name: 'id', type: 'text' as const, primaryKey: true },
28+
title: { name: 'title', type: 'text' as const },
29+
status: { name: 'status', type: 'text' as const },
30+
},
31+
};
32+
33+
/** Memory driver that records every AST handed to find/findOne. */
34+
function makeRecordingDriver() {
35+
const stores = new Map<string, Map<string, Record<string, unknown>>>();
36+
const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; };
37+
const finds: any[] = [];
38+
let nextId = 0;
39+
const matches = (row: Record<string, unknown>, where: any): boolean => {
40+
if (!where || typeof where !== 'object') return true;
41+
for (const [k, v] of Object.entries(where)) {
42+
if (k.startsWith('$')) continue;
43+
const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v;
44+
if ((row[k] ?? null) !== (exp ?? null)) return false;
45+
}
46+
return true;
47+
};
48+
const run = (o: string, ast: any) => {
49+
let rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where));
50+
const ord = Array.isArray(ast?.orderBy) ? ast.orderBy : [];
51+
if (ord.length > 0) {
52+
rows = [...rows].sort((a: any, b: any) => {
53+
for (const { field, order } of ord) {
54+
const cmp = String(a?.[field] ?? '').localeCompare(String(b?.[field] ?? ''));
55+
if (cmp !== 0) return order === 'desc' ? -cmp : cmp;
56+
}
57+
return 0;
58+
});
59+
}
60+
if (typeof ast?.offset === 'number' && ast.offset > 0) rows = rows.slice(ast.offset);
61+
return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows;
62+
};
63+
const driver: any = {
64+
name: 'memory', version: '0.0.0', supports: {},
65+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
66+
async find(o: string, ast: any) { finds.push(ast); return run(o, ast); },
67+
findStream() { throw new Error('ns'); },
68+
async findOne(o: string, ast: any) { finds.push(ast); return run(o, ast)[0] ?? null; },
69+
async create(o: string, data: Record<string, unknown>) {
70+
nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row;
71+
},
72+
async update(o: string, id: string, data: Record<string, unknown>) {
73+
const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`);
74+
const up = { ...cur, ...data, id }; s.set(id, up); return up;
75+
},
76+
async delete(o: string, id: string) { return storeFor(o).delete(id); },
77+
async count(o: string, ast: any) { return run(o, ast).length; },
78+
async bulkCreate(o: string, rows: Record<string, unknown>[]) { return Promise.all(rows.map((r) => this.create(o, r))); },
79+
async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {},
80+
};
81+
return { driver, stores, finds };
82+
}
83+
84+
describe('wire-only alias spellings are rejected on direct engine calls (#4371)', () => {
85+
let engine: ObjectQL;
86+
let finds: any[];
87+
88+
beforeEach(async () => {
89+
engine = new ObjectQL();
90+
const mem = makeRecordingDriver();
91+
finds = mem.finds;
92+
engine.registerDriver(mem.driver, true);
93+
await engine.init();
94+
engine.registry.registerObject(task as any);
95+
await engine.insert('task', { title: 'B', status: 'done' });
96+
await engine.insert('task', { title: 'A', status: 'open' });
97+
await engine.insert('task', { title: 'C', status: 'done' });
98+
finds.length = 0; // drop insert-path reads; the pins below own this log
99+
});
100+
101+
// ── each wire spelling throws before the driver sees anything ──────
102+
103+
it.each([
104+
['sort', { created_at: 'asc' }, 'orderBy'],
105+
['select', ['id', 'title'], 'fields'],
106+
['skip', 10, 'offset'],
107+
['populate', ['owner'], 'expand'],
108+
])('find({%s}) is refused, naming the canonical key', async (alias, value, canonical) => {
109+
await expect(engine.find('task', { [alias]: value } as any)).rejects.toThrow(
110+
new RegExp(`wire spelling of '${canonical}'.*Pass '${canonical}'`, 's'),
111+
);
112+
expect(finds).toHaveLength(0);
113+
});
114+
115+
it('findOne({sort}) is refused too — "first row of THIS order" degrades the same way', async () => {
116+
await expect(engine.findOne('task', { sort: { title: 'asc' } } as any))
117+
.rejects.toThrow(/findOne\('task'\) does not accept 'sort'/);
118+
expect(finds).toHaveLength(0);
119+
});
120+
121+
it('a sort already in SortNode[] shape is still refused — the KEY is the contract', async () => {
122+
await expect(engine.find('task', { sort: [{ field: 'title', order: 'asc' }] } as any))
123+
.rejects.toThrow(/wire spelling of 'orderBy'/);
124+
});
125+
126+
it('several wire spellings at once are reported in ONE rejection', async () => {
127+
await expect(
128+
engine.find('task', { sort: { title: 'asc' }, populate: ['owner'] } as any),
129+
).rejects.toThrow(/'sort'.*'populate'|'populate'.*'sort'/s);
130+
});
131+
132+
// ── the canonical spellings all work — the migration target is real ─
133+
134+
it('find({where, fields, orderBy, offset, limit}) reaches the driver canonically', async () => {
135+
const rows = await engine.find('task', {
136+
where: { status: 'done' },
137+
fields: ['id', 'title'],
138+
orderBy: [{ field: 'title', order: 'desc' }],
139+
offset: 0,
140+
limit: 10,
141+
});
142+
expect(rows.map((r: any) => r.title)).toEqual(['C', 'B']);
143+
expect(finds).toHaveLength(1);
144+
expect(finds[0].orderBy).toEqual([{ field: 'title', order: 'desc' }]);
145+
expect(finds[0].sort).toBeUndefined();
146+
});
147+
148+
// ── null stays a withdrawal, exactly like the folded slots ──────────
149+
150+
it('an explicit null wire spelling is a withdrawal, not a rejection', async () => {
151+
const rows = await engine.find('task', { sort: null } as any);
152+
expect(rows).toHaveLength(3);
153+
});
154+
155+
// ── the folded pairs did not regress behind the new parameter ──────
156+
157+
it('find({filter, top}) still folds (#4346) with the rejection guard in place', async () => {
158+
const rows = await engine.find('task', { filter: { status: 'done' }, top: 1 } as any);
159+
expect(rows).toHaveLength(1);
160+
expect(finds[0].where).toEqual({ status: 'done' });
161+
expect(finds[0].limit).toBe(1);
162+
});
163+
164+
// ── the RPC/wire path still folds — the fold must NOT move (#4371 pin 3) ─
165+
166+
it('wire `sort` through the protocol layer folds to orderBy and passes the guard', async () => {
167+
const protocol = new ObjectStackProtocolImplementation(engine as any);
168+
const out: any = await protocol.findData({ object: 'task', query: { sort: '-title' } });
169+
expect(out.records.map((r: any) => r.title)).toEqual(['C', 'B', 'A']);
170+
expect(finds).toHaveLength(1);
171+
expect(finds[0].orderBy).toEqual([{ field: 'title', order: 'desc' }]);
172+
expect(finds[0].sort).toBeUndefined();
173+
});
174+
175+
it('wire `select`/`skip`/`populate` through the protocol layer pass the guard as canonical keys', async () => {
176+
const protocol = new ObjectStackProtocolImplementation(engine as any);
177+
const out: any = await protocol.findData({
178+
object: 'task',
179+
query: { select: 'id,title', skip: 1, sort: 'title' },
180+
});
181+
expect(out.records.map((r: any) => r.title)).toEqual(['B', 'C']);
182+
expect(finds[0].fields).toEqual(expect.arrayContaining(['id', 'title']));
183+
expect(finds[0].offset).toBe(1);
184+
expect(finds[0].select).toBeUndefined();
185+
expect(finds[0].skip).toBeUndefined();
186+
});
187+
});

packages/objectql/src/engine.ts

Lines changed: 81 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,13 +116,57 @@ const ENGINE_WHERE_SLOTS: readonly QueryAliasSlot[] =
116116
const ENGINE_QUERY_SLOTS: readonly QueryAliasSlot[] =
117117
RPC_QUERY_ALIAS_SLOTS.filter((slot) => slot.canonical === 'where' || slot.canonical === 'limit');
118118

119+
/**
120+
* [#4371] The slots the engine does NOT fold — the wire-only pairs
121+
* (`select`→`fields`, `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand`),
122+
* derived as the complement of {@link ENGINE_QUERY_SLOTS} so a seventh pair
123+
* added to the spec table lands on exactly one side of the split.
124+
*
125+
* Their values need shape lowering (`sort`'s `{field: 'asc'}` record form,
126+
* `populate`'s name list) that belongs to the RPC/protocol layers, so folding
127+
* here would re-implement the lowering per reader — the #3795 condition. But a
128+
* DIRECT engine call never crosses those layers: the alias key used to ride
129+
* the AST verbatim, drivers read only the canonical name, and the parameter
130+
* was silently dropped — three shipped "latest N in arbitrary order" bugs
131+
* (#4370) plus the engine's own `seedAutonumber`. Declared ≠ enforced
132+
* (AGENTS.md PD #10): the find-shaped entry points now REJECT these spellings,
133+
* naming the canonical key and shape, so the mistake throws at the call site
134+
* instead of degrading the result.
135+
*
136+
* Deliberately NOT applied to the where-only methods
137+
* (`update`/`delete`/`count`/`aggregate`): their contracts honour no
138+
* sort/projection/pagination at all, so "pass `orderBy` instead" would
139+
* redirect the caller to a key those methods silently ignore too. Unknown-key
140+
* enforcement for those bags is #4371's option (2), scoped separately.
141+
*/
142+
const ENGINE_WIRE_ONLY_SLOTS: readonly QueryAliasSlot[] =
143+
RPC_QUERY_ALIAS_SLOTS.filter((slot) => !ENGINE_QUERY_SLOTS.includes(slot));
144+
145+
/**
146+
* Canonical shape each wire-only slot's value must be rewritten into — quoted
147+
* by the rejection so the error carries the full migration, not just the key
148+
* rename (the value shapes differ; that is WHY the engine cannot fold them).
149+
*/
150+
const WIRE_ONLY_CANONICAL_SHAPES: Record<string, string> = {
151+
fields: "a string[] of field names",
152+
orderBy: "SortNode[]: [{ field, order: 'asc' | 'desc' }]",
153+
offset: 'a number of rows to skip',
154+
expand: 'a record of { relationName: QueryAST }',
155+
};
156+
119157
/**
120158
* Fold the deprecated alias spellings of an engine option bag into their
121159
* canonical QueryAST keys, under the #3795/#4181 rule: an alias alone moves to
122160
* the canonical key, redundant identical spellings collapse, DIFFERENT values
123161
* for one slot are irreconcilable and throw (picking a winner IS the silent
124162
* drop), and an explicit `null` alias is a withdrawal.
125163
*
164+
* `rejectSlots` ({@link ENGINE_WIRE_ONLY_SLOTS}) names the slots whose alias
165+
* spellings the engine can neither fold nor honour: a non-null value under one
166+
* throws, quoting the canonical key and shape (#4371). `null` stays a
167+
* withdrawal here too — it carries no intent a drop could lose — and rides
168+
* through for drivers to ignore, exactly as before.
169+
*
126170
* Returns the SAME reference when no alias spelling is present (the common
127171
* path allocates nothing — `withResolvedWhere` discipline); otherwise folds a
128172
* shallow copy, because the bag belongs to the caller and may be reused (view
@@ -133,8 +177,31 @@ function foldEngineOptionAliases<T extends object | undefined>(
133177
operation: string,
134178
bag: T,
135179
slots: readonly QueryAliasSlot[],
180+
rejectSlots?: readonly QueryAliasSlot[],
136181
): T {
137182
if (!bag) return bag;
183+
if (rejectSlots) {
184+
const refused = rejectSlots.flatMap((slot) =>
185+
slot.aliases
186+
.filter((alias) => (bag as Record<string, unknown>)[alias] != null)
187+
.map((alias) => ({ alias, canonical: slot.canonical })),
188+
);
189+
if (refused.length > 0) {
190+
throw new Error(
191+
`${operation}('${object}') does not accept ` +
192+
`${refused.map((r) => `'${r.alias}'`).join(', ')}: ` +
193+
refused
194+
.map(
195+
(r) =>
196+
`'${r.alias}' is a wire spelling of '${r.canonical}', folded by the RPC/protocol ` +
197+
`layer — a direct engine call bypasses that fold, so the value would be silently ` +
198+
`dropped, not applied. Pass '${r.canonical}' ` +
199+
`(${WIRE_ONLY_CANONICAL_SHAPES[r.canonical] ?? 'the canonical QueryAST shape'}) instead.`,
200+
)
201+
.join(' '),
202+
);
203+
}
204+
}
138205
if (!slots.some((slot) => slot.aliases.some((alias) => alias in bag))) return bag;
139206
const folded: Record<string, unknown> = { ...bag };
140207
foldQueryAliasSlots(folded, slots, (conflict) => {
@@ -1169,8 +1236,13 @@ export class ObjectQL implements IDataEngine {
11691236
execCtx?: ExecutionContextInput,
11701237
): Promise<number> {
11711238
try {
1239+
// Canonical `fields`, not the wire spelling `select` — this call sat on
1240+
// the exact #4371 silent drop (the projection never applied; the scan
1241+
// worked only because an unprojected row still carries `field`), and the
1242+
// catch below would have swallowed the guard's rejection into "seed
1243+
// from 0", i.e. duplicate autonumbers.
11721244
const rows = await this.find(object, {
1173-
select: ['id', field],
1245+
fields: ['id', field],
11741246
limit: 5000,
11751247
context: execCtx,
11761248
} as any);
@@ -2919,8 +2991,11 @@ export class ObjectQL implements IDataEngine {
29192991
// spec's slot table — the driver AST only understands the canonical keys,
29202992
// so an unfolded `{ filter }` would match ALL rows (silent over-grant —
29212993
// surfaced by ADR-0057's sharing/graph read path). Same fold, same
2922-
// conflict rule on every engine entry point (#4346).
2923-
query = foldEngineOptionAliases(object, 'find', query, ENGINE_QUERY_SLOTS);
2994+
// conflict rule on every engine entry point (#4346). The wire-only
2995+
// spellings (`sort`/`select`/`skip`/`populate`) are REJECTED instead of
2996+
// folded — a direct call carrying one used to have it silently dropped
2997+
// (#4371, three shipped instances in #4370).
2998+
query = foldEngineOptionAliases(object, 'find', query, ENGINE_QUERY_SLOTS, ENGINE_WIRE_ONLY_SLOTS);
29242999
this.logger.debug('Find operation starting', { object, query });
29253000
const driver = this.getDriver(object);
29263001
const ast: QueryAST = { object, ...query };
@@ -3056,7 +3131,9 @@ export class ObjectQL implements IDataEngine {
30563131
// matched the first row of the WHOLE table rather than the predicate.
30573132
// `top` folds into `limit` here too, but findOne is single-row by
30583133
// contract, so the literal `limit: 1` below wins over both spellings.
3059-
query = foldEngineOptionAliases(objectName, 'findOne', query, ENGINE_QUERY_SLOTS);
3134+
// Wire-only spellings are rejected, same as find() (#4371) — `sort`
3135+
// matters here too: findOne({ sort }) means "first row of THIS order".
3136+
query = foldEngineOptionAliases(objectName, 'findOne', query, ENGINE_QUERY_SLOTS, ENGINE_WIRE_ONLY_SLOTS);
30603137
this.logger.debug('FindOne operation', { objectName });
30613138
const driver = this.getDriver(objectName);
30623139
const ast: QueryAST = { object: objectName, ...query, limit: 1 };

0 commit comments

Comments
 (0)