Skip to content

Commit 071a1d0

Browse files
os-zhuangclaude
andcommitted
fix(objectql,spec): filter folds to where on EVERY engine method; top/limit joins the #3795 slot table (#4346)
The filter → where fold that #3795 settled at the protocol layer existed at the engine layer in exactly one of six methods. find() folded it (with a comment saying why); findOne/count/update/delete/aggregate passed the option bag through with ast.where === undefined, which every driver reads as "no predicate" — so a caller filtering with { filter } silently matched EVERY row: an over-grant on the reads, an unbounded write on update/delete (multi: true rewrote or emptied the whole table). The deprecated DataEngine*OptionsSchema contracts declare filter on all of them, the ScopedContext/ObjectRepository facade forwards hook-body arguments verbatim, and the spec's own hook docs taught the broken call — auth-manager's stampIdentitySource was a live in-repo victim (findOne({filter}) read the table's first row; count({filter}) counted the whole table). Every entry point now folds through the spec's own machinery (RPC_QUERY_ALIAS_SLOTS + foldQueryAliasSlots) instead of find's hand-rolled copy, under the #4181 rule: alias alone folds, identical duplicates collapse, different values throw, an explicit null alias is a withdrawal. Write paths fold BEFORE withResolvedWhere, so filter-carried placeholder tokens resolve too (#3810 parity). top → limit — the pair the #3795 scope note excluded — joins the slot table. The protocol normalizer folded it BACKWARDS (options.limit = Number(options.top): alias overwrote canonical) while engine.find folded it canonical-wins, so {top: 1, limit: 3} answered 1 over HTTP and 3 in-process. All three readers (wire, RPC parse, engine) now agree: top alone still limits, a conflicting pair is refused. The hook.zod.ts / createContext doc examples teach where now. Pinned per method with a real engine + in-memory driver, write paths included (nothing lands before the refusal); the protocol SLOTS table and the RPC parse pins gained the sixth pair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent c5dd9c6 commit 071a1d0

8 files changed

Lines changed: 408 additions & 41 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/objectql": patch
4+
"@objectstack/metadata-protocol": patch
5+
---
6+
7+
fix(objectql,spec): `filter` folds to `where` on EVERY engine method, and `top`/`limit` joins the #3795 slot table (#4346)
8+
9+
The `filter``where` fold that #3795 settled at the protocol layer existed
10+
at the **engine** layer in exactly one of six methods. `ObjectQL.find()`
11+
folded it; `findOne`/`count`/`update`/`delete`/`aggregate` passed the option
12+
bag through with `ast.where === undefined`, which every driver reads as "no
13+
predicate" — so a caller filtering with `{ filter }` silently matched EVERY
14+
row:
15+
16+
| call | before | after |
17+
|---|---|---|
18+
| `findOne({filter: {status:'done'}})` | first row of the table | a matching row |
19+
| `count({filter})` | whole-table count | matching count |
20+
| `update(data, {filter, multi:true})` | **every row rewritten** | matching rows |
21+
| `delete({filter, multi:true})` | **table emptied** | matching rows |
22+
| `aggregate({filter, …})` | aggregated all rows | matching rows |
23+
24+
This was reachable, not theoretical: the deprecated
25+
`DataEngine{Query,Update,Delete,Count,Aggregate}OptionsSchema` contracts all
26+
declare `filter`, `ScopedContext`/`ObjectRepository` (the cross-object API
27+
handed to L2 hook bodies) forwards its argument verbatim, and the spec's own
28+
hook documentation taught the broken call
29+
(`users.findOne({ filter: { role: 'admin' } })` — now corrected to `where`).
30+
31+
Every engine entry point now folds through the spec's own #3795 machinery
32+
(`RPC_QUERY_ALIAS_SLOTS` + `foldQueryAliasSlots`) instead of `find`'s
33+
hand-rolled copy, under the #4181 rule: an alias alone folds, redundant
34+
identical spellings collapse, DIFFERENT values for one slot throw
35+
("Send exactly one") instead of silently picking a winner, and an explicit
36+
`null` alias is a withdrawal.
37+
38+
**The sixth pair.** `top``limit` — the pair the #3795 scope note excluded
39+
as "the OData layer" — joins `RPC_QUERY_ALIAS_SLOTS`. The protocol normalizer
40+
folded it BACKWARDS (`options.limit = Number(options.top)` — the alias
41+
overwrote the canonical key) while `engine.find` folded it canonical-wins, so
42+
`{top: 1, limit: 3}` answered 1 over HTTP and 3 through a direct engine call.
43+
All three readers (wire normalizer, RPC schema parse, engine) now resolve the
44+
pair identically: `top` alone still limits, a conflicting `{top, limit}` is
45+
refused.
46+
47+
Behavior change to note: option bags that previously smuggled conflicting
48+
spellings (`{where: X, filter: Y}`, `{top: 1, limit: 3}`) are now refused
49+
loudly on every path instead of silently resolving differently per layer.
50+
Pinned per method, write paths included — a regression here is silent and
51+
destructive, and the class went unnoticed precisely because `find` was the
52+
only method anyone thought to check.

packages/metadata-protocol/src/protocol.ts

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3676,15 +3676,16 @@ export class ObjectStackProtocolImplementation implements
36763676
delete options[dollar];
36773677
}
36783678

3679-
// [#3795] One slot, one value. Every alias spelling of the five
3679+
// [#3795] One slot, one value. Every alias spelling of the six
36803680
// QueryAST slots resolves HERE, by the spec's own table, before the
36813681
// per-slot wire coercion below ever runs — so that coercion reads
36823682
// canonical keys only. An alias alone folds into its canonical key;
36833683
// redundant identical spellings collapse; different values for one
36843684
// slot are refused (the #4181 rule, generalized from the filter slot
3685-
// to all five — four of which used to resolve BACKWARDS here, each in
3686-
// its own way, disagreeing with the spec's documented precedence and
3687-
// with the runtime dispatcher's copy of the same fold).
3685+
// to all of them — four used to resolve BACKWARDS here, each in its
3686+
// own way, disagreeing with the spec's documented precedence and with
3687+
// the runtime dispatcher's copy of the same fold; `top`→`limit`
3688+
// joined the table last, via #4346).
36883689
//
36893690
// `arrivedAs` remembers which spelling carried each slot's value;
36903691
// composed with `wireSpelling` it names the parameter the caller
@@ -3695,12 +3696,12 @@ export class ObjectStackProtocolImplementation implements
36953696
});
36963697
const slotParam = (canonical: string): string => spellingFor(arrivedAs[canonical] ?? canonical);
36973698

3698-
// Numeric fields — normalize top → limit ($top is the OData layer,
3699-
// outside the #3795 slot table), then coerce querystring strings.
3700-
if (options.top != null) {
3701-
options.limit = Number(options.top);
3702-
delete options.top;
3703-
}
3699+
// Numeric fields — coerce querystring strings. `top` already folded
3700+
// into `limit` above: the pair joined the #3795 slot table with #4346,
3701+
// replacing an open-coded rewrite here that resolved it BACKWARDS
3702+
// (`options.limit = Number(options.top)` — the alias overwrote the
3703+
// canonical key, so `{top: 1, limit: 3}` answered 1 over HTTP and 3
3704+
// through a direct engine call).
37043705
if (options.limit != null) options.limit = Number(options.limit);
37053706
if (options.offset != null) options.offset = Number(options.offset);
37063707

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #4346 — the `filter` → `where` alias folds on EVERY engine entry point, not
5+
* just `find()`, with a REAL {@link ObjectQL} engine + in-memory driver.
6+
*
7+
* Regression: the DataEngine contract (`DataEngine*OptionsSchema`) declares
8+
* `filter` as a legitimate option on every read and write method, but only
9+
* `find()` folded it into the `where` key the driver AST understands.
10+
* `findOne`/`count`/`update`/`delete`/`aggregate` passed the bag through with
11+
* `ast.where === undefined`, which every driver reads as "no predicate" — so a
12+
* caller filtering with `{ filter }` silently matched EVERY row: an over-grant
13+
* on the reads, an unbounded write on `update`/`delete`. The spec's own hook
14+
* documentation taught exactly this call (`users.findOne({ filter: … })`).
15+
*
16+
* The fold now runs once per entry point via the spec's #3795 slot table
17+
* (`RPC_QUERY_ALIAS_SLOTS` + `foldQueryAliasSlots`), under the #4181 rule:
18+
* alias alone folds, identical duplicates collapse, conflicting values are
19+
* refused. Same table also folds `top` → `limit` (canonical wins — the pair
20+
* the #3795 sweep scoped out).
21+
*
22+
* The write-path pins matter most: a regression there is silent and
23+
* destructive, and this whole class went unnoticed because `find` was the only
24+
* method anyone thought to check.
25+
*/
26+
27+
import { describe, it, expect, beforeEach } from 'vitest';
28+
import { ObjectQL } from './engine.js';
29+
30+
const task = {
31+
name: 'task',
32+
label: 'Task',
33+
fields: {
34+
id: { name: 'id', type: 'text' as const, primaryKey: true },
35+
title: { name: 'title', type: 'text' as const },
36+
status: { name: 'status', type: 'text' as const },
37+
},
38+
};
39+
40+
function makeMemoryDriver() {
41+
const stores = new Map<string, Map<string, Record<string, unknown>>>();
42+
const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; };
43+
let nextId = 0;
44+
const matches = (row: Record<string, unknown>, where: any): boolean => {
45+
if (!where || typeof where !== 'object') return true;
46+
for (const [k, v] of Object.entries(where)) {
47+
if (k.startsWith('$')) continue;
48+
const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v;
49+
if ((row[k] ?? null) !== (exp ?? null)) return false;
50+
}
51+
return true;
52+
};
53+
const driver: any = {
54+
name: 'memory', version: '0.0.0', supports: {},
55+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
56+
async find(o: string, ast: any) {
57+
const rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where));
58+
return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows;
59+
},
60+
findStream() { throw new Error('ns'); },
61+
async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; },
62+
async create(o: string, data: Record<string, unknown>) {
63+
nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row;
64+
},
65+
async update(o: string, id: string, data: Record<string, unknown>) {
66+
const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`);
67+
const up = { ...cur, ...data, id }; s.set(id, up); return up;
68+
},
69+
async updateMany(o: string, ast: any, data: Record<string, unknown>) {
70+
let n = 0;
71+
for (const [id, row] of storeFor(o)) {
72+
if (!matches(row, ast?.where)) continue;
73+
storeFor(o).set(id, { ...row, ...data, id }); n += 1;
74+
}
75+
return n;
76+
},
77+
async delete(o: string, id: string) { return storeFor(o).delete(id); },
78+
async deleteMany(o: string, ast: any) {
79+
let n = 0;
80+
for (const [id, row] of [...storeFor(o)]) {
81+
if (!matches(row, ast?.where)) continue;
82+
storeFor(o).delete(id); n += 1;
83+
}
84+
return n;
85+
},
86+
async count(o: string, ast: any) { return (await this.find(o, ast)).length; },
87+
async bulkCreate(o: string, rows: Record<string, unknown>[]) { return Promise.all(rows.map((r) => this.create(o, r))); },
88+
async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {},
89+
};
90+
return { driver, stores };
91+
}
92+
93+
describe('filter → where folds on every engine method (#4346)', () => {
94+
let engine: ObjectQL;
95+
let stores: Map<string, Map<string, Record<string, unknown>>>;
96+
let a: any, b: any, c: any;
97+
98+
const titles = () => new Set(Array.from(stores.get('task')?.values() ?? []).map((r) => r.title));
99+
const ids = () => new Set(Array.from(stores.get('task')?.keys() ?? []));
100+
101+
beforeEach(async () => {
102+
engine = new ObjectQL();
103+
const mem = makeMemoryDriver();
104+
stores = mem.stores;
105+
engine.registerDriver(mem.driver, true);
106+
await engine.init();
107+
engine.registry.registerObject(task as any);
108+
// The issue's repro set: one open row, two done rows.
109+
a = await engine.insert('task', { title: 'A', status: 'open' });
110+
b = await engine.insert('task', { title: 'B', status: 'done' });
111+
c = await engine.insert('task', { title: 'C', status: 'done' });
112+
});
113+
114+
// ── {filter} alone applies the predicate ───────────────────────────
115+
116+
it('find({filter}) applies the predicate (the one method that already folded)', async () => {
117+
const rows = await engine.find('task', { filter: { status: 'done' } } as any);
118+
expect(new Set(rows.map((r: any) => r.id))).toEqual(new Set([b.id, c.id]));
119+
});
120+
121+
it('findOne({filter}) returns a MATCHING row, not the first row of the table', async () => {
122+
const row = await engine.findOne('task', { filter: { status: 'done' } } as any);
123+
expect(row.status).toBe('done');
124+
expect(row.id).not.toBe(a.id);
125+
});
126+
127+
it('count({filter}) counts the matching rows, not the whole table', async () => {
128+
expect(await engine.count('task', { filter: { status: 'done' } } as any)).toBe(2);
129+
});
130+
131+
it('update(data, {filter, multi}) rewrites ONLY the matching rows', async () => {
132+
await engine.update('task', { title: 'ZAP' }, { filter: { status: 'done' }, multi: true } as any);
133+
expect(stores.get('task')!.get(a.id)!.title).toBe('A');
134+
expect(stores.get('task')!.get(b.id)!.title).toBe('ZAP');
135+
expect(stores.get('task')!.get(c.id)!.title).toBe('ZAP');
136+
});
137+
138+
it('delete({filter, multi}) deletes ONLY the matching rows — the table survives', async () => {
139+
await engine.delete('task', { filter: { status: 'done' }, multi: true } as any);
140+
expect(ids()).toEqual(new Set([a.id]));
141+
});
142+
143+
it('aggregate({filter}) aggregates over the matching rows only', async () => {
144+
const out = await engine.aggregate('task', {
145+
filter: { status: 'done' },
146+
aggregations: [{ function: 'count', alias: 'n' }],
147+
} as any);
148+
expect(out).toEqual([{ n: 2 }]);
149+
});
150+
151+
it('a scalar id inside {filter} reaches the by-id fast path, same as {where}', async () => {
152+
await engine.update('task', { title: 'ONLY-B' }, { filter: { id: b.id } } as any);
153+
expect(stores.get('task')!.get(b.id)!.title).toBe('ONLY-B');
154+
expect(stores.get('task')!.get(c.id)!.title).toBe('C');
155+
156+
await engine.delete('task', { filter: { id: c.id } } as any);
157+
expect(ids()).toEqual(new Set([a.id, b.id]));
158+
});
159+
160+
// ── {filter, where} conflicting is refused; identical collapses ────
161+
162+
it.each([
163+
['find', () => engine.find('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)],
164+
['findOne', () => engine.findOne('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)],
165+
['count', () => engine.count('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)],
166+
['update', () => engine.update('task', { title: 'ZAP' }, { filter: { status: 'done' }, where: { status: 'open' }, multi: true } as any)],
167+
['delete', () => engine.delete('task', { filter: { status: 'done' }, where: { status: 'open' }, multi: true } as any)],
168+
['aggregate', () => engine.aggregate('task', { filter: { status: 'done' }, where: { status: 'open' }, aggregations: [{ function: 'count', alias: 'n' }] } as any)],
169+
])('%s refuses {filter, where} carrying different values', async (_method, call) => {
170+
await expect(call()).rejects.toThrow(/spellings of the same parameter.*Send exactly one/s);
171+
// No write happened before the refusal.
172+
expect(titles()).toEqual(new Set(['A', 'B', 'C']));
173+
});
174+
175+
it('redundant IDENTICAL {filter, where} collapses instead of conflicting', async () => {
176+
const rows = await engine.find('task', { filter: { status: 'done' }, where: { status: 'done' } } as any);
177+
expect(rows).toHaveLength(2);
178+
expect(await engine.count('task', { filter: { status: 'done' }, where: { status: 'done' } } as any)).toBe(2);
179+
});
180+
181+
it('an explicit null filter is a withdrawal, not a predicate and not a conflict', async () => {
182+
const rows = await engine.find('task', { filter: null } as any);
183+
expect(rows).toHaveLength(3);
184+
});
185+
186+
// ── the sixth pair: top → limit (canonical wins, conflicts refused) ─
187+
188+
it('find({top}) alone still limits', async () => {
189+
const rows = await engine.find('task', { top: 1 } as any);
190+
expect(rows).toHaveLength(1);
191+
});
192+
193+
it('find({top, limit}) conflicting is refused — HTTP and in-process now agree', async () => {
194+
await expect(engine.find('task', { top: 1, limit: 3 } as any))
195+
.rejects.toThrow(/spellings of the same parameter \(canonical 'limit'\)/);
196+
});
197+
198+
it('find({top, limit}) identical collapses', async () => {
199+
const rows = await engine.find('task', { top: 2, limit: 2 } as any);
200+
expect(rows).toHaveLength(2);
201+
});
202+
203+
it('findOne({top}) stays single-row — the forced limit: 1 wins over the folded alias', async () => {
204+
const row = await engine.findOne('task', { top: 5, filter: { status: 'done' } } as any);
205+
expect(row.status).toBe('done');
206+
});
207+
208+
// ── the documented hook call path (ScopedContext / ObjectRepository) ─
209+
210+
it('the hook-docs call `ctx.object(...).findOne({ where })` and its legacy {filter} spelling agree', async () => {
211+
const ctx = engine.createContext({ userId: 'usr_1' });
212+
const repo = ctx.object('task');
213+
const viaWhere = await repo.findOne({ where: { status: 'done' } });
214+
const viaFilter = await repo.findOne({ filter: { status: 'done' } });
215+
expect(viaFilter.status).toBe('done');
216+
expect(viaWhere.status).toBe('done');
217+
});
218+
219+
it('a cleanup hook calling repo.delete({filter, multi}) no longer empties the object', async () => {
220+
const repo = engine.createContext({ userId: 'usr_1' }).object('task');
221+
await repo.delete({ filter: { status: 'done' }, multi: true });
222+
expect(ids()).toEqual(new Set([a.id]));
223+
});
224+
});

0 commit comments

Comments
 (0)