From 41111e17d4b89ce9f5395a532330b7fcdfaa1939 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:18:28 +0800 Subject: [PATCH] fix(objectql): a direct engine call carrying sort/select/skip/populate now throws instead of silently dropping it (#4371) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Claude Fable 5 --- .../engine-rejects-wire-only-aliases.md | 32 +++ .../src/engine-wire-alias-reject.test.ts | 187 ++++++++++++++++++ packages/objectql/src/engine.ts | 85 +++++++- 3 files changed, 300 insertions(+), 4 deletions(-) create mode 100644 .changeset/engine-rejects-wire-only-aliases.md create mode 100644 packages/objectql/src/engine-wire-alias-reject.test.ts diff --git a/.changeset/engine-rejects-wire-only-aliases.md b/.changeset/engine-rejects-wire-only-aliases.md new file mode 100644 index 0000000000..ce93316050 --- /dev/null +++ b/.changeset/engine-rejects-wire-only-aliases.md @@ -0,0 +1,32 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a direct engine call carrying `sort`/`select`/`skip`/`populate` now throws instead of silently dropping the parameter (#4371) + +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 (`sort`'s `{field: 'asc'}` record +form, `populate`'s name list) that belongs there. A **direct** `engine.find()` +/ `findOne()` never crosses that layer, so one of those keys used to ride the +AST verbatim, drivers read only the canonical name, and the request succeeded +with the parameter discarded — `sort` + `limit` ("the latest N") silently +returning an arbitrary N. Three shipped instances were fixed in #4370, and a +fourth sat in the engine's own autonumber seeding (`select` — now `fields`). + +`find`/`findOne` now reject a non-null wire-only spelling with an error naming +the canonical key and shape, e.g.: + +> `find('task') does not accept 'sort': 'sort' is a wire spelling of +> 'orderBy', folded by the RPC/protocol layer — a direct engine call bypasses +> that fold, so the value would be silently dropped, not applied. Pass +> 'orderBy' (SortNode[]: [{ field, order: 'asc' | 'desc' }]) instead.` + +Migration for direct engine callers (HTTP/RPC callers are unaffected — the +wire fold is unchanged): `select: [...]` → `fields: [...]`; +`sort: {f: 'asc'}` or `sort: [{field, order}]` → `orderBy: [{field, order}]`; +`skip: n` → `offset: n`; `populate: ['rel']` → `expand: {rel: {object: 'rel'}}`. +An explicit `null` under a wire spelling remains a withdrawal (ignored), and +the where-only methods (`update`/`delete`/`count`/`aggregate`) are unchanged — +their contracts honour no sort/projection/pagination in either spelling +(unknown-key enforcement there is #4371's follow-up scope). diff --git a/packages/objectql/src/engine-wire-alias-reject.test.ts b/packages/objectql/src/engine-wire-alias-reject.test.ts new file mode 100644 index 0000000000..468adbea60 --- /dev/null +++ b/packages/objectql/src/engine-wire-alias-reject.test.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4371 — a direct engine call carrying a wire-only alias spelling + * (`sort`/`select`/`skip`/`populate`) is REJECTED, not silently dropped. + * + * Regression: #4346 folds `filter`→`where` and `top`→`limit` on every engine + * entry point, but the other four pairs in `RPC_QUERY_ALIAS_SLOTS` fold at the + * RPC/protocol layer only (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 discarded — `sort` + `limit` ("the latest N") + * silently returning an arbitrary N. Three shipped instances were fixed in + * #4370; a fourth sat in the engine's own `seedAutonumber`. The engine now + * throws at the boundary, naming the canonical key and shape, while the + * RPC/wire path keeps folding exactly as before — the fold must NOT move. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +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 }, + }, +}; + +/** Memory driver that records every AST handed to find/findOne. */ +function makeRecordingDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { let s = stores.get(o); if (!s) { s = new Map(); stores.set(o, s); } return s; }; + const finds: any[] = []; + 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 run = (o: string, ast: any) => { + let rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + const ord = Array.isArray(ast?.orderBy) ? ast.orderBy : []; + if (ord.length > 0) { + rows = [...rows].sort((a: any, b: any) => { + for (const { field, order } of ord) { + const cmp = String(a?.[field] ?? '').localeCompare(String(b?.[field] ?? '')); + if (cmp !== 0) return order === 'desc' ? -cmp : cmp; + } + return 0; + }); + } + if (typeof ast?.offset === 'number' && ast.offset > 0) rows = rows.slice(ast.offset); + return typeof ast?.limit === 'number' && ast.limit > 0 ? rows.slice(0, ast.limit) : rows; + }; + 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) { finds.push(ast); return run(o, ast); }, + findStream() { throw new Error('ns'); }, + async findOne(o: string, ast: any) { finds.push(ast); return run(o, ast)[0] ?? 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 delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return run(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, finds }; +} + +describe('wire-only alias spellings are rejected on direct engine calls (#4371)', () => { + let engine: ObjectQL; + let finds: any[]; + + beforeEach(async () => { + engine = new ObjectQL(); + const mem = makeRecordingDriver(); + finds = mem.finds; + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(task as any); + await engine.insert('task', { title: 'B', status: 'done' }); + await engine.insert('task', { title: 'A', status: 'open' }); + await engine.insert('task', { title: 'C', status: 'done' }); + finds.length = 0; // drop insert-path reads; the pins below own this log + }); + + // ── each wire spelling throws before the driver sees anything ────── + + it.each([ + ['sort', { created_at: 'asc' }, 'orderBy'], + ['select', ['id', 'title'], 'fields'], + ['skip', 10, 'offset'], + ['populate', ['owner'], 'expand'], + ])('find({%s}) is refused, naming the canonical key', async (alias, value, canonical) => { + await expect(engine.find('task', { [alias]: value } as any)).rejects.toThrow( + new RegExp(`wire spelling of '${canonical}'.*Pass '${canonical}'`, 's'), + ); + expect(finds).toHaveLength(0); + }); + + it('findOne({sort}) is refused too — "first row of THIS order" degrades the same way', async () => { + await expect(engine.findOne('task', { sort: { title: 'asc' } } as any)) + .rejects.toThrow(/findOne\('task'\) does not accept 'sort'/); + expect(finds).toHaveLength(0); + }); + + it('a sort already in SortNode[] shape is still refused — the KEY is the contract', async () => { + await expect(engine.find('task', { sort: [{ field: 'title', order: 'asc' }] } as any)) + .rejects.toThrow(/wire spelling of 'orderBy'/); + }); + + it('several wire spellings at once are reported in ONE rejection', async () => { + await expect( + engine.find('task', { sort: { title: 'asc' }, populate: ['owner'] } as any), + ).rejects.toThrow(/'sort'.*'populate'|'populate'.*'sort'/s); + }); + + // ── the canonical spellings all work — the migration target is real ─ + + it('find({where, fields, orderBy, offset, limit}) reaches the driver canonically', async () => { + const rows = await engine.find('task', { + where: { status: 'done' }, + fields: ['id', 'title'], + orderBy: [{ field: 'title', order: 'desc' }], + offset: 0, + limit: 10, + }); + expect(rows.map((r: any) => r.title)).toEqual(['C', 'B']); + expect(finds).toHaveLength(1); + expect(finds[0].orderBy).toEqual([{ field: 'title', order: 'desc' }]); + expect(finds[0].sort).toBeUndefined(); + }); + + // ── null stays a withdrawal, exactly like the folded slots ────────── + + it('an explicit null wire spelling is a withdrawal, not a rejection', async () => { + const rows = await engine.find('task', { sort: null } as any); + expect(rows).toHaveLength(3); + }); + + // ── the folded pairs did not regress behind the new parameter ────── + + it('find({filter, top}) still folds (#4346) with the rejection guard in place', async () => { + const rows = await engine.find('task', { filter: { status: 'done' }, top: 1 } as any); + expect(rows).toHaveLength(1); + expect(finds[0].where).toEqual({ status: 'done' }); + expect(finds[0].limit).toBe(1); + }); + + // ── the RPC/wire path still folds — the fold must NOT move (#4371 pin 3) ─ + + it('wire `sort` through the protocol layer folds to orderBy and passes the guard', async () => { + const protocol = new ObjectStackProtocolImplementation(engine as any); + const out: any = await protocol.findData({ object: 'task', query: { sort: '-title' } }); + expect(out.records.map((r: any) => r.title)).toEqual(['C', 'B', 'A']); + expect(finds).toHaveLength(1); + expect(finds[0].orderBy).toEqual([{ field: 'title', order: 'desc' }]); + expect(finds[0].sort).toBeUndefined(); + }); + + it('wire `select`/`skip`/`populate` through the protocol layer pass the guard as canonical keys', async () => { + const protocol = new ObjectStackProtocolImplementation(engine as any); + const out: any = await protocol.findData({ + object: 'task', + query: { select: 'id,title', skip: 1, sort: 'title' }, + }); + expect(out.records.map((r: any) => r.title)).toEqual(['B', 'C']); + expect(finds[0].fields).toEqual(expect.arrayContaining(['id', 'title'])); + expect(finds[0].offset).toBe(1); + expect(finds[0].select).toBeUndefined(); + expect(finds[0].skip).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 93bc39ff4b..f747fb56bb 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -116,6 +116,44 @@ const ENGINE_WHERE_SLOTS: readonly QueryAliasSlot[] = const ENGINE_QUERY_SLOTS: readonly QueryAliasSlot[] = RPC_QUERY_ALIAS_SLOTS.filter((slot) => slot.canonical === 'where' || slot.canonical === 'limit'); +/** + * [#4371] The slots the engine does NOT fold — the wire-only pairs + * (`select`→`fields`, `sort`→`orderBy`, `skip`→`offset`, `populate`→`expand`), + * derived as the complement of {@link ENGINE_QUERY_SLOTS} so a seventh pair + * added to the spec table lands on exactly one side of the split. + * + * Their values need shape lowering (`sort`'s `{field: 'asc'}` record form, + * `populate`'s name list) that belongs to the RPC/protocol layers, so folding + * here would re-implement the lowering per reader — the #3795 condition. But a + * DIRECT engine call never crosses those layers: the alias key used to ride + * the AST verbatim, drivers read only the canonical name, and the parameter + * was silently dropped — three shipped "latest N in arbitrary order" bugs + * (#4370) plus the engine's own `seedAutonumber`. Declared ≠ enforced + * (AGENTS.md PD #10): the find-shaped entry points now REJECT these spellings, + * naming the canonical key and shape, so the mistake throws at the call site + * instead of degrading the result. + * + * Deliberately NOT applied to the where-only methods + * (`update`/`delete`/`count`/`aggregate`): their contracts honour no + * sort/projection/pagination at all, so "pass `orderBy` instead" would + * redirect the caller to a key those methods silently ignore too. Unknown-key + * enforcement for those bags is #4371's option (2), scoped separately. + */ +const ENGINE_WIRE_ONLY_SLOTS: readonly QueryAliasSlot[] = + RPC_QUERY_ALIAS_SLOTS.filter((slot) => !ENGINE_QUERY_SLOTS.includes(slot)); + +/** + * Canonical shape each wire-only slot's value must be rewritten into — quoted + * by the rejection so the error carries the full migration, not just the key + * rename (the value shapes differ; that is WHY the engine cannot fold them). + */ +const WIRE_ONLY_CANONICAL_SHAPES: Record = { + fields: "a string[] of field names", + orderBy: "SortNode[]: [{ field, order: 'asc' | 'desc' }]", + offset: 'a number of rows to skip', + expand: 'a record of { relationName: QueryAST }', +}; + /** * 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 @@ -123,6 +161,12 @@ const ENGINE_QUERY_SLOTS: readonly QueryAliasSlot[] = * for one slot are irreconcilable and throw (picking a winner IS the silent * drop), and an explicit `null` alias is a withdrawal. * + * `rejectSlots` ({@link ENGINE_WIRE_ONLY_SLOTS}) names the slots whose alias + * spellings the engine can neither fold nor honour: a non-null value under one + * throws, quoting the canonical key and shape (#4371). `null` stays a + * withdrawal here too — it carries no intent a drop could lose — and rides + * through for drivers to ignore, exactly as before. + * * 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 @@ -133,8 +177,31 @@ function foldEngineOptionAliases( operation: string, bag: T, slots: readonly QueryAliasSlot[], + rejectSlots?: readonly QueryAliasSlot[], ): T { if (!bag) return bag; + if (rejectSlots) { + const refused = rejectSlots.flatMap((slot) => + slot.aliases + .filter((alias) => (bag as Record)[alias] != null) + .map((alias) => ({ alias, canonical: slot.canonical })), + ); + if (refused.length > 0) { + throw new Error( + `${operation}('${object}') does not accept ` + + `${refused.map((r) => `'${r.alias}'`).join(', ')}: ` + + refused + .map( + (r) => + `'${r.alias}' is a wire spelling of '${r.canonical}', folded by the RPC/protocol ` + + `layer — a direct engine call bypasses that fold, so the value would be silently ` + + `dropped, not applied. Pass '${r.canonical}' ` + + `(${WIRE_ONLY_CANONICAL_SHAPES[r.canonical] ?? 'the canonical QueryAST shape'}) instead.`, + ) + .join(' '), + ); + } + } if (!slots.some((slot) => slot.aliases.some((alias) => alias in bag))) return bag; const folded: Record = { ...bag }; foldQueryAliasSlots(folded, slots, (conflict) => { @@ -1169,8 +1236,13 @@ export class ObjectQL implements IDataEngine { execCtx?: ExecutionContextInput, ): Promise { try { + // Canonical `fields`, not the wire spelling `select` — this call sat on + // the exact #4371 silent drop (the projection never applied; the scan + // worked only because an unprojected row still carries `field`), and the + // catch below would have swallowed the guard's rejection into "seed + // from 0", i.e. duplicate autonumbers. const rows = await this.find(object, { - select: ['id', field], + fields: ['id', field], limit: 5000, context: execCtx, } as any); @@ -2919,8 +2991,11 @@ export class ObjectQL implements IDataEngine { // 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); + // conflict rule on every engine entry point (#4346). The wire-only + // spellings (`sort`/`select`/`skip`/`populate`) are REJECTED instead of + // folded — a direct call carrying one used to have it silently dropped + // (#4371, three shipped instances in #4370). + query = foldEngineOptionAliases(object, 'find', query, ENGINE_QUERY_SLOTS, ENGINE_WIRE_ONLY_SLOTS); this.logger.debug('Find operation starting', { object, query }); const driver = this.getDriver(object); const ast: QueryAST = { object, ...query }; @@ -3056,7 +3131,9 @@ export class ObjectQL implements IDataEngine { // 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); + // Wire-only spellings are rejected, same as find() (#4371) — `sort` + // matters here too: findOne({ sort }) means "first row of THIS order". + query = foldEngineOptionAliases(objectName, 'findOne', query, ENGINE_QUERY_SLOTS, ENGINE_WIRE_ONLY_SLOTS); this.logger.debug('FindOne operation', { objectName }); const driver = this.getDriver(objectName); const ast: QueryAST = { object: objectName, ...query, limit: 1 };