From f972945fe41043f2fd8c18ef8b6154a92decae96 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:32:19 +0800 Subject: [PATCH] =?UTF-8?q?fix(objectql):=20every=20engine=20verb=20folds?= =?UTF-8?q?=20`filter`=20=E2=86=92=20`where`=20=E2=80=94=20`delete({filter?= =?UTF-8?q?})`=20no=20longer=20empties=20the=20object=20(#4346)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The driver AST understands `where` only, so an unfolded `filter` is not a narrower query — it is no query. `find()` folded it (with a comment saying exactly that, from the ADR-0057 over-grant); `findOne`/`count`/`aggregate`/ `update`/`delete` never did. Measured: findOne returned a non-matching row, count counted the whole object, and the write verbs reached updateMany / deleteMany with no predicate — rewriting every row, emptying the object. Reachable from the documented surface, not just internals: ScopedContext (`ctx.api.object('x')` in an L2 hook) forwards its bag verbatim with every param typed `any`, and the spec's own hook TSDoc taught `findOne({ filter: ... })` — corrected here. Each entry point now makes one call to the shared #3795 fold (foldQueryAliasSlots + ENGINE_FILTER_ALIAS_SLOTS) instead of five more copies of find's guard, and copies the caller's bag rather than mutating it. Same pass, the sixth alias pair: `top` is declared as the alias of `limit`, but protocol.ts overwrote `limit` with it unguarded while the engine kept `limit` — one query, two answers by path. Both read ENGINE_QUERY_ALIAS_SLOTS now; `top` stays a declared AST key. Co-Authored-By: Claude Fable 5 --- .changeset/engine-filter-alias-every-verb.md | 59 +++++ packages/metadata-protocol/src/protocol.ts | 21 +- .../src/engine-option-alias-fold.test.ts | 240 ++++++++++++++++++ packages/objectql/src/engine.ts | 98 +++++-- packages/objectql/src/protocol-data.test.ts | 21 ++ packages/spec/api-surface.json | 2 + packages/spec/src/data/data-engine.zod.ts | 41 ++- packages/spec/src/data/hook.zod.ts | 2 +- 8 files changed, 454 insertions(+), 30 deletions(-) create mode 100644 .changeset/engine-filter-alias-every-verb.md create mode 100644 packages/objectql/src/engine-option-alias-fold.test.ts diff --git a/.changeset/engine-filter-alias-every-verb.md b/.changeset/engine-filter-alias-every-verb.md new file mode 100644 index 0000000000..10065379c4 --- /dev/null +++ b/.changeset/engine-filter-alias-every-verb.md @@ -0,0 +1,59 @@ +--- +"@objectstack/objectql": patch +"@objectstack/spec": minor +"@objectstack/metadata-protocol": patch +--- + +fix(objectql): every engine verb folds `filter` → `where` — `delete({filter})` no longer empties the object (#4346) + +`ObjectQL.find()` folded the deprecated `filter` alias onto `where`, with a +comment explaining why it must: the driver AST understands `where` only, so an +unfolded `filter` is not a narrower query, it is **no query**. The five sibling +verbs never got that fold. + +Measured against a real engine + driver, three rows (`a: open`, `b: done`, +`c: done`), predicate passed as `filter`: + +| call | before | after | +|---|---|---| +| `find({filter: done})` | `[b, c]` ✅ | unchanged | +| `findOne({filter: done})` | **`a`** — a row that does not match | `b` | +| `count({filter: done})` | **3** | 2 | +| `aggregate({filter: done, …})` | whole object | matched rows | +| `update(data, {filter: done, multi})` | **all three rows rewritten** | only `b`, `c` | +| `delete({filter: done, multi})` | **object emptied** | only `b`, `c` deleted | + +The write paths reached `driver.updateMany` / `deleteMany` with +`ast.where === undefined`, which every driver reads as "no predicate". + +**Reachable from the documented authoring surface.** `ScopedContext` — the +cross-object API handed to L2 hook bodies as `ctx.api.object('x')` — forwards +its argument bag verbatim to these methods, every parameter typed `any`, and +the spec's own hook TSDoc taught `users.findOne({ filter: { role: 'admin' } })`. +That example is corrected to `where` here. The deprecated +`DataEngine{Query,Update,Delete,Count}OptionsSchema` also still declare `filter` +for exactly these verbs, so "callers should not pass it" was never the contract. + +The fold is now one call per entry point to the shared `foldQueryAliasSlots` + +`ENGINE_FILTER_ALIAS_SLOTS` table from `@objectstack/spec/data` (#3795's +machinery), rather than five more copies of `find`'s guard. Options bags are +copied, never mutated under the caller. + +**Same pass, the sixth alias pair.** `top` is declared as "Alias for limit +(OData compatibility)" on both `QuerySchema` and `EngineQueryOptionsSchema`, yet +`protocol.ts` folded it **unguarded** (`options.limit = Number(options.top)` — +the alias overwriting the canonical key) while `engine.find` kept `limit`. So +`{top: 1, limit: 3}` resolved to 1 over HTTP and 3 through a direct engine call +— #3795's divergence on the one pair its scope note excluded. Both now read +`ENGINE_QUERY_ALIAS_SLOTS`. `top` stays a declared AST key (unlike the five +deprecated RPC aliases it is **not** dropped from parsed output — that would be +a breaking type change deserving its own decision); it simply has one +precedence now. + +**Behaviour changes**, all in the direction of one answer: a predicate passed as +`filter` now actually applies on every verb (if you were relying on +`delete({filter})` deleting everything, it will now delete what you asked for), +and two spellings of one slot carrying **different** values are refused +(`400 INVALID_REQUEST`) rather than silently resolved — the #4181 rule, applied +at the engine layer too. A single spelling, and redundant identical spellings, +behave exactly as before. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index a368fcee6e..b93f533d5a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -18,7 +18,7 @@ import type { } from '@objectstack/spec/api'; import type { MetadataCacheRequest, MetadataCacheResponse, ServiceInfo, ApiRoutes, WellKnownCapabilities } from '@objectstack/spec/api'; import { readServiceSelfInfo } from '@objectstack/spec/api'; -import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, REFERENCE_VALUE_TYPES, RPC_QUERY_ALIAS_SLOTS, foldQueryAliasSlots, type QueryAliasConflict, type QueryAliasSlot, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; +import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, REFERENCE_VALUE_TYPES, RPC_QUERY_ALIAS_SLOTS, ENGINE_QUERY_ALIAS_SLOTS, foldQueryAliasSlots, type QueryAliasConflict, type QueryAliasSlot, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared'; import { applyConversionsToStoredItem } from '@objectstack/spec'; import { type FormView, isAggregatedViewContainer } from '@objectstack/spec/ui'; @@ -923,7 +923,14 @@ const WIRE_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = (() => { where: ['filters', '$filter'], expand: ['$expand'], }; - return RPC_QUERY_ALIAS_SLOTS.map((slot) => ({ + // [#4346] `limit`/`top` rides in from ENGINE_QUERY_ALIAS_SLOTS rather than + // being spelled again here: this layer folded it in the direction OPPOSITE + // to the engine (`options.limit = Number(options.top)`, no guard — the + // alias overwriting the canonical key), so `{top: 1, limit: 3}` answered 1 + // over HTTP and 3 through a direct engine call. + return ENGINE_QUERY_ALIAS_SLOTS.concat( + RPC_QUERY_ALIAS_SLOTS.filter((slot) => !ENGINE_QUERY_ALIAS_SLOTS.some((e) => e.canonical === slot.canonical)), + ).map((slot) => ({ canonical: slot.canonical, aliases: [...slot.aliases, ...(extra[slot.canonical] ?? [])], })); @@ -3695,12 +3702,10 @@ 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 coercion — a querystring carries every number as a string. + // `top` → `limit` folded above with the rest (#4346); what is left here + // is the coercion the fold deliberately does not do (it moves values + // verbatim). 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-option-alias-fold.test.ts b/packages/objectql/src/engine-option-alias-fold.test.ts new file mode 100644 index 0000000000..8e09fe8b22 --- /dev/null +++ b/packages/objectql/src/engine-option-alias-fold.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4346 — the `filter` → `where` fold existed in `find()` ONLY. + * + * The driver AST understands `where`; it has no idea what `filter` is. So an + * unfolded `filter` is not a narrower query, it is NO query — and the five + * sibling methods that never got `find()`'s fold each turned a caller's + * predicate into "every row": + * + * | call (predicate passed as `filter`) | before | + * |---|---| + * | `findOne` | returned a row that does not match | + * | `count` | counted the whole object | + * | `update` | **rewrote every row** | + * | `delete` | **emptied the object** | + * + * Reachable from the documented authoring surface, not just from internals: + * `ScopedContext` (`ctx.api.object('x')` in an L2 hook) forwards its argument + * bag verbatim, and the spec's own hook TSDoc taught + * `findOne({ filter: … })` — corrected in the same change. + * + * The sixth alias pair rides along: `top` is declared as "Alias for limit", + * yet the protocol layer let it OVERWRITE `limit` while the engine kept + * `limit`, so one query resolved two ways depending on the path (#3795's + * divergence, on the pair its scope note excluded). + * + * These tests drive a REAL {@link ObjectQL} engine with an in-memory driver + * that applies `ast.where` exactly as a production driver does — an engine + * double cannot show "the predicate never reached the driver", which is the + * entire failure. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const taskObject = { + name: 'alias_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + }, +}; + +/** A driver that honours `ast.where` — and, like every real one, treats its absence as "all rows". */ +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 expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(o: string, ast: any) { + const all = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + const off = ast?.offset ?? 0; + return typeof ast?.limit === 'number' ? all.slice(off, off + ast.limit) : all.slice(off); + }, + findStream() { throw new Error('not implemented'); }, + 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(`not found: ${o}/${id}`); + const up = { ...cur, ...data, id }; s.set(id, up); return up; + }, + async upsert(o: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(o).has(id)) return this.update(o, id, data); + return this.create(o, data); + }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async aggregate(o: string, ast: any) { return [{ count: (await this.find(o, ast)).length }]; }, + async bulkCreate(o: string, rows: Record[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(o: string, ast: any, data: Record) { + const s = storeFor(o); let n = 0; + for (const [id, row] of s) { + if (!matches(row, ast?.where)) continue; + s.set(id, { ...row, ...data, id }); n += 1; + } + return n; + }, + async deleteMany(o: string, ast: any) { + const s = storeFor(o); let n = 0; + for (const [id, row] of Array.from(s)) { + if (!matches(row, ast?.where)) continue; + s.delete(id); n += 1; + } + return n; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +const CONFLICT = { status: 400, code: 'INVALID_REQUEST' }; + +describe('#4346 — every engine verb folds `filter` → `where`, not just find()', () => { + let engine: ObjectQL; + let rows: Map>; + + beforeEach(async () => { + engine = new ObjectQL(); + const made = makeMemoryDriver(); + engine.registerDriver(made.driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + rows = new Map>([ + ['a', { id: 'a', title: 'A', status: 'open' }], + ['b', { id: 'b', title: 'B', status: 'done' }], + ['c', { id: 'c', title: 'C', status: 'done' }], + ]); + made.stores.set('alias_task', rows); + }); + + const DONE = { status: 'done' }; + + // ── reads ──────────────────────────────────────────────────────── + + it('find applies a `filter` predicate (the one verb that always did)', async () => { + const got = await engine.find('alias_task', { filter: DONE } as any); + expect(got.map((r: any) => r.id)).toEqual(['b', 'c']); + }); + + it('findOne returns a MATCHING row, not the first row of the object', async () => { + // Before: `a` — status 'open'. The predicate never reached the driver, + // so "first match" was "first row". + const got: any = await engine.findOne('alias_task', { filter: DONE } as any); + expect(got?.id).toBe('b'); + expect(got?.status).toBe('done'); + }); + + it('count counts the MATCHES, not the whole object', async () => { + expect(await engine.count('alias_task', { filter: DONE } as any)).toBe(2); + }); + + it('aggregate runs on the filtered set', async () => { + const got = await engine.aggregate('alias_task', { + filter: DONE, aggregations: [{ function: 'count', field: 'id', alias: 'count' }], + } as any); + expect(got[0].count).toBe(2); + }); + + // ── writes: the destructive half ───────────────────────────────── + + it('update rewrites ONLY the matched rows', async () => { + // Before: all three rows were rewritten — `updateMany` received an AST + // with no `where` at all. + await engine.update('alias_task', { title: 'ZAP' }, { filter: DONE, multi: true } as any); + expect(Array.from(rows.values()).map((r: any) => `${r.id}:${r.title}`)) + .toEqual(['a:A', 'b:ZAP', 'c:ZAP']); + }); + + it('delete removes ONLY the matched rows', async () => { + // Before: the object was emptied. + await engine.delete('alias_task', { filter: DONE, multi: true } as any); + expect(Array.from(rows.keys())).toEqual(['a']); + }); + + // ── one slot, one value ────────────────────────────────────────── + + it.each(['find', 'findOne', 'count'] as const)( + '%s refuses `filter` and `where` carrying different values', + async (method) => { + await expect( + (engine as any)[method]('alias_task', { filter: DONE, where: { status: 'open' } }), + ).rejects.toMatchObject(CONFLICT); + }, + ); + + it('delete refuses a conflicting pair rather than guessing which predicate to run', async () => { + await expect( + engine.delete('alias_task', { filter: DONE, where: { status: 'open' }, multi: true } as any), + ).rejects.toMatchObject(CONFLICT); + expect(Array.from(rows.keys())).toEqual(['a', 'b', 'c']); + }); + + it('accepts redundant IDENTICAL spellings — only a conflict is ambiguous', async () => { + const got = await engine.find('alias_task', { filter: DONE, where: { ...DONE } } as any); + expect(got.map((r: any) => r.id)).toEqual(['b', 'c']); + }); + + it('leaves the caller\'s own options object untouched', async () => { + // The fold copies: a caller reusing its bag must not find `filter` + // silently renamed underneath it. + const opts = { filter: DONE }; + await engine.find('alias_task', opts as any); + expect(opts).toEqual({ filter: DONE }); + }); + + // ── the sixth pair: top / limit ────────────────────────────────── + + it('`top` alone still limits the page (OData compatibility)', async () => { + expect((await engine.find('alias_task', { top: 2 } as any)).length).toBe(2); + }); + + it('refuses `top` and `limit` carrying different values, on every path', async () => { + // Before: the engine kept `limit` (3) while the protocol overwrote it + // with `top` (1) — the same query, two answers. + await expect( + engine.find('alias_task', { top: 1, limit: 3 } as any), + ).rejects.toMatchObject(CONFLICT); + }); + + it('findOne ignores `top` instead of colliding with its forced limit of 1', async () => { + // `findOne` sets `limit: 1` itself, so a caller's `top` is discarded — + // folding it would manufacture a conflict out of a request that has no + // ambiguity in it. + const got: any = await engine.findOne('alias_task', { top: 5, where: DONE } as any); + expect(got?.id).toBe('b'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index df867f2174..4f4a162435 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9,6 +9,11 @@ import { EngineDeleteOptions, EngineAggregateOptions, EngineCountOptions, + ENGINE_FILTER_ALIAS_SLOTS, + ENGINE_QUERY_ALIAS_SLOTS, + foldQueryAliasSlots, + type QueryAliasConflict, + type QueryAliasSlot, type DroppedFieldsEvent } from '@objectstack/spec/data'; import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; @@ -321,6 +326,52 @@ interface SummaryDescriptor { filter?: Record; } +/** + * [#4346] Two spellings of one option slot, carrying different values. Same + * rule the protocol layer applies (#4181): merging them would invent an intent + * the caller never expressed, and picking a winner is the silent drop itself. + * + * Carries the wire shape (`status` / `code`) so a call arriving through REST + * surfaces as a 400 rather than a 500 — the engine is reached both ways. + */ +function aliasConflictError(conflict: QueryAliasConflict): Error { + const err: any = new Error( + `Conflicting query options: ${conflict.spellings.map((s) => `'${s}'`).join(', ')} are ` + + `spellings of the same option (canonical '${conflict.canonical}') and were given ` + + 'different values. Send exactly one.', + ); + err.status = 400; + err.code = 'INVALID_REQUEST'; + return err; +} + +/** + * [#4346] Fold the deprecated option aliases onto their canonical QueryAST + * keys, by the spec's own table — returning a COPY, so a caller's bag is never + * mutated under it. + * + * Why every entry point needs this and not just `find()`: the driver AST + * understands `where` only, so an unfolded `filter` is not a narrower query, + * it is NO query. On a read that silently over-grants (the ADR-0057 finding + * that put the fold in `find()`); on `updateMany`/`deleteMany` it is an + * unbounded write over the whole object. The deprecated + * `DataEngine*OptionsSchema` shapes declare `filter` for exactly these verbs + * and `ScopedContext` forwards hook-authored bags verbatim, so "callers should + * not pass it" was never the contract. + */ +function foldOptionAliases( + options: T, + slots: readonly QueryAliasSlot[], +): T { + if (!options || typeof options !== 'object') return options; + const bag = options as Record; + // Fast path: nothing to fold, so nothing to copy either. + if (!slots.some((slot) => slot.aliases.some((alias) => bag[alias] !== undefined))) return options; + const copy = { ...bag }; + foldQueryAliasSlots(copy, slots, (conflict) => { throw aliasConflictError(conflict); }); + return copy as T; +} + export class ObjectQL implements IDataEngine { /** * Ambient transaction store (ADR-0034). While a `transaction()` callback @@ -2862,20 +2913,15 @@ export class ObjectQL implements IDataEngine { 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; + // [#4346] `filter` → `where` and `top` → `limit`, by the spec's table. + // This is where the fold used to be open-coded twice, in two directions: + // `where` won its pair (correct — the driver AST understands `where` only, + // so an unfolded `filter` matched ALL rows, the ADR-0057 over-grant), while + // the protocol layer let `top` overwrite `limit` (backwards — `top` is + // declared as the alias). One table now answers both, here and there. + foldQueryAliasSlots(ast as Record, ENGINE_QUERY_ALIAS_SLOTS, (conflict) => { + throw aliasConflictError(conflict); + }); // Plan formula projection: rewrite ast.fields to drop virtual formula // names and inject their dependencies, so the driver returns the raw @@ -3004,7 +3050,11 @@ export class ObjectQL implements IDataEngine { objectName = this.resolveObjectName(objectName); this.logger.debug('FindOne operation', { objectName }); const driver = this.getDriver(objectName); - const ast: QueryAST = { object: objectName, ...query, limit: 1 }; + // [#4346] Fold BEFORE `limit: 1` is applied: `findOne` forces the page + // size, so a caller's `top` is discarded rather than folded — feeding it to + // the pagination slot would collide with the forced `1` and reject a + // request that has no ambiguity in it. + const ast: QueryAST = { object: objectName, ...foldOptionAliases(query, ENGINE_FILTER_ALIAS_SLOTS), limit: 1 }; // Remove context and top alias from the AST delete (ast as any).context; delete (ast as any).top; @@ -3371,6 +3421,12 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'update'); const driver = this.getDriver(object); + // [#4346] `filter` → `where` FIRST: everything below reads `options.where` + // — the placeholder resolution, the by-id fast path, the seeded AST the + // middleware chain scopes — so an unfolded alias did not narrow the write, + // it left it unbounded, and `updateMany` rewrote every row in the object. + options = foldOptionAliases(options, ENGINE_FILTER_ALIAS_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 +3820,11 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'delete'); const driver = this.getDriver(object); + // [#4346] Same fold, same ordering reason as update() — and the sharpest + // consequence of skipping it: `delete({filter, multi: true})` reached + // `deleteMany` with no predicate and emptied the object. + options = foldOptionAliases(options, ENGINE_FILTER_ALIAS_SLOTS); + // Expand `{filter-placeholder}` values before the id is extracted — same // reasoning as update() above (#3810). options = this.withResolvedWhere(options); @@ -3894,6 +3955,10 @@ export class ObjectQL implements IDataEngine { object = this.resolveObjectName(object); const driver = this.getDriver(object); + // [#4346] An unfolded `filter` counted the RAW table — the same wrong + // number the #2737 middleware fix was about, reached by a different route. + query = foldOptionAliases(query, ENGINE_FILTER_ALIAS_SLOTS); + // The AST must ride on the opCtx so the security/sharing middlewares can // inject their read filters (RLS, OWD/sharing scope) into `ast.where` — // exactly like find(). Building it locally inside the executor (#2737) @@ -3969,6 +4034,9 @@ export class ObjectQL implements IDataEngine { async aggregate(object: string, query: EngineAggregateOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); + // [#4346] Folded before the credential gate reads it, so the gate judges + // the predicate that will actually run. + query = foldOptionAliases(query, ENGINE_FILTER_ALIAS_SLOTS); this.rejectCredentialAggregation(object, query); const driver = this.getDriver(object); this.logger.debug(`Aggregate on ${object} using ${driver.name}`, query); diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index e82357ccf4..bf3d28ca28 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -1278,6 +1278,27 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { ).rejects.toThrow(/'\$expand'/); }); + it('folds `top` into `limit` in the SAME direction the engine does (#4346)', async () => { + // This layer used to do `options.limit = Number(options.top)` with + // no guard — the alias overwriting the canonical key — while + // `engine.find` kept `limit`. `{top: 1, limit: 3}` answered 1 here + // and 3 there: #3795's divergence on the sixth pair. + const { protocol, engine } = makeProtocol(); + await protocol.findData({ object: 'showcase_task', query: { top: '7' } }); + expect(engine.find.mock.calls[0][1].limit).toBe(7); + expect('top' in engine.find.mock.calls[0][1]).toBe(false); + + await expect( + protocol.findData({ object: 'showcase_task', query: { top: 1, limit: 3 } }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + }); + + it('$top keeps working through the OData rewrite into the same slot', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ object: 'showcase_task', query: { $top: '4' } }); + expect(engine.find.mock.calls[0][1].limit).toBe(4); + }); + it('a canonical key alone is never touched by the fold', async () => { const { protocol, engine } = makeProtocol(); await protocol.findData({ diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index d9d40ce48a..6ef93e0ec0 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -313,6 +313,8 @@ "DriverType (const)", "DroppedFieldsEvent (type)", "DroppedFieldsEventSchema (const)", + "ENGINE_FILTER_ALIAS_SLOTS (const)", + "ENGINE_QUERY_ALIAS_SLOTS (const)", "ESignatureConfig (type)", "ESignatureConfigSchema (const)", "EffectiveApiMethods (interface)", diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index f3d1c4c453..de7a930367 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -423,6 +423,40 @@ export const RPC_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = [ { canonical: 'expand', aliases: ['populate'] }, ]; +/** + * The `where` slot alone (#4346) — for engine option bags that carry a + * predicate and nothing else to fold: `findOne`, `count`, `update`, `delete`, + * `aggregate`. + * + * These take the same `filter`-carrying option shapes the deprecated + * `DataEngine{Query,Update,Delete,Count}OptionsSchema` declare, but only + * `find()` ever folded the alias — so `delete({filter, multi: true})` reached + * the driver with NO predicate and emptied the object. The fold is what makes + * the deprecated spelling mean what it says on every verb. + */ +export const ENGINE_FILTER_ALIAS_SLOTS: readonly QueryAliasSlot[] = RPC_QUERY_ALIAS_SLOTS.filter( + (slot) => slot.canonical === 'where', +); + +/** + * The sixth pair (#4346): `top` is documented as "Alias for limit (OData + * compatibility)" on both `QuerySchema` and `EngineQueryOptionsSchema`, so + * `limit` is canonical — but the two normalizers folded it in OPPOSITE + * directions (the protocol overwrote `limit` with `top` unguarded; the engine + * kept `limit`), which is the #3795 divergence on the one pair that sweep's + * scope note excluded. + * + * Deliberately NOT in {@link RPC_QUERY_ALIAS_SLOTS}: those five are deprecated + * RPC keys that the parse transform drops from its output, while `top` is a + * DECLARED AST key. Removing it from the parsed type is a breaking change that + * deserves its own decision; giving it one precedence does not have to wait for + * that. + */ +export const ENGINE_QUERY_ALIAS_SLOTS: readonly QueryAliasSlot[] = [ + ...ENGINE_FILTER_ALIAS_SLOTS, + { canonical: 'limit', aliases: ['top'] }, +]; + /** * Fold alias spellings into their canonical slot key, in place. * @@ -478,11 +512,6 @@ export function foldQueryAliasSlots( return arrivedAs; } -/** The `where` slot alone — for RPC options that accept only the `filter` alias. */ -const RPC_WHERE_SLOT: readonly QueryAliasSlot[] = RPC_QUERY_ALIAS_SLOTS.filter( - (slot) => slot.canonical === 'where', -); - function aliasConflictIssue(conflict: QueryAliasConflict): { code: 'custom'; path: string[]; @@ -520,7 +549,7 @@ function foldRpcLegacyFilter( ctx: z.core.$RefinementCtx, ): Omit { const bag: Record = { ...input }; - foldQueryAliasSlots(bag, RPC_WHERE_SLOT, (conflict) => ctx.addIssue(aliasConflictIssue(conflict))); + foldQueryAliasSlots(bag, ENGINE_FILTER_ALIAS_SLOTS, (conflict) => ctx.addIssue(aliasConflictIssue(conflict))); return bag as Omit; } 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)'),