diff --git a/.changeset/19974-having-comparand-shape-face.md b/.changeset/19974-having-comparand-shape-face.md new file mode 100644 index 00000000000..b934433aa27 --- /dev/null +++ b/.changeset/19974-having-comparand-shape-face.md @@ -0,0 +1,30 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql)!: `engine.aggregate({ having })` walks through the shared comparand-shape face, so `having: { total: [5] }` is refused exactly as the same shape in `where` is (#19974) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what `having` accepts on `engine.aggregate` (and on the REST aggregate query that forwards it there). A `having` carrying one of the shapes below used to answer; it is now refused with `INVALID_FILTER` / 400, before any driver is asked for a row. The refusal is the shared face's own message, byte for byte the refusal the same shape gets in `where`, with the path rooted at `having` instead of `where`. It ships as `minor` under the launch-window convention for accept-set narrowings. + +The 2026-09-23 ruling on #19757 refuses an array in the equality slot at the shared comparand-shape face (`assertListComparandShapes` in `@objectstack/spec/data`) "for every driver at once". The face already ran on `where` and on each `aggregations[i].filter`. `having` never reaches a driver: the engine evaluates it itself after aggregation, on both the native `driver.aggregate()` path and the in-memory fallback. That evaluator answered every shape the face refuses. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sqlite-wasm`, over three groups with totals 500, 1250 and 20: + +| you wrote in `having` | what it did before | write instead | +|:--|:--|:--| +| `{ total: [500] }` or `{ total: { $eq: [500] } }`, at any depth under `$and` / `$or` / `$not` | kept the 500 group, because JS `500 == [500]` is true; under `$not` it kept the complement, the 1250 and 20 groups | `{ total: 500 }`, or `{ total: { $in: [500, 1250] } }` for "one of these" | +| `{ total: [] }` | kept no group | drop the condition, or write the value you meant | +| `{ customer_id: { $in: 'c1' } }` / `{ customer_id: { $nin: 'c1' } }` | `$in` kept no group; `$nin` kept every group | `{ customer_id: 'c1' }` / `{ customer_id: { $ne: 'c1' } }`, or wrap the value in a list | +| `{ customer_id: { $in: ['c1', null] } }` (or `$nin`) | the null member was compared as a value | `{ $or: [{ customer_id: { $in: ['c1'] } }, { customer_id: { $null: true } }] }` | +| `{ total: { $gt: null } }` (or `$gte` / `$lt` / `$lte`) | `$gt` / `$gte` kept every group; `$lt` / `$lte` kept none | `{ total: { $eq: null } }` for "has no value", `{ total: { $ne: null } }` for "has a value" | +| `{ total: { $between: 500 } }` or `{ total: { $between: [500] } }` | the scalar kept every group; the one-bound list kept the groups at or above it | `{ total: { $between: [min, max] } }` | +| `{ total: { $between: [null, 1000] } }`, `['', 1000]` or `[undefined, 1000]` | the blank bound compared as a value | `{ total: { $lte: 1000 } }` for a one-sided range, or the bound you meant | +| `{ total: { $between: [{ $field: 'order_count' }, 1000] } }` | the reference compared as a value | literal bounds. ⚠️ The refusal's own text suggests a two-bound `{ $field }` comparison, which `having` does not evaluate. In an operator slot the reference is compared as a value: under `$eq`, `$gt`, `$gte`, `$lt` or `$lte` it keeps no group, and under `$ne` it keeps every group. In the implicit slot (`{ total: { $field: 'order_count' } }`) it is refused as an unsupported operator (`INVALID_FILTER` / 400), though only when a grouped row carries that column: an empty grouped set evaluates nothing and comes back empty. That gap is not changed here | + +The gate is ONE call in `engine.aggregate`, ahead of both `having` evaluations, so the two paths cannot disagree, and the verdict belongs to the filter rather than to the data: an empty grouped set refuses the same `having` a populated one does. Whatever arm the shared face gains later, `having` gains with it. + +Who is affected: `having` is a request-only key (`QuerySchema.having`, `EngineAggregateOptions.having`), and no metadata type stores it. Every `having` in this repository's docs and published skills is a scalar comparison (`{ order_count: { $gt: 5 } }` and the like), and none authors a refused shape. Callers of `engine.aggregate` and of the REST aggregate query in a deployment were NOT measured. + +Not changed: scalars, `null` in the equality slot (the has-no-value predicate), `$in` / `$nin` lists including the empty list, a two-bound `$between`, and scalar ordering bounds all answer exactly as before, on both paths. `$ne` with a list is not judged by the face yet, so `having` still answers it. Neither the comparand-TYPE door nor the unknown-field and declared-type gates that `where` also passes are run on `having`; this change adds the comparand-shape face only. diff --git a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts new file mode 100644 index 00000000000..b9f71f3a262 --- /dev/null +++ b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts @@ -0,0 +1,346 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#19974] `engine.aggregate({ having })` walks through the shared + * comparand-SHAPE face — the same `assertListComparandShapes` the engine + * already applies to `where` and to `aggregations[i].filter` on this verb. + * + * The 2026-09-23 ruling on #19757 refuses an array in the equality slot at + * that face "for every driver at once". `having` never reaches a driver: the + * engine evaluates it itself (`applyHaving`, having-filter.ts), once after the + * native `driver.aggregate()` door and once after the in-memory fallback. That + * walker sent an array into its implicit-equality arm (`value == condition`) + * and compared `$eq` with `!=`, so by JS coercion (`500 == [500]` is true) the + * shape the ruling refuses still ANSWERED. Measured on the base (9b8c74c6c5) + * through the public `engine.aggregate`, on driver-memory AND + * driver-sqlite-wasm, on both doors, over the groups c1 (total 500), c2 (1250) + * and c3 (20): + * + * | `having` | `where` (same shape) | `having` (both doors, both drivers) | + * |:--|:--|:--| + * | `{ total: [500] }` | 400 `INVALID_FILTER` | c1 — answered by `==` coercion | + * | `{ total: { $eq: [500] } }` | 400 | c1 | + * | `{ customer_id: { $nin: 'c1' } }` | 400 | c1, c2, c3 — the filter dropped | + * | `{ total: { $between: 500 } }` | 400 | c1, c2, c3 — the filter dropped | + * | `{ total: { $lt: null } }` | 400 | no group | + * + * — and every other arm the face refuses answered the same way (the full + * table is {@link FACE_REFUSED}; before this change all 20 rows answered on + * `having` while all 20 were refused on `where`). + * + * Four pins, each on BOTH doors: + * + * 1. the where/having parity table — every shape the face refuses for `where` + * is refused for `having`, with the SAME envelope and the same words, path + * aside, and no driver is asked for a row; + * 2. the shared conformance table — `FILTER_COMPARAND_TYPE_CASES`' door-refusal + * rows that belong to this face, driven down the `having` path; + * 3. scalars and the declared list/range spellings pass and answer exactly as + * before; + * 4. the verdict is the FILTER's, not the data's — an empty grouped set refuses + * the same `having` a populated one does. + * + * Every refusal asserts the ADR-0112 envelope (`code` + `status`); a bare + * `toThrow()` would be satisfied by any uncoded error. + */ + +import { describe, it, expect } from 'vitest'; +import { + assertListComparandShapes, + normalizeFilterComparandTypes, + FILTER_COMPARAND_TYPE_CASES, + type ComparandTypeRefusalCase, + type EngineAggregateOptions, + type FilterCondition, +} from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; + +const OBJECT = 'order'; + +const ROWS = [ + { customer_id: 'c1', amount: 100 }, + { customer_id: 'c1', amount: 400 }, + { customer_id: 'c2', amount: 900 }, + { customer_id: 'c2', amount: 300 }, + { customer_id: 'c2', amount: 50 }, + { customer_id: 'c3', amount: 20 }, +]; + +const AGG_QUERY: EngineAggregateOptions = { + groupBy: ['customer_id'], + aggregations: [ + { function: 'count', alias: 'order_count' }, + { function: 'sum', field: 'amount', alias: 'total' }, + ], +}; + +/** + * The refused shapes are OFF-CONTRACT by design — a scalar `$in`, a null + * `$between` bound — so the bag carrying one is cast through `unknown` to the + * contract it bypasses, never erased to `any`: the rest of the call stays + * checked, and the cast names what is being bypassed. + */ +function offContract(bag: Record): EngineAggregateOptions { + return bag as unknown as EngineAggregateOptions; +} + +interface Calls { aggregate: number; find: number } + +/** + * A stand-in driver that records every read. `native: true` gives it an + * `aggregate()` (the first `applyHaving` door); `native: false` leaves only + * `find()`, so the engine takes the in-memory fallback (the second door). + */ +function makeDriver(rows: ReadonlyArray>, native: boolean) { + const calls: Calls = { aggregate: 0, find: 0 }; + const driver: any = { + name: native ? 'native-agg-recorder' : 'raw-recorder', + version: '0.0.0', + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { calls.find += 1; return rows.map((r) => ({ ...r })); }, + async findOne() { return rows[0] ?? null; }, + async create(_o: string, d: any) { return d; }, + async update(_o: string, _id: string, d: any) { return d; }, + async delete() { return true; }, + async count() { return rows.length; }, + async bulkCreate(_o: string, r: any[]) { return r; }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + if (native) { + // Groups and sums itself, ignores `ast.having` — as every real native + // driver does today; the engine's post-filter is what makes it live. + driver.aggregate = async () => { + calls.aggregate += 1; + const groups = new Map(); + for (const r of rows as Array<{ customer_id: string; amount: number }>) { + const g = groups.get(r.customer_id) ?? { customer_id: r.customer_id, order_count: 0, total: 0 }; + g.order_count += 1; + g.total += r.amount; + groups.set(r.customer_id, g); + } + return Array.from(groups.values()); + }; + } + return { driver, calls }; +} + +async function makeEngine(rows: ReadonlyArray>, native: boolean) { + const { driver, calls } = makeDriver(rows, native); + const engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: OBJECT, + fields: { customer_id: { type: 'text' }, amount: { type: 'number' } }, + } as any); + return { engine, calls }; +} + +const DOORS = [ + ['native driver.aggregate() door', true], + ['in-memory fallback door', false], +] as const; + +interface Refusal extends Error { code?: unknown; status?: unknown } + +async function refusalOf(run: () => Promise): Promise { + let out: unknown; + try { + out = await run(); + } catch (e) { + return e as Refusal; + } + throw new Error(`expected a refusal, but it answered ${JSON.stringify(out)}`); +} + +function syncRefusalOf(run: () => unknown): Refusal | undefined { + try { + run(); + } catch (e) { + return e as Refusal; + } + return undefined; +} + +function expectEnvelope(err: Refusal): void { + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); +} + +/** The group ids an aggregate answered, sorted. */ +const groups = (rows: any[]) => rows.map((r) => r.customer_id).sort(); + +/** + * The face's refused set — one row per arm it carries today, at every depth it + * walks. Each filter is written once and used VERBATIM as a `where` and as a + * `having`: the engine keeps its registry-less tolerance for filter field + * names, so `total` / `order_count` reach the face in `where` exactly as they + * do in `having`, and the two refusals can be compared word for word. + * + * The factory is deliberate: the filters are handed to the engine, and a + * shared instance across tests would let one call's handling change what the + * next one judged. + */ +const FACE_REFUSED: ReadonlyArray Record]> = [ + ['an array in the implicit-equality slot (the triage shape)', () => ({ total: [500] })], + ['an EMPTY array in the implicit-equality slot', () => ({ total: [] })], + ['an array under $eq (the triage shape)', () => ({ total: { $eq: [500] } })], + ['an equality-slot array nested in $or', () => ({ $or: [{ order_count: 99 }, { total: [500] }] })], + ['an equality-slot array nested in $and', () => ({ $and: [{ customer_id: 'c1' }, { total: [500] }] })], + ['an equality-slot array under $not', () => ({ $not: { total: [500] } })], + ['a scalar $in', () => ({ customer_id: { $in: 'c1' } })], + ['a scalar $nin', () => ({ customer_id: { $nin: 'c1' } })], + ['a null $in member', () => ({ customer_id: { $in: ['c1', null] } })], + ['a null $nin member', () => ({ customer_id: { $nin: ['c1', null] } })], + ['$gt: null', () => ({ total: { $gt: null } })], + ['$gte: null', () => ({ total: { $gte: null } })], + ['$lt: null', () => ({ total: { $lt: null } })], + ['$lte: null', () => ({ total: { $lte: null } })], + ['a scalar $between', () => ({ total: { $between: 500 } })], + ['a one-bound $between', () => ({ total: { $between: [500] } })], + ['a null $between bound', () => ({ total: { $between: [null, 1000] } })], + ['an empty-string $between bound', () => ({ total: { $between: ['', 1000] } })], + ['an undefined $between bound', () => ({ total: { $between: [undefined, 1000] } })], + ['a { $field } $between bound', () => ({ total: { $between: [{ $field: 'order_count' }, 1000] } })], +]; + +describe('[#19974] having — the where/having parity table over the comparand-shape face', () => { + for (const [name, filter] of FACE_REFUSED) { + it(`${name}: refused as a where AND as a having, one envelope, one wording, both doors`, async () => { + const { engine: whereEngine, calls: whereCalls } = await makeEngine(ROWS, true); + const whereErr = await refusalOf(() => + whereEngine.aggregate(OBJECT, offContract({ ...AGG_QUERY, where: filter() }))); + expectEnvelope(whereErr); + expect(whereCalls).toEqual({ aggregate: 0, find: 0 }); + // The row is a real member of the face's refused set, and the where + // refusal is the face's own sentence — not some other gate that happens + // to refuse the same input. + const faceWhere = syncRefusalOf(() => assertListComparandShapes(filter(), `aggregate('${OBJECT}')`)); + expect(faceWhere, 'the face must refuse this row directly').toBeDefined(); + expect(whereErr.message).toBe(faceWhere!.message); + expect(whereErr.message).toContain('where.'); + expect(whereErr.message).not.toContain('having.'); + + for (const [door, native] of DOORS) { + const { engine, calls } = await makeEngine(ROWS, native); + const havingErr = await refusalOf(() => + engine.aggregate(OBJECT, offContract({ ...AGG_QUERY, having: filter() }))); + expectEnvelope(havingErr); + // Byte for byte the `where` refusal of the same shape, path aside. + expect(havingErr.message, door).toBe(whereErr.message.replaceAll('where.', 'having.')); + // Refused before either door is chosen: no driver was asked for a row. + expect(calls, door).toEqual({ aggregate: 0, find: 0 }); + } + }); + } + + it('the arm this card does not move ($ne with an array) is held to the face\'s own answer', async () => { + // `$ne` is equality's negation, left out of the 2026-09-23 ruling and + // carried by #19886. Whatever the face answers for it, `having` answers + // the same — refused with the face's words once the face refuses it, and + // passed while the face passes it — so this row cannot drift from the + // `where` side in either direction. + const filter = () => ({ total: { $ne: [500] } }); + const face = syncRefusalOf(() => assertListComparandShapes(filter(), `aggregate('${OBJECT}')`, 'having')); + for (const [door, native] of DOORS) { + const { engine } = await makeEngine(ROWS, native); + const run = () => engine.aggregate(OBJECT, offContract({ ...AGG_QUERY, having: filter() })); + if (face) { + const err = await refusalOf(run); + expectEnvelope(err); + expect(err.message, door).toBe(face.message); + } else { + await expect(run(), door).resolves.toBeInstanceOf(Array); + } + } + }); +}); + +describe('[#19974] having — driven from the shared FILTER_COMPARAND_TYPE_CASES table', () => { + const doorRefusals = FILTER_COMPARAND_TYPE_CASES.filter( + (c): c is ComparandTypeRefusalCase => c.verdict === 'door-refusal'); + // The table carries rows for TWO faces: the comparand-SHAPE face (the #19757 + // equality-slot rows) and the comparand-TYPE door (#7872: undefined, a + // function, a Map, …). Only the first is this change's subject. The split is + // taken from the faces themselves, never from a list kept here, so a row + // added to the table later lands on the right side of it by construction. + const shapeRows = doorRefusals.filter((c) => + syncRefusalOf(() => assertListComparandShapes(c.filter(), `aggregate('${OBJECT}')`, 'having')) !== undefined); + const otherRows = doorRefusals.filter((c) => !shapeRows.includes(c)); + + it('the table carries shape-face rows at all — the leg below can never pass on zero rows', () => { + expect(shapeRows.length).toBeGreaterThanOrEqual(3); + }); + + it('every row the shape face does NOT refuse belongs to the comparand-TYPE door instead', () => { + // Recorded, not skipped: these rows are the type door's subject, not this + // face's, and the assertion proves the partition is principled. + for (const c of otherRows) { + expect(syncRefusalOf(() => normalizeFilterComparandTypes(c.filter())), c.name).toBeDefined(); + } + }); + + for (const c of shapeRows) { + it(`${c.name} — on the having path`, async () => { + for (const [door, native] of DOORS) { + const { engine, calls } = await makeEngine(ROWS, native); + const err = await refusalOf(() => + engine.aggregate(OBJECT, { ...AGG_QUERY, having: c.filter() })); + expect(err.code, door).toBe(c.code); + expect(err.status, door).toBe(400); + for (const needle of c.mustMention) { + expect(err.message, `${door}: ${needle}`).toContain(needle.replaceAll('where.', 'having.')); + } + expect(calls, door).toEqual({ aggregate: 0, find: 0 }); + } + }); + } +}); + +describe('[#19974] having — what the face leaves alone answers exactly as before', () => { + const PASSING: ReadonlyArray = [ + ['a scalar in the implicit-equality slot', { total: 500 }, ['c1']], + ['a scalar under $eq', { total: { $eq: 500 } }, ['c1']], + ['a string in the implicit-equality slot', { customer_id: 'c2' }, ['c2']], + ['$eq: null (the null predicate)', { total: { $eq: null } }, []], + ['a list under $in', { customer_id: { $in: ['c1', 'c3'] } }, ['c1', 'c3']], + ['an EMPTY list under $in', { customer_id: { $in: [] } }, []], + ['a list under $nin', { customer_id: { $nin: ['c1'] } }, ['c2', 'c3']], + ['a [min, max] pair under $between', { total: { $between: [100, 1000] } }, ['c1']], + ['a scalar ordering bound', { total: { $gt: 100 } }, ['c1', 'c2']], + ['scalars composed under $and / $or', { $or: [{ total: 20 }, { $and: [{ order_count: { $gte: 3 } }] }] }, ['c2', 'c3']], + ]; + + for (const [name, having, expected] of PASSING) { + it(`${name} answers ${JSON.stringify(expected)} on both doors`, async () => { + for (const [door, native] of DOORS) { + const { engine } = await makeEngine(ROWS, native); + const rows = await engine.aggregate(OBJECT, { ...AGG_QUERY, having }); + expect(groups(rows), door).toEqual([...expected]); + } + }); + } +}); + +describe('[#19974] having — the verdict belongs to the filter, not to the data', () => { + it('an EMPTY grouped set refuses the triage shapes exactly as a populated one does', async () => { + // On the base an empty set answered `[]` for `{ total: [500] }` with no + // error, and a populated one answered `['c1']` — neither was a refusal, and + // a walker-local check would have kept the empty set silent, because the + // walker only runs per aggregated row. + for (const having of [{ total: [500] }, { total: { $eq: [500] } }]) { + for (const [door, native] of DOORS) { + const { engine: empty } = await makeEngine([], native); + const { engine: populated } = await makeEngine(ROWS, native); + const emptyErr = await refusalOf(() => empty.aggregate(OBJECT, offContract({ ...AGG_QUERY, having }))); + const populatedErr = await refusalOf(() => populated.aggregate(OBJECT, offContract({ ...AGG_QUERY, having }))); + expectEnvelope(emptyErr); + expect(emptyErr.message, door).toBe(populatedErr.message); + } + } + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 277a986d82c..7830b0cee32 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -15603,6 +15603,30 @@ export class ObjectQL implements IObjectQLEngine { // single verb. assertTextOperatorTargetsAreStringCapable(object, 'aggregate', this._registry.getObject(object), aggFilter); } + // [#19974] `having` is this verb's THIRD filter position, and it walks + // through the same comparand-shape face the other two take above — + // called once, here, with the path seeded at `having`. The 2026-09-23 + // ruling on #19757 refuses an array in the equality slot at that face + // "for every driver at once", and the face refuses it for `where` on + // every driver. `having` never reaches a driver, though: the engine + // evaluates it itself (`applyHaving`, having-filter.ts), and that walker + // ANSWERED every shape the face refuses — `{ total: [5] }` by JS `==` + // coercion (`5 == [5]` is true), `{ total: { $eq: [5] } }` by `!=`, a + // scalar `$nin` or a malformed `$between` by keeping every group, a null + // `$lt` bound by keeping none. Measured on the base through this method, + // on driver-memory and driver-sqlite-wasm, on both doors below. + // + // ONE call covers BOTH `applyHaving` doors — the native + // `driver.aggregate()` path and the in-memory fallback — because it runs + // before either is chosen and before any driver is asked for a row. The + // shape of a comparand is a property of the FILTER, so it is judged once + // per query, never per aggregated row: an empty grouped set refuses the + // same `having` a populated one does. ⛔ Not a second face and not a + // walker-local check in having-filter.ts: the verdicts, the wording and + // the `INVALID_FILTER` / 400 envelope are the face's own, so a `having` + // refusal reads byte for byte as the `where` refusal of the same shape, + // path aside — and whatever arm the face gains next, `having` gains too. + assertListComparandShapes(object, 'aggregate', query.having, 'having'); const driver = this.getDriver(object); this.logger.debug(`Aggregate on ${object} using ${driver.name}`, query);