diff --git a/.changeset/engine-filter-fold-every-method.md b/.changeset/engine-filter-fold-every-method.md new file mode 100644 index 0000000000..c8d10ab25c --- /dev/null +++ b/.changeset/engine-filter-fold-every-method.md @@ -0,0 +1,52 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": patch +"@objectstack/metadata-protocol": patch +--- + +fix(objectql,spec): `filter` folds to `where` on EVERY engine method, and `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. `ObjectQL.find()` +folded it; `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: + +| call | before | after | +|---|---|---| +| `findOne({filter: {status:'done'}})` | first row of the table | a matching row | +| `count({filter})` | whole-table count | matching count | +| `update(data, {filter, multi:true})` | **every row rewritten** | matching rows | +| `delete({filter, multi:true})` | **table emptied** | matching rows | +| `aggregate({filter, …})` | aggregated all rows | matching rows | + +This was reachable, not theoretical: the deprecated +`DataEngine{Query,Update,Delete,Count,Aggregate}OptionsSchema` contracts all +declare `filter`, `ScopedContext`/`ObjectRepository` (the cross-object API +handed to L2 hook bodies) forwards its argument verbatim, and the spec's own +hook documentation taught the broken call +(`users.findOne({ filter: { role: 'admin' } })` — now corrected to `where`). + +Every engine entry point now folds through the spec's own #3795 machinery +(`RPC_QUERY_ALIAS_SLOTS` + `foldQueryAliasSlots`) instead of `find`'s +hand-rolled copy, under the #4181 rule: an alias alone folds, redundant +identical spellings collapse, DIFFERENT values for one slot throw +("Send exactly one") instead of silently picking a winner, and an explicit +`null` alias is a withdrawal. + +**The sixth pair.** `top` → `limit` — the pair the #3795 scope note excluded +as "the OData layer" — joins `RPC_QUERY_ALIAS_SLOTS`. The protocol normalizer +folded it BACKWARDS (`options.limit = Number(options.top)` — the alias +overwrote the canonical key) while `engine.find` folded it canonical-wins, so +`{top: 1, limit: 3}` answered 1 over HTTP and 3 through a direct engine call. +All three readers (wire normalizer, RPC schema parse, engine) now resolve the +pair identically: `top` alone still limits, a conflicting `{top, limit}` is +refused. + +Behavior change to note: option bags that previously smuggled conflicting +spellings (`{where: X, filter: Y}`, `{top: 1, limit: 3}`) are now refused +loudly on every path instead of silently resolving differently per layer. +Pinned per method, write paths included — a regression here is silent and +destructive, and the class went unnoticed precisely because `find` was the +only method anyone thought to check. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index a368fcee6e..145d907e72 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3676,15 +3676,16 @@ export class ObjectStackProtocolImplementation implements delete options[dollar]; } - // [#3795] One slot, one value. Every alias spelling of the five + // [#3795] One slot, one value. Every alias spelling of the six // QueryAST slots resolves HERE, by the spec's own table, before the // per-slot wire coercion below ever runs — so that coercion reads // canonical keys only. An alias alone folds into its canonical key; // redundant identical spellings collapse; different values for one // slot are refused (the #4181 rule, generalized from the filter slot - // to all five — four of which used to resolve BACKWARDS here, each in - // its own way, disagreeing with the spec's documented precedence and - // with the runtime dispatcher's copy of the same fold). + // to all of them — four used to resolve BACKWARDS here, each in its + // own way, disagreeing with the spec's documented precedence and with + // the runtime dispatcher's copy of the same fold; `top`→`limit` + // joined the table last, via #4346). // // `arrivedAs` remembers which spelling carried each slot's value; // composed with `wireSpelling` it names the parameter the caller @@ -3695,12 +3696,12 @@ export class ObjectStackProtocolImplementation implements }); const slotParam = (canonical: string): string => spellingFor(arrivedAs[canonical] ?? canonical); - // Numeric fields — normalize top → limit ($top is the OData layer, - // outside the #3795 slot table), then coerce querystring strings. - if (options.top != null) { - options.limit = Number(options.top); - delete options.top; - } + // Numeric fields — coerce querystring strings. `top` already folded + // into `limit` above: the pair joined the #3795 slot table with #4346, + // replacing an open-coded rewrite here that resolved it BACKWARDS + // (`options.limit = Number(options.top)` — the alias overwrote the + // canonical key, so `{top: 1, limit: 3}` answered 1 over HTTP and 3 + // through a direct engine call). if (options.limit != null) options.limit = Number(options.limit); if (options.offset != null) options.offset = Number(options.offset); diff --git a/packages/objectql/src/engine-filter-alias.test.ts b/packages/objectql/src/engine-filter-alias.test.ts new file mode 100644 index 0000000000..97f45f6715 --- /dev/null +++ b/packages/objectql/src/engine-filter-alias.test.ts @@ -0,0 +1,224 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4346 — the `filter` → `where` alias folds on EVERY engine entry point, not + * just `find()`, with a REAL {@link ObjectQL} engine + in-memory driver. + * + * Regression: the DataEngine contract (`DataEngine*OptionsSchema`) declares + * `filter` as a legitimate option on every read and write method, but only + * `find()` folded it into the `where` key the driver AST understands. + * `findOne`/`count`/`update`/`delete`/`aggregate` passed the 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`. The spec's own hook + * documentation taught exactly this call (`users.findOne({ filter: … })`). + * + * The fold now runs once per entry point via the spec's #3795 slot table + * (`RPC_QUERY_ALIAS_SLOTS` + `foldQueryAliasSlots`), under the #4181 rule: + * alias alone folds, identical duplicates collapse, conflicting values are + * refused. Same table also folds `top` → `limit` (canonical wins — the pair + * the #3795 sweep scoped out). + * + * The write-path pins matter most: a regression there is silent and + * destructive, and this whole class went unnoticed because `find` was the only + * method anyone thought to check. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const task = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', type: 'text' as const }, + status: { name: 'status', type: 'text' as const }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const exp = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (exp ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(o: string, ast: any) { + const rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows; + }, + findStream() { throw new Error('ns'); }, + async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; }, + async create(o: string, data: Record) { + nextId += 1; const id = (data.id as string) ?? `r_${nextId}`; const row = { ...data, id }; storeFor(o).set(id, row); return row; + }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); const cur = s.get(id); if (!cur) throw new Error(`nf ${o}/${id}`); + const up = { ...cur, ...data, id }; s.set(id, up); return up; + }, + async updateMany(o: string, ast: any, data: Record) { + 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; + }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + 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; + }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: Record[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +describe('filter → where folds on every engine method (#4346)', () => { + let engine: ObjectQL; + let stores: Map>>; + let a: any, b: any, c: any; + + const titles = () => new Set(Array.from(stores.get('task')?.values() ?? []).map((r) => r.title)); + const ids = () => new Set(Array.from(stores.get('task')?.keys() ?? [])); + + beforeEach(async () => { + engine = new ObjectQL(); + const mem = makeMemoryDriver(); + stores = mem.stores; + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(task as any); + // The issue's repro set: one open row, two done rows. + a = await engine.insert('task', { title: 'A', status: 'open' }); + b = await engine.insert('task', { title: 'B', status: 'done' }); + c = await engine.insert('task', { title: 'C', status: 'done' }); + }); + + // ── {filter} alone applies the predicate ─────────────────────────── + + it('find({filter}) applies the predicate (the one method that already folded)', async () => { + const rows = await engine.find('task', { filter: { status: 'done' } } as any); + expect(new Set(rows.map((r: any) => r.id))).toEqual(new Set([b.id, c.id])); + }); + + it('findOne({filter}) returns a MATCHING row, not the first row of the table', async () => { + const row = await engine.findOne('task', { filter: { status: 'done' } } as any); + expect(row.status).toBe('done'); + expect(row.id).not.toBe(a.id); + }); + + it('count({filter}) counts the matching rows, not the whole table', async () => { + expect(await engine.count('task', { filter: { status: 'done' } } as any)).toBe(2); + }); + + it('update(data, {filter, multi}) rewrites ONLY the matching rows', async () => { + await engine.update('task', { title: 'ZAP' }, { filter: { status: 'done' }, multi: true } as any); + expect(stores.get('task')!.get(a.id)!.title).toBe('A'); + expect(stores.get('task')!.get(b.id)!.title).toBe('ZAP'); + expect(stores.get('task')!.get(c.id)!.title).toBe('ZAP'); + }); + + it('delete({filter, multi}) deletes ONLY the matching rows — the table survives', async () => { + await engine.delete('task', { filter: { status: 'done' }, multi: true } as any); + expect(ids()).toEqual(new Set([a.id])); + }); + + it('aggregate({filter}) aggregates over the matching rows only', async () => { + const out = await engine.aggregate('task', { + filter: { status: 'done' }, + aggregations: [{ function: 'count', alias: 'n' }], + } as any); + expect(out).toEqual([{ n: 2 }]); + }); + + it('a scalar id inside {filter} reaches the by-id fast path, same as {where}', async () => { + await engine.update('task', { title: 'ONLY-B' }, { filter: { id: b.id } } as any); + expect(stores.get('task')!.get(b.id)!.title).toBe('ONLY-B'); + expect(stores.get('task')!.get(c.id)!.title).toBe('C'); + + await engine.delete('task', { filter: { id: c.id } } as any); + expect(ids()).toEqual(new Set([a.id, b.id])); + }); + + // ── {filter, where} conflicting is refused; identical collapses ──── + + it.each([ + ['find', () => engine.find('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)], + ['findOne', () => engine.findOne('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)], + ['count', () => engine.count('task', { filter: { status: 'done' }, where: { status: 'open' } } as any)], + ['update', () => engine.update('task', { title: 'ZAP' }, { filter: { status: 'done' }, where: { status: 'open' }, multi: true } as any)], + ['delete', () => engine.delete('task', { filter: { status: 'done' }, where: { status: 'open' }, multi: true } as any)], + ['aggregate', () => engine.aggregate('task', { filter: { status: 'done' }, where: { status: 'open' }, aggregations: [{ function: 'count', alias: 'n' }] } as any)], + ])('%s refuses {filter, where} carrying different values', async (_method, call) => { + await expect(call()).rejects.toThrow(/spellings of the same parameter.*Send exactly one/s); + // No write happened before the refusal. + expect(titles()).toEqual(new Set(['A', 'B', 'C'])); + }); + + it('redundant IDENTICAL {filter, where} collapses instead of conflicting', async () => { + const rows = await engine.find('task', { filter: { status: 'done' }, where: { status: 'done' } } as any); + expect(rows).toHaveLength(2); + expect(await engine.count('task', { filter: { status: 'done' }, where: { status: 'done' } } as any)).toBe(2); + }); + + it('an explicit null filter is a withdrawal, not a predicate and not a conflict', async () => { + const rows = await engine.find('task', { filter: null } as any); + expect(rows).toHaveLength(3); + }); + + // ── the sixth pair: top → limit (canonical wins, conflicts refused) ─ + + it('find({top}) alone still limits', async () => { + const rows = await engine.find('task', { top: 1 } as any); + expect(rows).toHaveLength(1); + }); + + it('find({top, limit}) conflicting is refused — HTTP and in-process now agree', async () => { + await expect(engine.find('task', { top: 1, limit: 3 } as any)) + .rejects.toThrow(/spellings of the same parameter \(canonical 'limit'\)/); + }); + + it('find({top, limit}) identical collapses', async () => { + const rows = await engine.find('task', { top: 2, limit: 2 } as any); + expect(rows).toHaveLength(2); + }); + + it('findOne({top}) stays single-row — the forced limit: 1 wins over the folded alias', async () => { + const row = await engine.findOne('task', { top: 5, filter: { status: 'done' } } as any); + expect(row.status).toBe('done'); + }); + + // ── the documented hook call path (ScopedContext / ObjectRepository) ─ + + it('the hook-docs call `ctx.object(...).findOne({ where })` and its legacy {filter} spelling agree', async () => { + const ctx = engine.createContext({ userId: 'usr_1' }); + const repo = ctx.object('task'); + const viaWhere = await repo.findOne({ where: { status: 'done' } }); + const viaFilter = await repo.findOne({ filter: { status: 'done' } }); + expect(viaFilter.status).toBe('done'); + expect(viaWhere.status).toBe('done'); + }); + + it('a cleanup hook calling repo.delete({filter, multi}) no longer empties the object', async () => { + const repo = engine.createContext({ userId: 'usr_1' }).object('task'); + await repo.delete({ filter: { status: 'done' }, multi: true }); + expect(ids()).toEqual(new Set([a.id])); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index df867f2174..93bc39ff4b 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9,6 +9,9 @@ import { EngineDeleteOptions, EngineAggregateOptions, EngineCountOptions, + RPC_QUERY_ALIAS_SLOTS, + foldQueryAliasSlots, + type QueryAliasSlot, type DroppedFieldsEvent } from '@objectstack/spec/data'; import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; @@ -90,6 +93,61 @@ const DISPATCHABLE_HOOK_EVENTS: ReadonlySet = new Set([ 'beforeDelete', 'afterDelete', ]); +/** + * [#4346] The alias slots the ENGINE option bags still admit, cut from the + * spec's own table (#3795) so the engine never re-declares a mapping. + * + * The deprecated `DataEngine{Query,Update,Delete,Count,Aggregate}Options` + * contracts declare `filter` on every read AND write method, but only `find` + * folded it — `findOne`/`count`/`update`/`delete`/`aggregate` passed the bag + * through with `where === undefined`, which every driver reads as "no + * predicate": a caller filtering with `{ filter }` silently matched EVERY row + * (an over-grant on the reads, an unbounded write on `update`/`delete`). + * + * `where` is the slot every method folds; `limit` additionally applies to the + * find-shaped bags, which declare `top` (OData) as its alias. The other four + * pairs in the table are RPC/wire spellings folded at parse by + * `RpcQueryOptionsSchema` / the protocol normalizer — their values need shape + * lowering (`sort` records, `populate` lists) that belongs to those layers, so + * the engine deliberately does not fold them. + */ +const ENGINE_WHERE_SLOTS: readonly QueryAliasSlot[] = + RPC_QUERY_ALIAS_SLOTS.filter((slot) => slot.canonical === 'where'); +const ENGINE_QUERY_SLOTS: readonly QueryAliasSlot[] = + RPC_QUERY_ALIAS_SLOTS.filter((slot) => slot.canonical === 'where' || slot.canonical === 'limit'); + +/** + * Fold the deprecated alias spellings of an engine option bag into their + * canonical QueryAST keys, under the #3795/#4181 rule: an alias alone moves to + * the canonical key, redundant identical spellings collapse, DIFFERENT values + * for one slot are irreconcilable and throw (picking a winner IS the silent + * drop), and an explicit `null` alias is a withdrawal. + * + * Returns the SAME reference when no alias spelling is present (the common + * path allocates nothing — `withResolvedWhere` discipline); otherwise folds a + * shallow copy, because the bag belongs to the caller and may be reused (view + * metadata, flow node config). + */ +function foldEngineOptionAliases( + object: string, + operation: string, + bag: T, + slots: readonly QueryAliasSlot[], +): T { + if (!bag) return bag; + if (!slots.some((slot) => slot.aliases.some((alias) => alias in bag))) return bag; + const folded: Record = { ...bag }; + foldQueryAliasSlots(folded, slots, (conflict) => { + throw new Error( + `Conflicting options on ${operation}('${object}'): ` + + `${conflict.spellings.map((s) => `'${s}'`).join(', ')} are spellings of the same ` + + `parameter (canonical '${conflict.canonical}') and were given different values. ` + + 'Send exactly one.', + ); + }); + return folded as T; +} + interface FormulaPlanEntry { name: string; expression: Expression; } function planFormulaProjection( @@ -2857,25 +2915,17 @@ export class ObjectQL implements IDataEngine { async find(object: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); + // Normalize the alias spellings (`filter`→`where`, `top`→`limit`) by the + // spec's slot table — the driver AST only understands the canonical keys, + // so an unfolded `{ filter }` would match ALL rows (silent over-grant — + // surfaced by ADR-0057's sharing/graph read path). Same fold, same + // conflict rule on every engine entry point (#4346). + query = foldEngineOptionAliases(object, 'find', query, ENGINE_QUERY_SLOTS); this.logger.debug('Find operation starting', { object, query }); const driver = this.getDriver(object); const ast: QueryAST = { object, ...query }; // Remove context from the AST — it's not a driver concern delete (ast as any).context; - // Normalize the `filter` alias → `where`. The DataEngine contract - // (`spec/data/data-engine.zod.ts`) exposes both, but the driver AST only - // understands `where`; an internal caller passing `{ filter }` would - // otherwise match ALL rows (silent over-grant — surfaced by ADR-0057's - // sharing/graph read path). `where` wins when both are present. - if ((ast as any).filter != null && ast.where == null) { - ast.where = (ast as any).filter; - } - delete (ast as any).filter; - // Normalize OData `top` alias → standard `limit` - if ((ast as any).top != null && ast.limit == null) { - ast.limit = (ast as any).top; - } - delete (ast as any).top; // Plan formula projection: rewrite ast.fields to drop virtual formula // names and inject their dependencies, so the driver returns the raw @@ -3002,12 +3052,16 @@ export class ObjectQL implements IDataEngine { async findOne(objectName: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { objectName = this.resolveObjectName(objectName); + // Same alias fold as find() (#4346). Without it, `findOne({ filter })` + // matched the first row of the WHOLE table rather than the predicate. + // `top` folds into `limit` here too, but findOne is single-row by + // contract, so the literal `limit: 1` below wins over both spellings. + query = foldEngineOptionAliases(objectName, 'findOne', query, ENGINE_QUERY_SLOTS); this.logger.debug('FindOne operation', { objectName }); const driver = this.getDriver(objectName); const ast: QueryAST = { object: objectName, ...query, limit: 1 }; - // Remove context and top alias from the AST + // Remove context from the AST — it's not a driver concern delete (ast as any).context; - delete (ast as any).top; // Plan formula projection (same as find): rewrite ast.fields so the driver // returns the raw dependency fields, then evaluate formulas after fetch. @@ -3371,6 +3425,12 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'update'); const driver = this.getDriver(object); + // Fold the `filter` alias into `where` FIRST (#4346): everything below — + // token resolution, the by-id fast path, the #2982 AST seeding — reads + // `options.where` only, so an unfolded `{ filter }` left the AST with no + // predicate at all and a `multi: true` update rewrote EVERY row. + options = foldEngineOptionAliases(object, 'update', options, ENGINE_WHERE_SLOTS); + // Expand `{filter-placeholder}` values BEFORE the id is extracted (#3810). // The read path resolves them; without the same call here the SAME filter // selected different rows depending on the verb — `find({owner: @@ -3764,6 +3824,11 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'delete'); const driver = this.getDriver(object); + // Fold the `filter` alias into `where` first — same reasoning as update() + // above (#4346): unfolded, a `multi: true` delete with `{ filter }` had no + // predicate on its AST and emptied the table. + options = foldEngineOptionAliases(object, 'delete', options, ENGINE_WHERE_SLOTS); + // Expand `{filter-placeholder}` values before the id is extracted — same // reasoning as update() above (#3810). options = this.withResolvedWhere(options); @@ -3892,6 +3957,9 @@ export class ObjectQL implements IDataEngine { async count(object: string, query?: EngineCountOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); + // Fold the `filter` alias into `where` (#4346) — the AST below reads + // `query.where` only, so an unfolded `{ filter }` counted the whole table. + query = foldEngineOptionAliases(object, 'count', query, ENGINE_WHERE_SLOTS); const driver = this.getDriver(object); // The AST must ride on the opCtx so the security/sharing middlewares can @@ -3969,6 +4037,9 @@ export class ObjectQL implements IDataEngine { async aggregate(object: string, query: EngineAggregateOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); + // Fold the `filter` alias into `where` (#4346) — the AST below reads + // `query.where` only, so an unfolded `{ filter }` aggregated every row. + query = foldEngineOptionAliases(object, 'aggregate', query, ENGINE_WHERE_SLOTS); this.rejectCredentialAggregation(object, query); const driver = this.getDriver(object); this.logger.debug(`Aggregate on ${object} using ${driver.name}`, query); @@ -4412,7 +4483,7 @@ export class ObjectQL implements IDataEngine { * Usage: * const ctx = engine.createContext({ userId: '...', tenantId: '...' }); * const users = ctx.object('user'); - * await users.find({ filter: { status: 'active' } }); + * await users.find({ where: { status: 'active' } }); */ createContext(ctx: Partial): ScopedContext { return new ScopedContext( diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index e82357ccf4..1f7d55958e 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -1175,7 +1175,7 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { }); // ═══════════════════════════════════════════════════════════════ - // #3795 — one slot, one value, for ALL five alias pairs + // #3795 — one slot, one value, for ALL the alias pairs // // The spec documented `where` > `filter`, `fields` > `select`, // `orderBy` > `sort`, `offset` > `skip`, `expand` > `populate` — in prose @@ -1184,10 +1184,12 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { // the opposite precedence on three of them, so one request resolved // differently per path. The fold now lives once, in the spec's own table // (`RPC_QUERY_ALIAS_SLOTS`), under #4181's rule: alias alone folds, - // identical duplicates collapse, different values are refused. + // identical duplicates collapse, different values are refused. `limit` > + // `top` — the pair #3795 scoped out as "the OData layer" — joined the + // table with #4346. // ═══════════════════════════════════════════════════════════════ - describe('alias precedence resolves by the spec table, all five slots (#3795)', () => { + describe('alias precedence resolves by the spec table, all six slots (#3795, #4346)', () => { function makeProtocol() { const engine: any = { find: vi.fn().mockResolvedValue([]), @@ -1217,6 +1219,12 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { ['sort', 'orderBy', 'title', [{ field: 'status', order: 'asc' }], [{ field: 'title', order: 'asc' }]], ['populate', 'expand', ['assignee'], { project: { object: 'project' } }, { assignee: { object: 'assignee' } }], ['filter', 'where', { status: 'done' }, { status: 'open' }, { status: 'done' }], + // [#4346] The sixth pair — the one the #3795 scope note excluded. + // This normalizer used to fold it BACKWARDS (`options.limit = + // Number(options.top)` — alias overwrote canonical) while the + // engine folded it canonical-wins, so `{top: 1, limit: 3}` + // answered 1 over HTTP and 3 through a direct engine call. + ['top', 'limit', 5, 3, 5], ] as const; it.each(SLOTS)( diff --git a/packages/spec/src/data/data-engine.test.ts b/packages/spec/src/data/data-engine.test.ts index 14003c03e8..2320c76093 100644 --- a/packages/spec/src/data/data-engine.test.ts +++ b/packages/spec/src/data/data-engine.test.ts @@ -463,7 +463,7 @@ describe('DataEngineFindRequestSchema', () => { expect(request.query?.where).toBeDefined(); }); - it('folds legacy params into their canonical keys and drops the aliases (#3795)', () => { + it('folds legacy params into their canonical keys and drops the aliases (#3795, #4346)', () => { const request = DataEngineFindRequestSchema.parse({ method: 'find', object: 'account', @@ -473,6 +473,7 @@ describe('DataEngineFindRequestSchema', () => { sort: [{ field: 'name', order: 'asc' }], skip: 10, populate: ['owner'], + top: 5, }, }); @@ -483,7 +484,8 @@ describe('DataEngineFindRequestSchema', () => { expect(request.query?.orderBy).toEqual([{ field: 'name', order: 'asc' }]); expect(request.query?.offset).toBe(10); expect(request.query?.expand).toEqual({ owner: { object: 'owner' } }); - for (const alias of ['filter', 'select', 'sort', 'skip', 'populate']) { + expect(request.query?.limit).toBe(5); + for (const alias of ['filter', 'select', 'sort', 'skip', 'populate', 'top']) { expect(request.query && alias in request.query).toBe(false); } }); @@ -527,13 +529,14 @@ describe('DataEngineFindRequestSchema', () => { } }); - it('refuses a conflicting value on every alias pair (#3795)', () => { + it('refuses a conflicting value on every alias pair (#3795, #4346)', () => { const cases: Array> = [ { filter: { a: 1 }, where: { b: 2 } }, { select: ['a'], fields: ['b'] }, { sort: [{ field: 'a', order: 'asc' }], orderBy: [{ field: 'b', order: 'asc' }] }, { skip: 1, offset: 2 }, { populate: ['a'], expand: { b: { object: 'b' } } }, + { top: 1, limit: 3 }, ]; for (const query of cases) { const result = DataEngineFindRequestSchema.safeParse({ method: 'find', object: 'account', query }); diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index f3d1c4c453..c4dcfc569b 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -406,14 +406,21 @@ export interface QueryAliasConflict { } /** - * The five alias pairs the RPC query surface accepts (#3795) — the ONE place - * the alias → canonical mapping is declared. The schema transform below folds - * parsed input by this table, and the protocol normalizer + * The six alias pairs the RPC query surface accepts (#3795, #4346) — the ONE + * place the alias → canonical mapping is declared. The schema transform below + * folds parsed input by this table, the protocol normalizer * (`metadata-protocol`) folds raw wire input by the same table, extended with * the wire-only spellings `filters` / `$filter` / `$expand` that no schema - * declares. Before this table existed the precedence lived in prose only, so - * every reader re-implemented it — the #3713 condition — and the two readers - * disagreed on three of the five pairs, four of them backwards. + * declares, and the ObjectQL engine folds the slots its own option bags still + * admit (`where`, `limit`) on every entry point. Before this table existed the + * precedence lived in prose only, so every reader re-implemented it — the + * #3713 condition — and the two readers disagreed on three of the five pairs, + * four of them backwards. + * + * `limit`/`top` joined last (#4346): the #3795 sweep scoped it out as "the + * OData layer", leaving the protocol folding it BACKWARDS (`top` overwrote + * `limit`) while the engine folded it canonical-wins — `{top: 1, limit: 3}` + * answered 1 over HTTP and 3 in-process. */ export const RPC_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = [ { canonical: 'where', aliases: ['filter'] }, @@ -421,6 +428,7 @@ export const RPC_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = [ { canonical: 'orderBy', aliases: ['sort'] }, { canonical: 'offset', aliases: ['skip'] }, { canonical: 'expand', aliases: ['populate'] }, + { canonical: 'limit', aliases: ['top'] }, ]; /** @@ -555,7 +563,7 @@ function foldRpcQueryOptions(input: object, ctx: z.core.$RefinementCtx): EngineQ * * **One slot, one value (#3795):** each legacy alias is folded into its * canonical key at parse — `filter`→`where`, `select`→`fields`, - * `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand` + * `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand`, `top`→`limit` * ({@link RPC_QUERY_ALIAS_SLOTS}) — and the alias is dropped from the parsed * output, so consumers only ever read canonical QueryAST keys. Sending both * spellings with the SAME value is redundant and tolerated; sending DIFFERENT diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 30b0a56c8c..cfcee9759a 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -400,7 +400,7 @@ export const HookContextSchema = lazySchema(() => z.object({ * * Usage in hooks: * const users = ctx.api.object('user'); - * const admin = await users.findOne({ filter: { role: 'admin' } }); + * const admin = await users.findOne({ where: { role: 'admin' } }); */ api: z.unknown().optional().describe('Cross-object data access (ScopedContext)'),