From 866212209fe46bb2ba0a57017b7e2e455188d55e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 11:38:49 +0000 Subject: [PATCH 1/6] fix(objectql)!: a per-aggregation filter is judged once, before any row is read (#20122) `ObjectQL.aggregate` evaluates `aggregations[i].filter` itself, per source row of each bucket, with the walker `having` uses. So its refusals were reached only when a row was: `{ amount: { $median: 1 } }` answered 400 on a populated table and 200 on an empty one, a condition on a column the row does not carry counted zero, and a `$or` whose first branch held counted every row. The per-aggregation loop now runs, after the shared doors it already took, the comparand-TYPE door `where` takes (rooted at the aggregation's position, a narrowed bigint copied on write) and the row-independent walk `having` got at its own entry (`assertAggregationFilterIsEvaluable`), in the per-aggregation clause's words. The walk's `{ $field }` NAME check stays `having`'s alone: the filter reads the object's raw columns, whose names the engine does not judge on `where` either. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- ...0122-aggregation-filter-row-independent.md | 32 +++ .../src/engine-aggregate-filter.test.ts | 230 +++++++++++++++++- packages/objectql/src/engine.ts | 53 +++- packages/objectql/src/having-filter.ts | 75 +++++- 4 files changed, 368 insertions(+), 22 deletions(-) create mode 100644 .changeset/20122-aggregation-filter-row-independent.md diff --git a/.changeset/20122-aggregation-filter-row-independent.md b/.changeset/20122-aggregation-filter-row-independent.md new file mode 100644 index 0000000000..3635bf0d77 --- /dev/null +++ b/.changeset/20122-aggregation-filter-row-independent.md @@ -0,0 +1,32 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine.aggregate` is judged once, before any row is read — its refusals no longer depend on whether the table has rows, and it takes the comparand-type door `where` takes (#20122) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what a per-aggregation `filter` accepts on `engine.aggregate`, and on the REST aggregate query (`POST /data/:object/query` with `aggregations`) that forwards it there. Every refusal below is `INVALID_FILTER` / 400, raised in the engine's per-aggregation loop, before any driver is asked for a row, identically on an empty and on a populated table. The refusal names the aggregation that carries the filter (`aggregations[1].filter`). It ships as `minor` under the launch-window convention for accept-set narrowings. + +The engine evaluates a per-aggregation filter itself, in the in-memory fallback, once per source row of each bucket, with the same walker `having` uses. So the walker's refusals were reached only when a row was. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, and through `POST /data/:object/query` on both, over six rows in three groups, with `{ function: 'count', alias: 'n', filter }`: + +| you wrote in `aggregations[i].filter` | what it did before | write instead | +|:--|:--|:--| +| an unknown or retired operator (`{ amount: { $median: 1 } }`, `$nand`, `$regex`, `$regex` with `$options`), an operator of `where` the walker does not evaluate (`$like`, `$ilike`), a non-`$` key beside an operator, or an empty or non-string `$icontains` | refused on a populated table only. An empty table answered `200 []` with a `groupBy`, and `[{ n: 0 }]` without one. (At the REST door an empty or non-string `$icontains` was already refused at ingress, whatever the rows.) | the operator the refusal names | +| an unknown operator on a column the source row does not carry (`{ nope: { $median: 1 } }`) | counted no row, with no error, on a populated table too | the operator the refusal names | +| an unknown operator in a `$or` branch after one that held (`{ $or: [{ amount: { $gt: 0 } }, { amount: { $median: 1 } }] }`) | counted EVERY row on a populated table: the walk stopped at the branch that held | the operator the refusal names | +| `{ amount: { $field: 'cap' } }` (a reference with no operator) | refused as an unsupported `$field` operator on a populated table only | `{ amount: { $eq: { $field: 'cap' } } }`, or `$ne` / `$gt` / `$gte` / `$lt` / `$lte` | +| a `{ $field }` reference as an `$in` / `$nin` member, a `$contains` pattern, or an `$exists` operand | compared the reference object itself: no row under `$in` / `$contains`, every row under `$nin` / `$exists` | a literal there, or the comparison as one of the six scalar operators | +| a `{ $field }` reference whose `addDays` is not an integer (`1.5`, `'7'`) | counted rows by the in-memory evaluator's own reading of that offset, which `FieldReferenceSchema` refuses | a whole-day `addDays` | +| `{ amount: { $eq: { v: 1 } } }`, `{ amount: undefined }`, `{ $eq: new Map() }`, a function, an `undefined` `$in` member, a bigint beyond 2^53, or `{ $gt: { $field: 5 } }` | counted no row (each measured under the operator shown, or in the implicit slot). The same comparand in `where` is refused by the comparand-type door; the per-aggregation filter now gets that door's refusal, rooted at its own position | a string, number, bigint, boolean, `null` or `Date` | +| a `Symbol` comparand | under `$ne`, counted every row; under `$gt`, threw a raw `TypeError` with no `code` and no `status` on a populated table | a literal of one of the types above | + +Not refused, but answering differently: + +- **An exact-range bigint comparand is narrowed to a number, as it is in `where`.** `{ amount: { $in: [400n, 20n] } }` counted no row, because `[400n].includes(400)` is false. It now counts the rows it names. The caller's aggregation entry is not edited. + +Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. Callers in a deployment were NOT measured. + +Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, the zero-row filter `{ $not: {} }`, and `{}`, on both drivers, measured identical before and after. A `filter` that is not a filter object at all (a string, a number, a boolean, a `Map`, an array) is answered as before. diff --git a/packages/objectql/src/engine-aggregate-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index 474a8c6b71..ae896b43fb 100644 --- a/packages/objectql/src/engine-aggregate-filter.test.ts +++ b/packages/objectql/src/engine-aggregate-filter.test.ts @@ -24,10 +24,14 @@ // ADR-0112 `INVALID_FILTER`/400 envelope, naming the aggregation position // — ignoring it would silently answer the unfiltered aggregate, which is // the very defect this key closes. +// * [#20122] every such refusal is the FILTER's, not the data's: raised once, +// before any driver is asked for a row, identically on an empty and on a +// populated table (the last block of this file). import { describe, it, expect } from 'vitest'; -import type { EngineAggregateOptions } from '@objectstack/spec/data'; +import { normalizeFilterComparandTypes, type EngineAggregateOptions } from '@objectstack/spec/data'; import { ObjectQL } from './engine.js'; +import { matchesAggregationFilter } from './having-filter.js'; // The #10413 measurement's dataset shape: opportunities with a stage and an // amount. 6 rows, 2 closed_won worth 700 total. @@ -295,3 +299,227 @@ describe('engine.aggregate — per-aggregation filter (#10576, the #10413 contra expect(thrown!.message).toContain('aggregations[0].filter'); }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#20122] The verdict belongs to the filter, not to the data +// ─────────────────────────────────────────────────────────────────────────── + +/** + * A driver that records every read, with or without a native `aggregate()`. + * A per-aggregation filter forces the in-memory fallback on both kinds (the + * #10576 fork), so a refusal raised AFTER a read shows up as `find: 1`. + */ +function makeCountingDriver(rows: ReadonlyArray>, native: boolean) { + const 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) driver.aggregate = async () => { calls.aggregate += 1; return []; }; + return { driver, calls }; +} + +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; +} + +/** + * The filter sits on the SECOND aggregation, so every refusal must name + * `aggregations[1].filter` — which aggregation carries it, not merely that one + * does. + */ +function withFilter(filter: unknown, groupBy?: string[]): EngineAggregateOptions { + return { + ...(groupBy ? { groupBy } : {}), + aggregations: [ + { function: 'count', alias: 'opp_count' }, + { function: 'count', alias: 'picked', filter }, + ], + } as unknown as EngineAggregateOptions; +} + +const AT = 'aggregations[1].filter'; + +/** + * Refused on both driver kinds, on an EMPTY and a populated table, grouped and + * ungrouped, with one message and no driver read. Returns the message. + * + * Before: every empty-table cell answered — `[]` grouped, `[{ opp_count: 0, + * picked: 0 }]` ungrouped — and the populated cells refused only after the + * driver had been read (`find: 1`), when they refused at all. + */ +async function expectFilterRefusal(filter: () => unknown): Promise { + let message: string | undefined; + for (const native of [true, false]) { + for (const [population, rows] of [['empty', []], ['populated', OPPORTUNITIES]] as const) { + for (const groupBy of [undefined, ['region']]) { + const cell = `${native ? 'native-capable' : 'raw'} driver, ${population}, ${groupBy ? 'grouped' : 'ungrouped'}`; + const { driver, calls } = makeCountingDriver(rows, native); + const engine = await makeEngine(driver); + const err = await refusalOf(() => engine.aggregate('crm_opportunity', withFilter(filter(), groupBy))); + expect(err, cell).toBeInstanceOf(Error); + expect(err.code, cell).toBe('INVALID_FILTER'); + expect(err.status, cell).toBe(400); + expect(calls, cell).toEqual({ aggregate: 0, find: 0 }); + message ??= err.message; + expect(err.message, cell).toBe(message); + } + } + } + return message!; +} + +describe('[#20122] per-aggregation filter — the walker\'s refusals belong to the filter, not to the data', () => { + // Each row: the filter, and a SOURCE row that walks the per-row evaluator + // INTO the refused arm — the floor whose words the engine-level refusal + // keeps. Before, measured through `engine.aggregate` on driver-memory and + // driver-sql (and through `POST /data/:object/query`): the populated table + // refused, the empty one answered; the column-absent row counted ZERO on a + // populated table too (the no-value exit sat before the operator was read); + // and the `$or` row counted EVERY row, its first branch having held. + const WALKER_REFUSED: ReadonlyArray Record, Record]> = [ + ['an unknown condition operator (the triage shape)', () => ({ amount: { $median: 1 } }), { amount: 1 }], + ['an unknown logical operator', () => ({ $nand: [{ amount: 1 }] }), { amount: 1 }], + ['a retired operator', () => ({ stage: { $regex: 'won' } }), { stage: 'closed_won' }], + ['a retired operator with its retired sibling', () => ({ stage: { $regex: 'won', $options: 'i' } }), { stage: 'closed_won' }], + ['an operator of `where` this walker does not evaluate ($like)', () => ({ stage: { $like: 'closed%' } }), { stage: 'closed_won' }], + ['a $like with a dangling escape', () => ({ stage: { $like: 'closed\\' } }), { stage: 'closed_won' }], + ['an empty $icontains', () => ({ stage: { $icontains: '' } }), { stage: 'closed_won' }], + ['a non-string $icontains', () => ({ stage: { $icontains: 5 } }), { stage: 'closed_won' }], + ['a non-$ key beside an operator', () => ({ amount: { $gt: 1, foo: 2 } }), { amount: 5 }], + ['an unknown operator on a column the row does not carry', () => ({ nope: { $median: 1 } }), { nope: 1 }], + ['an unknown operator behind a $or branch that already held', () => ({ $or: [{ amount: { $gt: 0 } }, { amount: { $median: 1 } }] }), { amount: -1 }], + ['an unknown operator under $not', () => ({ $not: { amount: { $median: 1 } } }), { amount: 1 }], + ]; + + for (const [name, filter, floorRow] of WALKER_REFUSED) { + it(`${name}: refused on an empty and a populated table, in the walker's own words`, async () => { + const floor = syncRefusalOf(() => matchesAggregationFilter(floorRow, filter() as never, 1)); + expect(floor, 'the per-row walker must refuse this row when it reaches it').toBeDefined(); + const message = await expectFilterRefusal(filter); + expect(message).toBe(floor!.message); + expect(message).toContain(AT); + }); + } + + // Before: each row counted nothing in any group (`$nin` and `$exists` + // counted every row), or — the bare form — was refused on a populated table + // only, as an unsupported `$field` operator. + const REFERENCE_REFUSED: ReadonlyArray Record, string]> = [ + ['a bare reference with no operator', () => ({ amount: { $field: 'amount' } }), `${AT}.amount`], + ['a reference as an $in member', () => ({ amount: { $in: [{ $field: 'amount' }] } }), `${AT}.amount.$in`], + ['a reference as a $nin member', () => ({ amount: { $nin: [1, { $field: 'amount' }] } }), `${AT}.amount.$nin`], + ['a reference as a $contains pattern', () => ({ stage: { $contains: { $field: 'region' } } }), `${AT}.stage.$contains`], + ['a reference under $exists', () => ({ amount: { $exists: { $field: 'amount' } } }), `${AT}.amount.$exists`], + ['a fractional addDays', () => ({ amount: { $lte: { $field: 'amount', addDays: 1.5 } } }), `${AT}.amount.$lte`], + ['a string addDays', () => ({ amount: { $lte: { $field: 'amount', addDays: '7' } } }), `${AT}.amount.$lte`], + ]; + + for (const [name, filter, path] of REFERENCE_REFUSED) { + it(`${name}: refused at ${path}, whatever the rows`, async () => { + expect(await expectFilterRefusal(filter)).toContain(path); + }); + } +}); + +describe('[#20122] per-aggregation filter — the comparand-TYPE door `where` takes', () => { + // Before: each row counted no row (`$ne` every row) on a populated table, and + // a Symbol under an ordering operator threw a raw `TypeError` with no `code` + // and no `status` — populated tables only; an empty one answered. + const TYPE_REFUSED: ReadonlyArray Record]> = [ + ['a plain object under $eq', () => ({ amount: { $eq: { v: 1 } } })], + ['undefined in the implicit-equality slot', () => ({ amount: undefined })], + ['a Map under $eq', () => ({ amount: { $eq: new Map() } })], + ['a function under $gt', () => ({ amount: { $gt: () => 1 } })], + ['a Symbol under $gt', () => ({ amount: { $gt: Symbol('x') } })], + ['a Symbol under $ne', () => ({ amount: { $ne: Symbol('x') } })], + ['an undefined $in member', () => ({ amount: { $in: [undefined] } })], + ['a bigint beyond 2^53', () => ({ amount: { $gt: 2n ** 60n } })], + ['a { $field } whose $field is not a string', () => ({ amount: { $gt: { $field: 5 } } })], + ['a plain object nested in $or', () => ({ $or: [{ amount: 1 }, { amount: { $eq: { v: 1 } } }] })], + ]; + + for (const [name, filter] of TYPE_REFUSED) { + it(`${name}: refused in the type door's own words, rooted at the aggregation, whatever the rows`, async () => { + const door = syncRefusalOf(() => normalizeFilterComparandTypes(filter(), "aggregate('crm_opportunity')", AT)); + expect(door, 'the type door must refuse this row directly').toBeDefined(); + expect(await expectFilterRefusal(filter)).toBe(door!.message); + }); + } + + it('an exact-range bigint is NARROWED, as it is in where — and the caller\'s entry is not edited', async () => { + // Before: `{ $in: [500n, 200n] }` counted no row — `[500n].includes(500)` + // is false — while the same list in `where` is narrowed to numbers first. + for (const native of [true, false]) { + const { driver } = makeCountingDriver(OPPORTUNITIES, native); + const engine = await makeEngine(driver); + const query = withFilter({ amount: { $in: [500n, 200n] } }); + const rows = await engine.aggregate('crm_opportunity', query); + expect(rows).toEqual([{ opp_count: 6, picked: 2 }]); + expect((query.aggregations![1] as { filter?: unknown }).filter).toEqual({ amount: { $in: [500n, 200n] } }); + } + }); +}); + +describe('[#20122] per-aggregation filter — what the walk leaves alone answers exactly as before', () => { + // Measured identical at base and head through `engine.aggregate` on + // driver-memory and driver-sql. Each answers on both driver kinds; the empty + // table answers the ruled empty values. + const PASSING: ReadonlyArray Record, number]> = [ + ['implicit equality', () => ({ stage: 'closed_won' }), 2], + ['a scalar ordering bound', () => ({ amount: { $gt: 100 } }), 4], + ['a list under $in', () => ({ amount: { $in: [500, 20] } }), 2], + ['a list under $nin', () => ({ stage: { $nin: ['open'] } }), 4], + ['a [min, max] pair under $between', () => ({ amount: { $between: [50, 300] } }), 3], + ['a case-insensitive $icontains', () => ({ stage: { $icontains: 'WON' } }), 2], + ['$startsWith', () => ({ stage: { $startsWith: 'closed' } }), 4], + ['$ne: null', () => ({ region: { $ne: null } }), 6], + ['$exists', () => ({ region: { $exists: true } }), 6], + ['composed under $or / $not', () => ({ $or: [{ amount: { $gt: 800 } }, { $not: { stage: { $ne: 'closed_lost' } } }] }), 3], + ['a { $field } reference in a scalar comparison', () => ({ amount: { $gte: { $field: 'amount' } } }), 6], + ['the zero-row filter analytics lowers FALSE to', () => ({ $not: {} }), 0], + ['an exact bigint in the implicit slot', () => ({ amount: 500n }), 1], + ]; + + for (const [name, filter, populatedCount] of PASSING) { + it(`${name} counts ${populatedCount} of 6, and 0 on an empty table`, async () => { + for (const native of [true, false]) { + const { driver } = makeCountingDriver(OPPORTUNITIES, native); + const populated = await makeEngine(driver); + expect(await populated.aggregate('crm_opportunity', withFilter(filter()))).toEqual([{ opp_count: 6, picked: populatedCount }]); + const { driver: emptyDriver } = makeCountingDriver([], native); + const empty = await makeEngine(emptyDriver); + expect(await empty.aggregate('crm_opportunity', withFilter(filter()))).toEqual([{ opp_count: 0, picked: 0 }]); + } + }); + } +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d1ed9ee6fe..2594a03a37 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -259,6 +259,7 @@ import { import { applyHaving, aggregatedRowColumns, + assertAggregationFilterIsEvaluable, assertHavingIsEvaluable, assertHavingIsFilterCondition, } from './having-filter.js'; @@ -15984,17 +15985,47 @@ export class ObjectQL implements IObjectQLEngine { // (#8296 — a typo'd column would otherwise select ZERO rows for that one // aggregation, silently, which is the same wrong-number shape #10413 // measured). The path names which aggregation carries the offending key. - for (const [i, agg] of (Array.isArray(query.aggregations) ? query.aggregations : []).entries()) { - const aggFilter = (agg as { filter?: unknown })?.filter; - if (aggFilter == null) continue; - assertListComparandShapes(object, 'aggregate', aggFilter, `aggregations[${i}].filter`); - assertFilterIsMaterializable(object, 'aggregate', this._registry.getObject(object), aggFilter); - // [#15661] …and the declared-type door for the text operators: a - // `$contains` over a numeric column in ONE aggregation's filter is - // the same silent zero at a second filter position, and a door that - // spoke on `where` alone would answer one mistake two ways within a - // single verb. - assertTextOperatorTargetsAreStringCapable(object, 'aggregate', this._registry.getObject(object), aggFilter); + { + const aggregations = Array.isArray(query.aggregations) ? query.aggregations : []; + let typedAggregations: typeof aggregations | undefined; + for (const [i, agg] of aggregations.entries()) { + const aggFilter = (agg as { filter?: unknown })?.filter; + if (aggFilter == null) continue; + assertListComparandShapes(object, 'aggregate', aggFilter, `aggregations[${i}].filter`); + assertFilterIsMaterializable(object, 'aggregate', this._registry.getObject(object), aggFilter); + // [#15661] …and the declared-type door for the text operators: a + // `$contains` over a numeric column in ONE aggregation's filter is + // the same silent zero at a second filter position, and a door that + // spoke on `where` alone would answer one mistake two ways within a + // single verb. + assertTextOperatorTargetsAreStringCapable(object, 'aggregate', this._registry.getObject(object), aggFilter); + // [#20122] …and the two doors `having` took at its own entry + // (#20099), so a refusal here is the FILTER's, never the data's: + // 1. the comparand-TYPE door `where` takes in + // `lowerWhereFilterArray`, rooted at this position — a plain + // object, a `Map` or a function comparand was compared as it + // stood and counted no row (every row under `$ne`), and a + // `Symbol` under an ordering operator threw an uncoded + // `TypeError`, on a populated table only. An exact-range + // bigint is narrowed copy-on-write, as it is there; + // 2. the walker's own refusals, judged once + // (`assertAggregationFilterIsEvaluable`). The fallback below + // walks the filter per SOURCE row, so `{ amount: { $median: 1 } }` + // was a 400 on a populated table and a `200 []` on an empty one, + // and a `$or` whose first branch held counted every row. + // ⛔ Not the condition-object check `having` got: a `filter` that is + // not a filter node at all (a string, a number, a `Map`) is a + // question about the clause's SHAPE, answered by the shape door + // for every filter position at once — not a refusal that depends + // on the data, which is what this block closes. + const typed = normalizeFilterComparandTypes(aggFilter, `aggregate('${object}')`, `aggregations[${i}].filter`); + assertAggregationFilterIsEvaluable(typed, i); + if (typed !== aggFilter) { + typedAggregations ??= [...aggregations]; + typedAggregations[i] = { ...(agg as object), filter: typed } as typeof agg; + } + } + if (typedAggregations) query = { ...query, aggregations: typedAggregations }; } // [#19974] `having` is this verb's THIRD filter position, and it walks // through the same comparand-shape face the other two take above — diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index 7d9f8ace64..af5dd5d3d2 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -80,6 +80,13 @@ // comparand of the six scalar comparisons — the one position the Filter // Protocol declares for it (`FieldReferenceSchema`) and the one the // `{ $field }` `$between` refusal prescribes. See {@link compareWithReference}. +// +// [#20122] …and the per-aggregation `filter` takes the same row-independent +// walk. It shares this walker (`matchesAggregationFilter`), so its refusals +// depended on the rows exactly as `having`'s did: `{ amount: { $median: 1 } }` +// was a 400 on a populated table and a 200 on an empty one. The engine now +// judges each `aggregations[i].filter` once, before any driver is asked for a +// row — see {@link assertAggregationFilterIsEvaluable}. import type { FilterCondition } from '@objectstack/spec/data'; // [#20099] The reference's own declaration, so a malformed `addDays` is refused @@ -495,25 +502,69 @@ export function assertHavingIsFilterCondition(having: unknown): void { * {@link assertHavingIsFilterCondition}. */ export function assertHavingIsEvaluable(having: unknown, columns: readonly string[]): void { - assertNodeIsEvaluable(having, 'having', columns); + assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns }); +} + +/** + * [#20122] Judge one `aggregations[i].filter` ONCE, independent of the rows — + * the same walk {@link assertHavingIsEvaluable} takes, in the words the + * per-row walk ({@link matchesAggregationFilter}) uses for that position. + * + * The per-aggregation filter is evaluated by the in-memory fallback, per SOURCE + * row of each bucket, so every refusal the walker raises used to depend on the + * rows: an empty table never reached it (`200 []`, or `[{ n: 0 }]` without a + * `groupBy`), a `$or` whose first branch held never reached its second (the + * UNFILTERED count), and a column the row does not carry left the walk at the + * no-value exit before the operator was read (a count of zero). The same + * filter was a 400 on a populated table. + * + * `ObjectQL.aggregate` calls this in its per-aggregation loop, after the + * shared doors `where` also takes there and the comparand-TYPE door, and + * before `getDriver`, the middleware chain and the fallback that evaluates the + * filter. What it refuses is exactly {@link assertHavingIsEvaluable}'s list + * minus the name check: the filter reads the object's RAW columns, whose names + * the engine does not judge on `where` either (its registry-less tolerance — + * see `assertFilterIsMaterializable`), so a `{ $field }` here is judged for its + * position and its declaration, never for its name. + * + * Read-only. + */ +export function assertAggregationFilterIsEvaluable(filter: unknown, index: number): void { + const clause = aggregationFilterClause(index); + assertNodeIsEvaluable(filter, clause.root, { clause }); +} + +/** + * [#20122] What one row-independent walk judges against: the clause whose + * words its refusals use, and — where the position has a closed namespace — + * the column set a `{ $field }` reference must name. + */ +interface EvaluableScope { + clause: FilterClause; + /** + * The names a `{ $field }` reference may take. `undefined` when the position + * has no closed column set to judge a name against (the per-aggregation + * filter reads the object's raw columns). + */ + columns?: readonly string[]; } /** One node: the `$and` / `$or` / `$not` walk {@link matchesHaving} takes. */ -function assertNodeIsEvaluable(cond: unknown, path: string, columns: readonly string[]): void { +function assertNodeIsEvaluable(cond: unknown, path: string, scope: EvaluableScope): void { if (!cond || typeof cond !== 'object') return; for (const [key, value] of Object.entries(cond)) { const here = `${path}.${key}`; if (key === '$and' || key === '$or') { const branches = Array.isArray(value) ? value : [value]; - branches.forEach((c, i) => assertNodeIsEvaluable(c, `${here}[${i}]`, columns)); + branches.forEach((c, i) => assertNodeIsEvaluable(c, `${here}[${i}]`, scope)); continue; } if (key === '$not') { - assertNodeIsEvaluable(value, here, columns); + assertNodeIsEvaluable(value, here, scope); continue; } - if (key.startsWith('$')) throw unknownOperator(key, 'logical'); - assertConditionIsEvaluable(value, key, here, columns); + if (key.startsWith('$')) throw unknownOperator(key, 'logical', [], scope.clause); + assertConditionIsEvaluable(value, key, here, scope); } } @@ -522,7 +573,7 @@ function assertConditionIsEvaluable( condition: unknown, field: string, path: string, - columns: readonly string[], + scope: EvaluableScope, ): void { // Implicit equality with a literal: nothing to refuse here. An array in this // slot is the shared comparand-shape face's, and it has already run. @@ -542,10 +593,10 @@ function assertConditionIsEvaluable( throw icontainsComparandError(field, target, `${path}.${op}`); } if (!(CONDITION_OPERATORS as readonly string[]).includes(op)) { - throw unknownOperator(op, 'condition', keys); + throw unknownOperator(op, 'condition', keys, scope.clause); } if (REFERENCE_COMPARISON_OPERATORS.has(op)) { - if (isFieldReferenceShape(target)) assertReferenceResolves(target, `${path}.${op}`, columns); + if (isFieldReferenceShape(target)) assertReferenceResolves(target, `${path}.${op}`, scope.columns); continue; } if (Array.isArray(target)) { @@ -561,16 +612,20 @@ function assertConditionIsEvaluable( * [#20099] A reference in a scalar comparison: well-formed by its own * declaration, and naming columns the aggregated row has — the `$field`, and an * `addDays` offset's nested `$field` when it carries one. + * + * [#20122] `columns` is absent for a position with no closed column set (the + * per-aggregation filter); there only the declaration is judged. */ function assertReferenceResolves( reference: Record, path: string, - columns: readonly string[], + columns: readonly string[] | undefined, ): void { const parsed = FieldReferenceSchema.safeParse(reference); if (!parsed.success) { throw malformedFieldReferenceError(path, parsed.error.issues[0]?.message ?? 'it does not parse'); } + if (!columns) return; const names = [reference.$field]; if (isFieldReferenceShape(reference.addDays)) names.push(reference.addDays.$field); for (const name of names) { From 71e3ae20923804eb528f06ffb6c4f7fc2a35e988 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 11:42:17 +0000 Subject: [PATCH 2/6] fix(objectql)!: a `having` key naming no column of the aggregated row is refused (#20123) The aggregated row carries exactly the columns the query projects, so a `having` key outside them read "no value" in every group: a typo for an alias (`{ totl: { $gt: 100 } }`) kept no group, and a negated test on it kept every group, with no error either way. `assertHavingIsEvaluable` now judges every key, at any depth under `$and` / `$or` / `$not`, against the same `aggregatedRowColumns` set the `{ $field }` reference is judged against, once the rest of the clause has passed, and refuses the unknown ones together (INVALID_FILTER / 400), naming each, the first with its position, and listing the columns. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- .../20123-having-unknown-column-refused.md | 28 +++++++ ...e-aggregate-having-comparand-shape.test.ts | 83 +++++++++++++++++++ packages/objectql/src/engine.ts | 5 +- packages/objectql/src/having-filter.ts | 63 ++++++++++++-- 4 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 .changeset/20123-having-unknown-column-refused.md diff --git a/.changeset/20123-having-unknown-column-refused.md b/.changeset/20123-having-unknown-column-refused.md new file mode 100644 index 0000000000..25a54bdf15 --- /dev/null +++ b/.changeset/20123-having-unknown-column-refused.md @@ -0,0 +1,28 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql)!: a `having` key that names no column of the aggregated row is refused on `engine.aggregate`, instead of answering as if that column had no value in every group (#20123) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what `having` accepts on `engine.aggregate`, and on the REST aggregate query (`POST /data/:object/query`) that forwards it there. A `having` key — at any depth under `$and` / `$or` / `$not` — must name a column of the aggregated row: a groupBy projection (the field name, or a structured item's `alias`) or an aggregation alias. Any other key is refused with `INVALID_FILTER` / 400, once per query, before any driver is asked for a row, on both the native `driver.aggregate()` path and the in-memory fallback, whether or not any group exists. It ships as `minor` under the launch-window convention for accept-set narrowings. + +The engine evaluates `having` itself, per aggregated row, and read a key the row does not carry as a column with no value. So a typo for an alias answered like a real query. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, both paths, and through `POST /data/:object/query` on both, over three groups by `customer_id`, each with a positive `total` (a `sum` alias) beside a `count` alias `n`: + +| `having` | before | now | +|:--|:--|:--| +| `{ totl: { $gt: 100 } }`, `{ totl: 500 }`, or `{ amount: { $gt: 100 } }` (a source column the aggregated row does not project) | no group, no error | refused, naming the key, its position and the query's columns | +| `{ totl: { $ne: 1 } }`, `{ totl: { $exists: false } }`, or `{ $not: { totl: { $gt: 100 } } }` | EVERY group, no error | refused | +| `{ $or: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }` | every group whose `total` is positive: the walk stopped at the branch that held | refused | +| `{ $and: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }` | no group | refused | +| `{ 'customer_id.name': 'c1' }` (a dotted path) | no group | refused | +| `{ customer_id: 'c1' }` when the groupBy item is `{ field: 'customer_id', alias: 'cust' }` | no group: the row projects `cust` | refused; `{ cust: 'c1' }` answers | + +The refusal opens the way the REST ingress's refusal of an unknown `where` field does ("filters on 'totl' … which is not a column of the aggregated row"), names every unknown key, and lists the aggregated row's columns. Its code is `INVALID_FILTER`, the code of every other `having` refusal: the name is a column of the query's own projection, not a field of the object. It is judged after the rest of the clause: a condition on an unknown column that also carries an unknown operator (`{ nope: { $median: 1 } }`) is still refused for its operator first, as before. + +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 names an aggregation alias of its own query (`{ order_count: { $gt: 5 } }` and the like), which answers exactly as before. Callers of `engine.aggregate` and of the REST aggregate query in a deployment were NOT measured. + +Not changed: a key naming a groupBy column, a structured item's alias, a `count` / `sum` / `max` alias, or any of those under `$and` / `$or` / `$not`, answers exactly as before on both paths, measured identical before and after. diff --git a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts index 62bfb2cc6e..af98776758 100644 --- a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts +++ b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts @@ -667,3 +667,86 @@ describe('[#20099] having — a { $field } reference resolves against the aggreg } }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#20123] A `having` key names a column of the aggregated row +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#20123] having — a key naming no column of the aggregated row is refused, whatever the rows', () => { + // `AGG_QUERY` projects exactly three columns. + const COLUMNS = ['customer_id', 'order_count', 'total']; + + // Before, measured through `engine.aggregate` on driver-memory and + // driver-sql, both doors, and through `POST /data/:object/query`: no row + // raised anything. A test for a value kept no group; a test for absence or + // a negation — and a `$or` whose first branch held — kept EVERY group. + const UNKNOWN_KEY: ReadonlyArray Record, string, string]> = [ + ['a typo for an alias under $gt (the triage shape)', () => ({ totl: { $gt: 100 } }), 'totl', 'having.totl'], + ['a typo in the implicit-equality slot', () => ({ totl: 500 }), 'totl', 'having.totl'], + ['a typo under $ne', () => ({ totl: { $ne: 1 } }), 'totl', 'having.totl'], + ['a typo under $exists: false', () => ({ totl: { $exists: false } }), 'totl', 'having.totl'], + ['a typo nested in $and', () => ({ $and: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }), 'totl', 'having.$and[1].totl'], + ['a typo behind a $or branch that already held', () => ({ $or: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }), 'totl', 'having.$or[1].totl'], + ['a typo under $not', () => ({ $not: { totl: { $gt: 100 } } }), 'totl', 'having.$not.totl'], + ['a SOURCE column the aggregated row does not project', () => ({ amount: { $gt: 100 } }), 'amount', 'having.amount'], + ['a dotted path', () => ({ 'customer_id.name': 'c1' }), 'customer_id.name', 'having.customer_id.name'], + ['a key that also carries a { $field } reference', () => ({ totl: { $gt: { $field: 'total' } } }), 'totl', 'having.totl'], + ]; + + for (const [name, having, key, path] of UNKNOWN_KEY) { + it(`${name}: refused at ${path}, naming the columns`, async () => { + const message = await expectRowIndependentRefusal(offContract({ ...AGG_QUERY, having: having() }), ROWS); + expect(message).toContain(`'${key}' at ${path}`); + for (const column of COLUMNS) expect(message).toContain(column); + }); + } + + it('every unknown key is named — the first with its position, the rest after it', async () => { + const message = await expectRowIndependentRefusal( + offContract({ ...AGG_QUERY, having: { $and: [{ totl: 1 }, { cnt: { $gt: 1 } }, { totl: 2 }] } }), ROWS); + expect(message).toContain("'totl' at having.$and[0].totl"); + expect(message).toContain('(also: cnt)'); + }); + + it('an operator refusal on an unknown column is still the operator\'s — the key is judged last', async () => { + // The #20099 rule: a condition on a column the row does not carry is read + // for its operator. Held here so the new check cannot jump ahead of it. + const floor = syncRefusalOf(() => applyHaving([{ nope: 1 }], { nope: { $median: 1 } } as FilterCondition)); + const message = await expectRowIndependentRefusal(offContract({ ...AGG_QUERY, having: { nope: { $median: 1 } } }), ROWS); + expect(message).toBe(floor!.message); + }); + + it('the source name of an ALIASED groupBy projection is not a column — its alias is', async () => { + // Before: `{ customer_id: 'c1' }` kept no group — the row projects `cust`. + const query = (having: FilterCondition): EngineAggregateOptions => ({ + groupBy: [{ field: 'customer_id', alias: 'cust' }], + aggregations: [{ function: 'count', alias: 'order_count' }], + having, + }); + const message = await expectRowIndependentRefusal(query({ customer_id: 'c1' }), ROWS); + expect(message).toContain("'customer_id' at having.customer_id"); + expect(message).toContain('cust, order_count'); + // The alias itself answers, on the door that projects it (the stand-in + // native driver groups by `customer_id` and ignores the alias). + const { engine } = await makeEngine(ROWS, false); + const rows = await engine.aggregate(OBJECT, query({ cust: 'c1' })); + expect(rows).toEqual([{ cust: 'c1', order_count: 2 }]); + }); + + // Every key names a column: answered exactly as before, both doors. + const PASSING: ReadonlyArray = [ + ['a groupBy column', { customer_id: 'c1' }, ['c1']], + ['an aggregation alias', { total: { $gt: 100 } }, ['c1', 'c2']], + ['a count alias', { order_count: { $gte: 2 } }, ['c1', 'c2']], + ['columns nested under $or / $and / $not', { $or: [{ total: { $gt: 1000 } }, { $and: [{ $not: { customer_id: 'c1' } }, { order_count: 1 }] }] }, ['c2', 'c3']], + ['a negation of a column', { total: { $ne: 500 } }, ['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); + expect(groups(await engine.aggregate(OBJECT, { ...AGG_QUERY, having })), door).toEqual([...expected]); + } + }); + } +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 2594a03a37..2cc48c49df 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -16070,7 +16070,10 @@ export class ObjectQL implements IObjectQLEngine { // position and name, judged against the aggregated row's column set // read off THIS query (`assertHavingIsEvaluable`). The walker raised // them per aggregated row, so an empty grouped set answered `200 []` - // for a `having` a populated one refused. + // for a `having` a populated one refused. [#20123] The same column + // set judges every KEY too: a key naming no column read "no value" + // in every group, so a typo for an alias kept no group (every group + // under a negation) with no error. // A reference that passes is then RESOLVED against each aggregated row // (having-filter.ts `compareWithReference`) on both doors below. assertHavingIsFilterCondition(query.having); diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index af5dd5d3d2..7a26470a4d 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -414,6 +414,43 @@ function malformedFieldReferenceError(path: string, detail: string): Error { return invalidFilterError(`The { "$field" } reference at ${path} is malformed: ${detail}`); } +/** + * [#20123] A `having` KEY that names no column of the aggregated row — the + * twin of {@link unresolvedFieldReferenceError} for the other place a column + * is named. + * + * Refused on the filter, never answered per row: {@link checkCondition} reads + * such a key as a column with NO VALUE in every group, so a typo for an alias + * (`{ totl: { $gt: 100 } }`) kept no group and a negated test on it + * (`$ne`, `$exists: false`, a `$not`) kept every group — an answer + * indistinguishable from a real one. It is the same mistake the REST + * ingress refuses on `where` for a field the object does not have, and the + * opening words follow that refusal's ("filters on 'X', which is not a …"; + * "the query was refused instead of answered"). The code stays this face's + * own `INVALID_FILTER`: the name is a column of the QUERY's own projection, + * not a field of the object, which is what `INVALID_FIELD` answers about. + * + * Every unknown key is named, the first with its position, and the column + * list is printed because it is the author's own query, and it is the answer. + */ +function unknownHavingColumnError( + unknown: ReadonlyArray<{ key: string; path: string }>, + columns: readonly string[], +): Error { + const [first] = unknown; + const others = [...new Set(unknown.map((u) => u.key))].filter((key) => key !== first.key); + const also = others.length ? ` (also: ${others.join(', ')})` : ''; + const known = columns.length ? columns.join(', ') : '(none — the query projects no column)'; + return invalidFilterError( + `\`having\` filters on '${first.key}' at ${first.path}, which is not a column of the aggregated ` + + `row${also}. A condition on a column the row does not carry is answered as if that column had no ` + + `value in every group — a test for a value keeps no group, and a test for its absence or a ` + + `negation keeps every group — so the query was refused instead of answered. \`having\` filters ` + + `the aggregated row's own columns — the groupBy projections and the aggregation aliases — which ` + + `here are: ${known}.`, + ); +} + /** * [#5905] Operators whose answer for a column with NO VALUE is decided by the * operator's own arm below, not by the early exit in {@link checkCondition}. @@ -496,13 +533,22 @@ export function assertHavingIsFilterCondition(having: unknown): void { * - a reference outside the six scalar comparisons — * {@link fieldReferencePositionError}; * - a reference its declaration refuses (a malformed `addDays`), or one naming - * no column of the aggregated row — {@link unresolvedFieldReferenceError}. + * no column of the aggregated row — {@link unresolvedFieldReferenceError}; + * - [#20123] and, once the whole clause has passed those, a KEY naming no + * column of the aggregated row, at any depth — + * {@link unknownHavingColumnError}. Last on purpose: a condition on a column + * the row does not carry is still read for its operator first (the #20099 + * rule above), so the refusal an author meets for `{ nope: { $median: 1 } }` + * is the operator's, and fixing it does not reveal a second one they could + * have been told about already. * * Read-only; `having` is assumed to have passed * {@link assertHavingIsFilterCondition}. */ export function assertHavingIsEvaluable(having: unknown, columns: readonly string[]): void { - assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns }); + const unknownKeys: Array<{ key: string; path: string }> = []; + assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns, unknownKeys }); + if (unknownKeys.length > 0) throw unknownHavingColumnError(unknownKeys, columns); } /** @@ -537,16 +583,18 @@ export function assertAggregationFilterIsEvaluable(filter: unknown, index: numbe /** * [#20122] What one row-independent walk judges against: the clause whose * words its refusals use, and — where the position has a closed namespace — - * the column set a `{ $field }` reference must name. + * the column set a key and a `{ $field }` reference must name. */ interface EvaluableScope { clause: FilterClause; /** - * The names a `{ $field }` reference may take. `undefined` when the position - * has no closed column set to judge a name against (the per-aggregation - * filter reads the object's raw columns). + * The names a key and a `{ $field }` reference may take. `undefined` when + * the position has no closed column set to judge a name against (the + * per-aggregation filter reads the object's raw columns). */ columns?: readonly string[]; + /** [#20123] Keys naming none of `columns`, collected in walk order. */ + unknownKeys?: Array<{ key: string; path: string }>; } /** One node: the `$and` / `$or` / `$not` walk {@link matchesHaving} takes. */ @@ -565,6 +613,9 @@ function assertNodeIsEvaluable(cond: unknown, path: string, scope: EvaluableScop } if (key.startsWith('$')) throw unknownOperator(key, 'logical', [], scope.clause); assertConditionIsEvaluable(value, key, here, scope); + // [#20123] Rows carry exactly the columns the query projects, so a key + // outside them can only ever read "no value" — see unknownHavingColumnError. + if (scope.columns && !scope.columns.includes(key)) scope.unknownKeys?.push({ key, path: here }); } } From 1690b79b5963f3359d97259cb0460a20333becb1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 11:47:16 +0000 Subject: [PATCH 3/6] fix(objectql)!: `having` evaluates `addDays` only between two temporal columns of one class (#20127) `FieldReferenceSchema.addDays` declares that the offset applies between two temporal columns of the same class (date/date, datetime/datetime), read from an integer or a numeric column, and `driver-sql` refuses every other pair on `where`. `having` resolved every pair through the in-memory evaluator, which reads a number as epoch milliseconds, because an aggregated row declared no class for its columns. `aggregatedRowColumnClasses` now reads each column's class statically: a groupBy projection takes its field's declared type (a `day` bucket is a date, a coarser bucket a text label), count / count_distinct / sum / avg are numeric, and min / max take the type of the field they read. The engine hands it, with the object's field map, to `assertHavingIsEvaluable`, which refuses a mismatched pair in driver-sql's own sentences, once per query. A class the declaration cannot tell is not judged. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- .../20127-having-adddays-temporal-pair.md | 34 ++++ ...e-aggregate-having-comparand-shape.test.ts | 114 +++++++++++ packages/objectql/src/engine.ts | 15 +- packages/objectql/src/having-filter.ts | 187 +++++++++++++++++- 4 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 .changeset/20127-having-adddays-temporal-pair.md diff --git a/.changeset/20127-having-adddays-temporal-pair.md b/.changeset/20127-having-adddays-temporal-pair.md new file mode 100644 index 0000000000..80f2268abb --- /dev/null +++ b/.changeset/20127-having-adddays-temporal-pair.md @@ -0,0 +1,34 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql)!: in `engine.aggregate({ having })`, a `{ $field, addDays }` reference is evaluated only between two temporal columns of one class, with a numeric offset column, as `FieldReferenceSchema.addDays` declares, instead of answering by epoch-millisecond coercion (#20127) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what `having` accepts on `engine.aggregate`, and on the REST aggregate query (`POST /data/:object/query`) that forwards it there. A `{ $field }` reference that carries `addDays` in one of the six scalar comparisons is now refused with `INVALID_FILTER` / 400 unless the column it filters and the column it references are both `date` or both `datetime`, and an `addDays` offset read from a column reads a numeric one. The refusal is raised once per query, before any driver is asked for a row, on both the native `driver.aggregate()` path and the in-memory fallback, whether or not any group exists. It ships as `minor` under the launch-window convention for accept-set narrowings. + +An aggregated row has no declared field types, so each column's class is now read off the query and the object's declaration, before any row exists: + +- a groupBy projection takes its field's declared type. A `day` date bucket is a `date`, because its label is `YYYY-MM-DD` on every face. A `week`, `month`, `quarter` or `year` bucket is a text label; +- `count`, `count_distinct`, `sum` and `avg` are numeric; +- `min` and `max` take the type of the field they read. + +A column whose class the declaration cannot tell is not judged: an object with no field map, a field it does not declare, or a `formula` field. + +`having` resolved every pair through `@objectstack/formula`'s evaluator, which reads a number as epoch milliseconds, while `driver-sql` refuses the same pair on `where`. The refusal reuses `driver-sql`'s sentence for that pair, with "is stored as" read as "is", because an aggregated column is computed rather than stored. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, both paths, and through `POST /data/:object/query` on both, over three groups with a `sum` alias `total`, a `max` of a number `max_cap`, `max` / `min` of two `date` fields, `max` / `min` of two `datetime` fields and a `count` `n`: + +| `having` | before | now | +|:--|:--|:--| +| `{ total: { $gt: { $field: 'max_cap', addDays: 1 } } }` (two numeric columns) | no group, no error | refused: "addDays adds whole days to a date or datetime column, and "max_cap" is numeric — an offset has no meaning on it." | +| `{ n: { $gte: { $field: 'n', addDays: 0 } } }` (a count against itself) | every group | refused, in the same words | +| a `date` column against a numeric column, a numeric column against a `date` one, or the `customer_id` groupBy text column against a `date` one | no group | refused, naming both columns and their classes: "… and a cross-class comparison answers differently in SQL (storage-class ordering) than in memory (JS coercion) — compare same-class columns." | +| a `date` column against a `datetime` one | one group | refused, in the cross-class words | +| a `datetime` column against a `date` one | two groups | refused, in the cross-class words | +| a `date` pair whose `addDays` reads a text column or a `date` column | no group | refused: "the addDays offset … is not a numeric column, and a day offset must be a number of days." | + +Who is affected: `having` is a request-only key (`QuerySchema.having`, `EngineAggregateOptions.having`), and no metadata type stores it. No `having` in this repository's docs and published skills carries a `{ $field }` reference. Callers of `engine.aggregate` and of the REST aggregate query in a deployment were NOT measured. + +Not changed, measured identical before and after on both paths: a `date` / `date` pair and a `datetime` / `datetime` pair, with a positive or negative whole-day literal or with an offset read from a numeric column (`max` of a number, or a `count`); a `day` date bucket against a `date` column; and any `{ $field }` comparison WITHOUT `addDays`, including a numeric pair and a numeric column against a `date` one. A per-aggregation `filter` (`aggregations[i].filter`) is not judged by this rule: it reads the object's raw columns, and this change classifies only the aggregated row's. diff --git a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts index af98776758..9b96dad2d6 100644 --- a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts +++ b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts @@ -180,6 +180,10 @@ async function makeEngine(rows: ReadonlyArray>, native: placed_on: { type: 'date' }, due_on: { type: 'date' }, grace: { type: 'number' }, + // [#20127] Declared types are what an aggregated column's class is read + // from, so the `addDays` pairs need a datetime class beside the date one. + opened_at: { type: 'datetime' }, + closed_at: { type: 'datetime' }, }, } as any); return { engine, calls }; @@ -750,3 +754,113 @@ describe('[#20123] having — a key naming no column of the aggregated row is re }); } }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#20127] `addDays` between two temporal columns of one class +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#20127] having — a { $field, addDays } pair is judged by each aggregated column\'s class, whatever the rows', () => { + const OPENED = ['2026-01-01T10:00:00.000Z', '2026-01-02T10:00:00.000Z', '2026-02-01T10:00:00.000Z', '2026-02-05T10:00:00.000Z', '2026-02-06T10:00:00.000Z', '2026-03-01T10:00:00.000Z']; + const CLOSED = ['2026-01-03T10:00:00.000Z', '2026-01-02T12:00:00.000Z', '2026-02-10T10:00:00.000Z', '2026-02-05T11:00:00.000Z', '2026-02-06T10:00:00.000Z', '2026-03-01T10:00:00.000Z']; + const DT_ROWS = [ + { customer_id: 'c1', amount: 100, cap: 50, placed_on: '2026-01-10', due_on: '2026-01-05', grace: 3 }, + { customer_id: 'c1', amount: 400, cap: 10, placed_on: '2026-01-02', due_on: '2026-01-20', grace: 10 }, + { customer_id: 'c2', amount: 900, cap: 5000, placed_on: '2026-03-01', due_on: '2026-01-01', grace: 1 }, + { customer_id: 'c2', amount: 300, cap: 1, placed_on: '2026-02-01', due_on: '2026-02-01', grace: 1 }, + { customer_id: 'c2', amount: 50, cap: 2, placed_on: '2026-01-15', due_on: '2026-03-01', grace: 1 }, + { customer_id: 'c3', amount: 20, cap: 20, placed_on: '2026-02-01', due_on: '2026-01-31', grace: 0 }, + ].map((row, i) => ({ ...row, opened_at: OPENED[i], closed_at: CLOSED[i] })); + // Grouped: total max_cap last_placed first_due max_grace first_opened last_closed + // c1 500 50 2026-01-10 2026-01-05 10 2026-01-01T10:00Z 2026-01-03T10:00Z + // c2 1250 5000 2026-03-01 2026-01-01 1 2026-02-01T10:00Z 2026-02-10T10:00Z + // c3 20 20 2026-02-01 2026-01-31 0 2026-03-01T10:00Z 2026-03-01T10:00Z + const DT_QUERY: EngineAggregateOptions = { + groupBy: ['customer_id'], + aggregations: [ + { function: 'count', alias: 'order_count' }, + { function: 'sum', field: 'amount', alias: 'total' }, + { function: 'max', field: 'cap', alias: 'max_cap' }, + { function: 'max', field: 'placed_on', alias: 'last_placed' }, + { function: 'min', field: 'due_on', alias: 'first_due' }, + { function: 'max', field: 'grace', alias: 'max_grace' }, + { function: 'min', field: 'opened_at', alias: 'first_opened' }, + { function: 'max', field: 'closed_at', alias: 'last_closed' }, + ], + }; + + // Before, measured through `engine.aggregate` on driver-memory and + // driver-sql, both doors, and through `POST /data/:object/query`: every row + // ANSWERED, by `@objectstack/formula`'s reading of the pair (a number read + // as epoch milliseconds, a day added to it), where `driver-sql` refuses the + // same pair on `where`. Each fragment is `driver-sql`'s own sentence. + const REFUSED: ReadonlyArray Record, string]> = [ + ['two numeric columns (the triage shape)', () => ({ total: { $gt: { $field: 'max_cap', addDays: 1 } } }), 'addDays adds whole days to a date or datetime column, and "max_cap" is numeric'], + ['a count against itself', () => ({ order_count: { $gte: { $field: 'order_count', addDays: 0 } } }), '"order_count" is numeric — an offset has no meaning on it'], + ['a date target against a numeric referent', () => ({ last_placed: { $lte: { $field: 'max_cap', addDays: 1 } } }), '"last_placed" is date but "max_cap" is numeric'], + ['a numeric target against a date referent', () => ({ total: { $gt: { $field: 'first_due', addDays: 1 } } }), '"total" is numeric but "first_due" is date'], + ['a date against a datetime', () => ({ last_placed: { $lte: { $field: 'last_closed', addDays: 1 } } }), '"last_placed" is date but "last_closed" is datetime'], + ['a datetime against a date', () => ({ last_closed: { $gte: { $field: 'first_due', addDays: 1 } } }), '"last_closed" is datetime but "first_due" is date'], + ['a groupBy text column against a date', () => ({ customer_id: { $lte: { $field: 'first_due', addDays: 1 } } }), '"customer_id" is text but "first_due" is date'], + ['a text offset column', () => ({ last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'customer_id' } } } }), 'the addDays offset "customer_id" (text) is not a numeric column'], + ['a date offset column', () => ({ last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'last_placed' } } } }), 'the addDays offset "last_placed" (date) is not a numeric column'], + ['a numeric pair nested under $not in a $or', () => ({ $or: [{ total: { $gt: 0 } }, { $not: { total: { $gt: { $field: 'max_cap', addDays: 1 } } } }] }), '"max_cap" is numeric'], + ]; + + for (const [name, having, fragment] of REFUSED) { + it(`${name}: refused in driver-sql's words`, async () => { + const message = await expectRowIndependentRefusal(offContract({ ...DT_QUERY, having: having() }), DT_ROWS); + expect(message).toContain(fragment); + }); + } + + // Answered exactly as before, on both doors: the pairs the declaration admits. + const ANSWERED: ReadonlyArray = [ + ['date / date with a positive literal', { last_placed: { $lte: { $field: 'first_due', addDays: 7 } } }, ['c1', 'c3']], + ['date / date with a negative literal', { last_placed: { $gte: { $field: 'first_due', addDays: -3 } } }, ['c1', 'c2', 'c3']], + ['date / date with a numeric offset column (a max)', { last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'max_grace' } } } }, ['c1']], + ['date / date with a numeric offset column (a count)', { last_placed: { $lte: { $field: 'first_due', addDays: { $field: 'order_count' } } } }, ['c3']], + ['datetime / datetime', { last_closed: { $gte: { $field: 'first_opened', addDays: 1 } } }, ['c1', 'c2']], + ['a numeric pair with NO addDays (the rule is the offset\'s)', { total: { $gt: { $field: 'max_cap' } } }, ['c1']], + ]; + + for (const [name, having, expected] of ANSWERED) { + it(`${name} answers ${JSON.stringify(expected)} on both doors`, async () => { + for (const [door, native] of DOORS) { + const { engine } = await makeEngine(DT_ROWS, native); + expect(groups(await engine.aggregate(OBJECT, { ...DT_QUERY, having })), door).toEqual([...expected]); + } + }); + } + + it('a "day" date bucket is a date column; a coarser bucket is a text label', async () => { + const bucketed = (granularity: 'day' | 'month', having: FilterCondition): EngineAggregateOptions => ({ + groupBy: [{ field: 'placed_on', dateGranularity: granularity, alias: 'placed' }], + aggregations: [{ function: 'min', field: 'due_on', alias: 'first_due' }], + having, + }); + const shifted = { placed: { $lte: { $field: 'first_due', addDays: 1 } } }; + for (const [door, native] of DOORS) { + const { engine } = await makeEngine(DT_ROWS, native); + const rows = await engine.aggregate(OBJECT, bucketed('day', shifted)); + expect(rows.map((r: any) => r.placed).sort(), door).toEqual(['2026-01-02', '2026-01-15', '2026-02-01']); + } + const message = await expectRowIndependentRefusal(bucketed('month', shifted), DT_ROWS); + expect(message).toContain('"placed" is text but "first_due" is date'); + }); + + it('a column whose class the declaration cannot tell is not judged — the pair is answered as before', async () => { + // `ghost` is declared nowhere, so `min(ghost)` has no class to judge. The + // engine keeps the fail-open direction its other declared-type doors take + // for what the declaration cannot see; the reference reads no value, so + // the ordering is false in every group. + const query = offContract({ + ...DT_QUERY, + aggregations: [...DT_QUERY.aggregations!, { function: 'min', field: 'ghost', alias: 'ghost_min' }], + having: { first_due: { $gte: { $field: 'ghost_min', addDays: 1 } } }, + }); + for (const [door, native] of DOORS) { + const { engine } = await makeEngine(DT_ROWS, native); + expect(groups(await engine.aggregate(OBJECT, query)), door).toEqual([]); + } + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 2cc48c49df..dc804d1243 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -259,6 +259,7 @@ import { import { applyHaving, aggregatedRowColumns, + aggregatedRowColumnClasses, assertAggregationFilterIsEvaluable, assertHavingIsEvaluable, assertHavingIsFilterCondition, @@ -16080,7 +16081,19 @@ export class ObjectQL implements IObjectQLEngine { assertListComparandShapes(object, 'aggregate', query.having, 'having'); { const having = normalizeFilterComparandTypes(query.having, `aggregate('${object}')`, 'having'); - assertHavingIsEvaluable(having, aggregatedRowColumns(query.groupBy, query.aggregations)); + // [#20127] …and each column's class, read off the query and the + // object's declaration, so a `{ $field, addDays }` pair is judged by + // the rule `FieldReferenceSchema.addDays` declares (two temporal + // columns of one class) rather than answered by epoch-ms coercion. + assertHavingIsEvaluable( + having, + aggregatedRowColumns(query.groupBy, query.aggregations), + aggregatedRowColumnClasses( + query.groupBy, + query.aggregations, + (this._registry.getObject(object) as { fields?: Record } | undefined)?.fields, + ), + ); if (having !== query.having) query = { ...query, having }; } const driver = this.getDriver(object); diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index 7a26470a4d..876293a1e9 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -87,6 +87,12 @@ // was a 400 on a populated table and a 200 on an empty one. The engine now // judges each `aggregations[i].filter` once, before any driver is asked for a // row — see {@link assertAggregationFilterIsEvaluable}. +// +// [#20127] In `having`, a reference carrying `addDays` is judged by the rule +// `FieldReferenceSchema.addDays` declares — "between two temporal columns of +// the same class (date/date, datetime/datetime)", the offset column numeric — +// against each aggregated column's class, read statically off the query and +// the object's declaration. See {@link aggregatedRowColumnClasses}. import type { FilterCondition } from '@objectstack/spec/data'; // [#20099] The reference's own declaration, so a malformed `addDays` is refused @@ -96,6 +102,16 @@ import { FieldReferenceSchema } from '@objectstack/spec/data'; // held to row for row (`cross-field-conformance-cases.ts`): the NULL totality // and the whole-day `addDays` arithmetic of a reference live there once. import { matchesFilterCondition } from '@objectstack/formula'; +// [#20127] The declared value classes an aggregated column's class is read +// from — the spec's sets, so this face and the SQL compilers classify a field +// type by one list. +import { + BOOLEAN_VALUE_TYPES, + CALENDAR_DATE_TYPES, + CLOCK_TIME_TYPES, + INSTANT_TYPES, + NUMERIC_VALUE_TYPES, +} from '@objectstack/spec/data'; // [#5702] The retired operators and the prescription a refusal prints. HAVING is // the fifth of the five refusal sites `RETIRED_FILTER_OPERATORS`' own doc names, // and reads the table for the same reason the four driver sites do: one @@ -451,6 +467,72 @@ function unknownHavingColumnError( ); } +/** + * [#20127] A `{ $field, addDays }` reference between aggregated columns the + * offset has no meaning on. + * + * `FieldReferenceSchema.addDays` declares where the offset applies: "between + * two temporal columns of the same class (date/date, datetime/datetime)", read + * from an integer or a numeric column. `driver-sql` compiles exactly that on + * `where` and refuses every other pair. `having` evaluated every pair through + * `@objectstack/formula`, which reads a number as epoch milliseconds, so + * `{ total: { $gt: { $field: 'max_cap', addDays: 1 } } }` answered — a day + * added to a sum. The aggregated row now carries a class per column + * ({@link aggregatedRowColumnClasses}), so the pair is judged here, once. + * + * `reason` is `driver-sql`'s own sentence for the same pair on `where` + * (`applyCrossFieldComparison`'s `addDays` arm), with "is stored as" read as + * "is" — an aggregated column is computed, not stored. Unlike `driver-sql`'s, + * this diagnostic is not withheld: every column it names is the author's own + * projection, as in {@link unresolvedFieldReferenceError}. + */ +function offsetPairError(field: string, op: string, ref: string, path: string, reason: string): Error { + return invalidFilterError( + `Operator "${op}" on field "${field}" at ${path} compares against another column ` + + `({ "$field": "${ref}" } with addDays), which cannot be evaluated here: ${reason} An aggregated ` + + `column's class is read off the query: a groupBy projection takes its field's declared type (a ` + + `"day" date bucket is a date, a coarser bucket a text label), count / count_distinct / sum / avg ` + + `are numeric, and min / max take the type of the field they read.`, + ); +} + +/** + * [#20127] The `addDays` pair rule, in `driver-sql`'s order: the two compared + * columns share a class, that class is temporal, and an offset column is + * numeric. A class the declaration cannot tell is not judged. + */ +function assertOffsetPairIsTemporal( + field: string, + op: string, + reference: Record, + path: string, + classes: ReadonlyMap, +): void { + const ref = String(reference.$field); + const refClass = classes.get(ref); + const targetClass = classes.get(field); + if (refClass !== undefined && targetClass !== undefined && refClass !== targetClass) { + throw offsetPairError(field, op, ref, path, + `"${field}" is ${targetClass} but "${ref}" is ${refClass}, and a cross-class comparison answers ` + + `differently in SQL (storage-class ordering) than in memory (JS coercion) — compare same-class ` + + `columns.`); + } + if (refClass !== undefined && refClass !== 'date' && refClass !== 'datetime') { + throw offsetPairError(field, op, ref, path, + `addDays adds whole days to a date or datetime column, and "${ref}" is ${refClass} — an offset ` + + `has no meaning on it.`); + } + const offset = reference.addDays; + if (!isFieldReferenceShape(offset)) return; + const offsetRef = String(offset.$field); + const offsetClass = classes.get(offsetRef); + if (offsetClass !== undefined && offsetClass !== 'numeric') { + throw offsetPairError(field, op, ref, path, + `the addDays offset "${offsetRef}" (${offsetClass}) is not a numeric column, and a day offset ` + + `must be a number of days.`); + } +} + /** * [#5905] Operators whose answer for a column with NO VALUE is decided by the * operator's own arm below, not by the early exit in {@link checkCondition}. @@ -494,6 +576,87 @@ export function aggregatedRowColumns(groupBy: unknown, aggregations: unknown): s return [...columns]; } +/** + * [#20127] An aggregated column's comparison class — `driver-sql`'s + * cross-field vocabulary (`crossFieldComparisonClass`), so the words a `having` + * refusal prints are the words the same pair gets on `where`. + */ +export type AggregatedColumnClass = 'numeric' | 'text' | 'boolean' | 'date' | 'datetime' | 'time'; + +/** The aggregation functions whose result is a number whatever they read. */ +const NUMERIC_RESULT_FUNCTIONS: ReadonlySet = new Set(['count', 'count_distinct', 'sum', 'avg']); + +/** The aggregation functions whose result is one of the values they read. */ +const VALUE_RESULT_FUNCTIONS: ReadonlySet = new Set(['min', 'max']); + +/** + * A declared field's class, by the spec's value-class sets. `undefined` when + * the declaration cannot tell: no field map (a registry-less host), no such + * field, or a `formula`, whose type names no stored value class. + */ +function declaredFieldClass( + fields: Record | undefined, + name: unknown, +): AggregatedColumnClass | undefined { + if (!fields || typeof fields !== 'object' || typeof name !== 'string') return undefined; + if (!Object.prototype.hasOwnProperty.call(fields, name)) return undefined; + const type = (fields[name] as { type?: unknown } | undefined)?.type; + if (typeof type !== 'string' || type === 'formula') return undefined; + if (CALENDAR_DATE_TYPES.has(type)) return 'date'; + if (INSTANT_TYPES.has(type)) return 'datetime'; + if (CLOCK_TIME_TYPES.has(type)) return 'time'; + if (NUMERIC_VALUE_TYPES.has(type)) return 'numeric'; + if (BOOLEAN_VALUE_TYPES.has(type)) return 'boolean'; + // Everything else is read and compared as text, as `driver-sql` stores it. + return 'text'; +} + +/** + * [#20127] Each aggregated column's class, read STATICALLY — off the query and + * the object's field declaration, never off a row — so a verdict that needs it + * is the filter's, the same on an empty grouped set as on a populated one: + * + * - a groupBy projection takes its field's declared class; a `day` date bucket + * is a calendar DATE (its label is `YYYY-MM-DD` on every face — the bucket + * label contract in `@objectstack/core`'s `bucketDateKey`), and a coarser + * bucket (`week` / `month` / `quarter` / `year`) is a text label; + * - `count` / `count_distinct` / `sum` / `avg` are numeric; + * - `min` / `max` take the class of the field they read. + * + * A column whose class the declaration cannot tell maps to `undefined`, and a + * rule reading this map does not judge it — the fail-open direction the + * engine's other declared-type doors take for a registry-less host. + */ +export function aggregatedRowColumnClasses( + groupBy: unknown, + aggregations: unknown, + fields: Record | undefined, +): Map { + const classes = new Map(); + for (const g of Array.isArray(groupBy) ? groupBy : []) { + if (typeof g === 'string') { + classes.set(g, declaredFieldClass(fields, g)); + continue; + } + const item = g as { alias?: unknown; field?: unknown; dateGranularity?: unknown } | null; + const name = item?.alias ?? item?.field; + if (typeof name !== 'string') continue; + const granularity = item?.dateGranularity; + classes.set(name, granularity == null + ? declaredFieldClass(fields, item?.field) + : granularity === 'day' ? 'date' : 'text'); + } + for (const a of Array.isArray(aggregations) ? aggregations : []) { + const agg = a as { alias?: unknown; function?: unknown; field?: unknown } | null; + if (typeof agg?.alias !== 'string') continue; + const fn = String(agg.function); + classes.set(agg.alias, NUMERIC_RESULT_FUNCTIONS.has(fn) + ? 'numeric' + : VALUE_RESULT_FUNCTIONS.has(fn) ? declaredFieldClass(fields, agg.field) : undefined); + } + return classes; +} + /** * [#20099] Refuse a `having` that is not a filter condition at all: anything but * `null` / `undefined` (no clause) and a plain object. See @@ -534,6 +697,10 @@ export function assertHavingIsFilterCondition(having: unknown): void { * {@link fieldReferencePositionError}; * - a reference its declaration refuses (a malformed `addDays`), or one naming * no column of the aggregated row — {@link unresolvedFieldReferenceError}; + * - [#20127] a reference whose `addDays` pairs columns the offset has no + * meaning on — not two temporal columns of one class, or an offset column + * that is not numeric — judged against `classes` when the caller passes it + * ({@link offsetPairError}); * - [#20123] and, once the whole clause has passed those, a KEY naming no * column of the aggregated row, at any depth — * {@link unknownHavingColumnError}. Last on purpose: a condition on a column @@ -545,9 +712,13 @@ export function assertHavingIsFilterCondition(having: unknown): void { * Read-only; `having` is assumed to have passed * {@link assertHavingIsFilterCondition}. */ -export function assertHavingIsEvaluable(having: unknown, columns: readonly string[]): void { +export function assertHavingIsEvaluable( + having: unknown, + columns: readonly string[], + classes?: ReadonlyMap, +): void { const unknownKeys: Array<{ key: string; path: string }> = []; - assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns, unknownKeys }); + assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns, classes, unknownKeys }); if (unknownKeys.length > 0) throw unknownHavingColumnError(unknownKeys, columns); } @@ -593,6 +764,11 @@ interface EvaluableScope { * per-aggregation filter reads the object's raw columns). */ columns?: readonly string[]; + /** + * [#20127] Each column's class, where the query and the declaration tell + * it — what an `addDays` pair is judged against. `undefined` = not judged. + */ + classes?: ReadonlyMap; /** [#20123] Keys naming none of `columns`, collected in walk order. */ unknownKeys?: Array<{ key: string; path: string }>; } @@ -647,7 +823,12 @@ function assertConditionIsEvaluable( throw unknownOperator(op, 'condition', keys, scope.clause); } if (REFERENCE_COMPARISON_OPERATORS.has(op)) { - if (isFieldReferenceShape(target)) assertReferenceResolves(target, `${path}.${op}`, scope.columns); + if (isFieldReferenceShape(target)) { + assertReferenceResolves(target, `${path}.${op}`, scope.columns); + if (scope.classes && target.addDays !== undefined) { + assertOffsetPairIsTemporal(field, op, target, `${path}.${op}`, scope.classes); + } + } continue; } if (Array.isArray(target)) { From a17770aa771da1c534ae97fed835490cad1cc0e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 12:19:02 +0000 Subject: [PATCH 4/6] fix(objectql)!: a per-aggregation filter that is not a filter object is refused (#20122) The seat's amendment to the combined claim folds the per-aggregation filter's shape door into this card. In the `aggregations[i].filter` loop, a filter that is neither absent, `null`, a filter object nor an array (a string, a number, a boolean, `''`, a `Map`, a `Date`) was stepped around by every door there and then dropped: the aggregation read every row of its group, with no error. The loop now opens with the shape gate `where` takes on its own seam, reusing its accept set (`isWhereFilterObject`) and its words (`describeNonFilterWhere`), with a consequence clause for this position, INVALID_FILTER / 400 before any driver call. An array is left to the doors below, as `where` leaves one to its array branch. The note that excluded this gate from the loop is removed. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- ...0122-aggregation-filter-row-independent.md | 7 +-- .../src/engine-aggregate-filter.test.ts | 51 +++++++++++++++++++ packages/objectql/src/engine.ts | 24 +++++++-- 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/.changeset/20122-aggregation-filter-row-independent.md b/.changeset/20122-aggregation-filter-row-independent.md index 3635bf0d77..3f4df77250 100644 --- a/.changeset/20122-aggregation-filter-row-independent.md +++ b/.changeset/20122-aggregation-filter-row-independent.md @@ -2,7 +2,7 @@ "@objectstack/objectql": minor --- -fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine.aggregate` is judged once, before any row is read — its refusals no longer depend on whether the table has rows, and it takes the comparand-type door `where` takes (#20122) +fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine.aggregate` is judged once, before any row is read — its refusals no longer depend on whether the table has rows, and it takes the shape gate and the comparand-type door `where` takes (#20122) Clause-②: no (narrowing) @@ -22,11 +22,12 @@ The engine evaluates a per-aggregation filter itself, in the in-memory fallback, | a `{ $field }` reference whose `addDays` is not an integer (`1.5`, `'7'`) | counted rows by the in-memory evaluator's own reading of that offset, which `FieldReferenceSchema` refuses | a whole-day `addDays` | | `{ amount: { $eq: { v: 1 } } }`, `{ amount: undefined }`, `{ $eq: new Map() }`, a function, an `undefined` `$in` member, a bigint beyond 2^53, or `{ $gt: { $field: 5 } }` | counted no row (each measured under the operator shown, or in the implicit slot). The same comparand in `where` is refused by the comparand-type door; the per-aggregation filter now gets that door's refusal, rooted at its own position | a string, number, bigint, boolean, `null` or `Date` | | a `Symbol` comparand | under `$ne`, counted every row; under `$gt`, threw a raw `TypeError` with no `code` and no `status` on a populated table | a literal of one of the types above | +| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes (`'where' must be a filter object or condition array, received …`), in the same words, naming the aggregation | a filter object, `{ stage: 'won' }` | Not refused, but answering differently: - **An exact-range bigint comparand is narrowed to a number, as it is in `where`.** `{ amount: { $in: [400n, 20n] } }` counted no row, because `[400n].includes(400)` is false. It now counts the rows it names. The caller's aggregation entry is not edited. -Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. Callers in a deployment were NOT measured. +Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. The shape row is reachable in-process only: the REST door already refuses a non-object `filter`. Callers in a deployment were NOT measured. -Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, the zero-row filter `{ $not: {} }`, and `{}`, on both drivers, measured identical before and after. A `filter` that is not a filter object at all (a string, a number, a boolean, a `Map`, an array) is answered as before. +Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, the zero-row filter `{ $not: {} }`, and `{}`, on both drivers, measured identical before and after. `null`, an absent `filter`, a null-prototype filter object and an array are not refused by the shape gate, as they are not on `where`: `[]` is still read as no filter, and a condition array (`[['amount', '>', 100]]`) is still read by the walker as an object whose index keys are column names, so it counts no row, as before — `AggregationNodeSchema.filter` does not declare that sugar, and the REST door refuses it. diff --git a/packages/objectql/src/engine-aggregate-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index ae896b43fb..ddd93231a8 100644 --- a/packages/objectql/src/engine-aggregate-filter.test.ts +++ b/packages/objectql/src/engine-aggregate-filter.test.ts @@ -523,3 +523,54 @@ describe('[#20122] per-aggregation filter — what the walk leaves alone answers }); } }); + +describe('[#20122] per-aggregation filter — the shape gate `where` takes: a filter that is not a filter object is refused', () => { + // Before, measured through `engine.aggregate` on driver-memory and + // driver-sql: each shape was DROPPED — the aggregation counted every row of + // its group, on a populated table, with no error (driver-sql's native + // aggregate answered the non-empty string with NOT_IMPLEMENTED / 501). The + // REST door refuses each one already, through `AggregationNodeSchema`. + const SHAPE_REFUSED: ReadonlyArray unknown, string]> = [ + ['a string', () => "stage = 'closed_won'", `received string "stage = 'closed_won'"`], + ['a number', () => 42, 'received number 42'], + ['a Map', () => new Map([['stage', 'closed_won']]), 'received Map'], + ['true', () => true, 'received boolean true'], + ['the empty string', () => '', 'received string ""'], + ['a Date', () => new Date(0), 'received Date'], + ]; + + for (const [name, filter, received] of SHAPE_REFUSED) { + it(`${name}: refused before any read, whatever the rows`, async () => { + const message = await expectFilterRefusal(filter); + expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object or condition array, ${received}.`); + expect(message).toContain('It was not applied'); + }); + } + + it('a filter object answers as before — a literal and a null-prototype one alike', async () => { + for (const native of [true, false]) { + for (const filter of [{ stage: 'closed_won' }, Object.assign(Object.create(null), { stage: 'closed_won' })]) { + const { driver } = makeCountingDriver(OPPORTUNITIES, native); + const engine = await makeEngine(driver); + expect(await engine.aggregate('crm_opportunity', withFilter(filter))).toEqual([{ opp_count: 6, picked: 2 }]); + } + } + }); + + it('an array is not this gate\'s to refuse, as it is not on `where` — `[]` and a FilterArray answer as before', async () => { + // Held, not endorsed: the FilterArray's answer is the walker's reading of an + // array (its index keys read as column names, so no row matches) — the + // pre-existing reading, left for its own decision; `[]` is the vacuous + // filter. What this pins is that the shape gate moves neither. + for (const native of [true, false]) { + const { driver } = makeCountingDriver(OPPORTUNITIES, native); + const engine = await makeEngine(driver); + expect(await engine.aggregate('crm_opportunity', withFilter([['amount', '>', 100]]))).toEqual([{ opp_count: 6, picked: 0 }]); + } + // `[]` is read as no filter and so keeps the native push-down; the stand-in's + // native door computes nothing, so it is read on the fallback door here. + const { driver } = makeCountingDriver(OPPORTUNITIES, false); + const engine = await makeEngine(driver); + expect(await engine.aggregate('crm_opportunity', withFilter([]))).toEqual([{ opp_count: 6, picked: 6 }]); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 3cb99915db..ec0b3bae67 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -16090,6 +16090,25 @@ export class ObjectQL implements IObjectQLEngine { for (const [i, agg] of aggregations.entries()) { const aggFilter = (agg as { filter?: unknown })?.filter; if (aggFilter == null) continue; + // [#20122] The SHAPE gate `where` takes first on its own seam + // (`lowerWhereFilterArray`, #20121) — first here too, with its + // accept set and its words. A filter that is not a filter object + // (a string, a number, a boolean, `''`, a `Map`, a `Date`) was + // stepped around by every door below, each of which walks a filter + // node's keys, and then dropped: the fork below reads it as "no + // filter", so the aggregation read every row of its group + // (`driver-sql`'s native aggregate answered a non-empty string + // with a 501 instead). The wire door already refuses these shapes + // through `AggregationNodeSchema`; this closes the in-process one. + // An array is left to the doors below, as `where` leaves one to + // its array branch. + if (!Array.isArray(aggFilter) && !isWhereFilterObject(aggFilter)) { + throw invalidFilterError( + `aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object or condition ` + + `array, received ${describeNonFilterWhere(aggFilter)}. It was not applied, and an ` + + `unapplied filter would have aggregated every row of each group for that aggregation.`, + ); + } assertListComparandShapes(object, 'aggregate', aggFilter, `aggregations[${i}].filter`); assertFilterIsMaterializable(object, 'aggregate', this._registry.getObject(object), aggFilter); // [#15661] …and the declared-type door for the text operators: a @@ -16112,11 +16131,6 @@ export class ObjectQL implements IObjectQLEngine { // walks the filter per SOURCE row, so `{ amount: { $median: 1 } }` // was a 400 on a populated table and a `200 []` on an empty one, // and a `$or` whose first branch held counted every row. - // ⛔ Not the condition-object check `having` got: a `filter` that is - // not a filter node at all (a string, a number, a `Map`) is a - // question about the clause's SHAPE, answered by the shape door - // for every filter position at once — not a refusal that depends - // on the data, which is what this block closes. const typed = normalizeFilterComparandTypes(aggFilter, `aggregate('${object}')`, `aggregations[${i}].filter`); assertAggregationFilterIsEvaluable(typed, i); if (typed !== aggFilter) { From 796b06a12ef96f9f589cca866ccf6d9abfd97d5d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 12:42:44 +0000 Subject: [PATCH 5/6] docs(changeset): name the per-aggregation shape refusal in its own words, and keep each measured claim to what was measured The shape row now quotes the refusal as the code spells it, with the aggregation named; the zero-row filter is credited to its unit pin rather than to the real-driver measurement, which did not carry it; and the addDays note says the refusal names each aggregated column's class where driver-sql names a stored type. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- .changeset/20122-aggregation-filter-row-independent.md | 4 ++-- .changeset/20127-having-adddays-temporal-pair.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/20122-aggregation-filter-row-independent.md b/.changeset/20122-aggregation-filter-row-independent.md index 3f4df77250..0c1cd2dfaf 100644 --- a/.changeset/20122-aggregation-filter-row-independent.md +++ b/.changeset/20122-aggregation-filter-row-independent.md @@ -22,7 +22,7 @@ The engine evaluates a per-aggregation filter itself, in the in-memory fallback, | a `{ $field }` reference whose `addDays` is not an integer (`1.5`, `'7'`) | counted rows by the in-memory evaluator's own reading of that offset, which `FieldReferenceSchema` refuses | a whole-day `addDays` | | `{ amount: { $eq: { v: 1 } } }`, `{ amount: undefined }`, `{ $eq: new Map() }`, a function, an `undefined` `$in` member, a bigint beyond 2^53, or `{ $gt: { $field: 5 } }` | counted no row (each measured under the operator shown, or in the implicit slot). The same comparand in `where` is refused by the comparand-type door; the per-aggregation filter now gets that door's refusal, rooted at its own position | a string, number, bigint, boolean, `null` or `Date` | | a `Symbol` comparand | under `$ne`, counted every row; under `$gt`, threw a raw `TypeError` with no `code` and no `status` on a populated table | a literal of one of the types above | -| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes (`'where' must be a filter object or condition array, received …`), in the same words, naming the aggregation | a filter object, `{ stage: 'won' }` | +| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes, in `where`'s words with the aggregation named (`'aggregations[1].filter' must be a filter object or condition array, received …`) | a filter object, `{ stage: 'won' }` | Not refused, but answering differently: @@ -30,4 +30,4 @@ Not refused, but answering differently: Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. The shape row is reachable in-process only: the REST door already refuses a non-object `filter`. Callers in a deployment were NOT measured. -Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, the zero-row filter `{ $not: {} }`, and `{}`, on both drivers, measured identical before and after. `null`, an absent `filter`, a null-prototype filter object and an array are not refused by the shape gate, as they are not on `where`: `[]` is still read as no filter, and a condition array (`[['amount', '>', 100]]`) is still read by the walker as an object whose index keys are column names, so it counts no row, as before — `AggregationNodeSchema.filter` does not declare that sugar, and the REST door refuses it. +Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, and `{}`, on both drivers, measured identical before and after. The zero-row filter `{ $not: {} }`, which the analytics service lowers FALSE to, still counts no row (a unit pin, green against the base code too). `null`, an absent `filter`, a null-prototype filter object and an array are not refused by the shape gate, as they are not on `where`: `[]` is still read as no filter, and a condition array (`[['amount', '>', 100]]`) is still read by the walker as an object whose index keys are column names, so it counts no row, as before — `AggregationNodeSchema.filter` does not declare that sugar, and the REST door refuses it. diff --git a/.changeset/20127-having-adddays-temporal-pair.md b/.changeset/20127-having-adddays-temporal-pair.md index 80f2268abb..a47a3502a0 100644 --- a/.changeset/20127-having-adddays-temporal-pair.md +++ b/.changeset/20127-having-adddays-temporal-pair.md @@ -18,7 +18,7 @@ An aggregated row has no declared field types, so each column's class is now rea A column whose class the declaration cannot tell is not judged: an object with no field map, a field it does not declare, or a `formula` field. -`having` resolved every pair through `@objectstack/formula`'s evaluator, which reads a number as epoch milliseconds, while `driver-sql` refuses the same pair on `where`. The refusal reuses `driver-sql`'s sentence for that pair, with "is stored as" read as "is", because an aggregated column is computed rather than stored. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, both paths, and through `POST /data/:object/query` on both, over three groups with a `sum` alias `total`, a `max` of a number `max_cap`, `max` / `min` of two `date` fields, `max` / `min` of two `datetime` fields and a `count` `n`: +`having` resolved every pair through `@objectstack/formula`'s evaluator, which reads a number as epoch milliseconds, while `driver-sql` refuses the same pair on `where`. The refusal reuses `driver-sql`'s sentences for the pair, naming each aggregated column's class where `driver-sql` names a stored type ("is numeric" for "is stored as numeric"), because an aggregated column is computed rather than stored. Measured on the base through `engine.aggregate` on `driver-memory` and `driver-sql`, both paths, and through `POST /data/:object/query` on both, over three groups with a `sum` alias `total`, a `max` of a number `max_cap`, `max` / `min` of two `date` fields, `max` / `min` of two `datetime` fields and a `count` `n`: | `having` | before | now | |:--|:--|:--| From 211cfba4770d5afbf78a31265412e481a38ae73e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 13:05:00 +0000 Subject: [PATCH 6/6] fix(objectql)!: a per-aggregation filter that is an array is refused, `[]` included (#20122) Seat amendment 2 (ruling A on the PR's open question). The shape gate on `aggregations[i].filter` no longer exempts an array: the slot is declared `FilterConditionSchema`, which admits no array form, and the REST door already refuses every array there (VALIDATION_FAILED). In-process, a condition array counted no row (its index positions read as column names) and `[]` read as no filter. An array of any length is now refused with INVALID_FILTER / 400 before any driver call, in words naming the object form; `undefined`, `null`, a plain object and a null-prototype object are unchanged. The non-array refusal now reads "must be a filter object" as well, since a condition array is not accepted here. The held array control becomes three refusal rows. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude --- ...0122-aggregation-filter-row-independent.md | 9 +++-- .../src/engine-aggregate-filter.test.ts | 37 ++++++++++--------- packages/objectql/src/engine.ts | 36 +++++++++++++++--- 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/.changeset/20122-aggregation-filter-row-independent.md b/.changeset/20122-aggregation-filter-row-independent.md index 0c1cd2dfaf..40b648f72c 100644 --- a/.changeset/20122-aggregation-filter-row-independent.md +++ b/.changeset/20122-aggregation-filter-row-independent.md @@ -6,7 +6,7 @@ fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine Clause-②: no (narrowing) - + **BREAKING**: this narrows what a per-aggregation `filter` accepts on `engine.aggregate`, and on the REST aggregate query (`POST /data/:object/query` with `aggregations`) that forwards it there. Every refusal below is `INVALID_FILTER` / 400, raised in the engine's per-aggregation loop, before any driver is asked for a row, identically on an empty and on a populated table. The refusal names the aggregation that carries the filter (`aggregations[1].filter`). It ships as `minor` under the launch-window convention for accept-set narrowings. @@ -22,12 +22,13 @@ The engine evaluates a per-aggregation filter itself, in the in-memory fallback, | a `{ $field }` reference whose `addDays` is not an integer (`1.5`, `'7'`) | counted rows by the in-memory evaluator's own reading of that offset, which `FieldReferenceSchema` refuses | a whole-day `addDays` | | `{ amount: { $eq: { v: 1 } } }`, `{ amount: undefined }`, `{ $eq: new Map() }`, a function, an `undefined` `$in` member, a bigint beyond 2^53, or `{ $gt: { $field: 5 } }` | counted no row (each measured under the operator shown, or in the implicit slot). The same comparand in `where` is refused by the comparand-type door; the per-aggregation filter now gets that door's refusal, rooted at its own position | a string, number, bigint, boolean, `null` or `Date` | | a `Symbol` comparand | under `$ne`, counted every row; under `$gt`, threw a raw `TypeError` with no `code` and no `status` on a populated table | a literal of one of the types above | -| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes, in `where`'s words with the aggregation named (`'aggregations[1].filter' must be a filter object or condition array, received …`) | a filter object, `{ stage: 'won' }` | +| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes, with the aggregation named (`'aggregations[1].filter' must be a filter object, received …`) | a filter object, `{ stage: 'won' }` | +| an array, `[]` included: a condition array (`[['amount', '>', 100]]`, `['and', …]`) or an empty one | a condition array counted NO row: the walker read its index positions as column names. `[]` was read as no filter. The REST door already refused every array here with `VALIDATION_FAILED` / 400. Now refused in-process too (`'aggregations[1].filter' must be a filter object, received an array (…)`), because `AggregationNodeSchema.filter` is declared `FilterConditionSchema`, which admits no array form: the condition-array sugar is lowered on `where` alone | the object form, `{ amount: { $gt: 100 } }`; omit `filter` for no filter | Not refused, but answering differently: - **An exact-range bigint comparand is narrowed to a number, as it is in `where`.** `{ amount: { $in: [400n, 20n] } }` counted no row, because `[400n].includes(400)` is false. It now counts the rows it names. The caller's aggregation entry is not edited. -Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. The shape row is reachable in-process only: the REST door already refuses a non-object `filter`. Callers in a deployment were NOT measured. +Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. The two shape rows are reachable in-process only: the REST door already refuses a non-object `filter` and every array. Callers in a deployment were NOT measured. -Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, and `{}`, on both drivers, measured identical before and after. The zero-row filter `{ $not: {} }`, which the analytics service lowers FALSE to, still counts no row (a unit pin, green against the base code too). `null`, an absent `filter`, a null-prototype filter object and an array are not refused by the shape gate, as they are not on `where`: `[]` is still read as no filter, and a condition array (`[['amount', '>', 100]]`) is still read by the walker as an object whose index keys are column names, so it counts no row, as before — `AggregationNodeSchema.filter` does not declare that sugar, and the REST door refuses it. +Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, and `{}`, on both drivers, measured identical before and after. The zero-row filter `{ $not: {} }`, which the analytics service lowers FALSE to, still counts no row (a unit pin, green against the base code too). `null`, an absent `filter` and a null-prototype filter object are not refused by the shape gate, as they are not on `where`, and answer as before. diff --git a/packages/objectql/src/engine-aggregate-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index ddd93231a8..66c475b589 100644 --- a/packages/objectql/src/engine-aggregate-filter.test.ts +++ b/packages/objectql/src/engine-aggregate-filter.test.ts @@ -542,7 +542,7 @@ describe('[#20122] per-aggregation filter — the shape gate `where` takes: a fi for (const [name, filter, received] of SHAPE_REFUSED) { it(`${name}: refused before any read, whatever the rows`, async () => { const message = await expectFilterRefusal(filter); - expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object or condition array, ${received}.`); + expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object, ${received}.`); expect(message).toContain('It was not applied'); }); } @@ -557,20 +557,23 @@ describe('[#20122] per-aggregation filter — the shape gate `where` takes: a fi } }); - it('an array is not this gate\'s to refuse, as it is not on `where` — `[]` and a FilterArray answer as before', async () => { - // Held, not endorsed: the FilterArray's answer is the walker's reading of an - // array (its index keys read as column names, so no row matches) — the - // pre-existing reading, left for its own decision; `[]` is the vacuous - // filter. What this pins is that the shape gate moves neither. - for (const native of [true, false]) { - const { driver } = makeCountingDriver(OPPORTUNITIES, native); - const engine = await makeEngine(driver); - expect(await engine.aggregate('crm_opportunity', withFilter([['amount', '>', 100]]))).toEqual([{ opp_count: 6, picked: 0 }]); - } - // `[]` is read as no filter and so keeps the native push-down; the stand-in's - // native door computes nothing, so it is read on the fallback door here. - const { driver } = makeCountingDriver(OPPORTUNITIES, false); - const engine = await makeEngine(driver); - expect(await engine.aggregate('crm_opportunity', withFilter([]))).toEqual([{ opp_count: 6, picked: 6 }]); - }); + // [#20122, seat ruling A on the array question] An ARRAY is refused too, `[]` + // included: the slot is declared `FilterConditionSchema`, which admits no + // array form, and the REST door refuses every array there already + // (`VALIDATION_FAILED`). Before, in-process, a condition array counted NO row + // (the walker read its index positions as column names) and `[]` read as no + // filter — neither is what the declaration allows. + const ARRAY_REFUSED: ReadonlyArray unknown, string]> = [ + ['an empty array', () => [], 'received an array ([])'], + ['a condition array', () => [['amount', '>', 100]], 'received an array ([["amount",">",100]])'], + ['a logical-group array', () => ['and', ['stage', '=', 'closed_won'], ['amount', '>', 100]], 'received an array (["and",'], + ]; + + for (const [name, filter, received] of ARRAY_REFUSED) { + it(`${name}: refused before any read, whatever the rows, naming the object form`, async () => { + const message = await expectFilterRefusal(filter); + expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object, ${received}`); + expect(message).toContain('input-only sugar'); + }); + } }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index ec0b3bae67..45301fa9e4 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -16092,7 +16092,8 @@ export class ObjectQL implements IObjectQLEngine { if (aggFilter == null) continue; // [#20122] The SHAPE gate `where` takes first on its own seam // (`lowerWhereFilterArray`, #20121) — first here too, with its - // accept set and its words. A filter that is not a filter object + // object test (`isWhereFilterObject`) and its words, minus the + // array half (below). A filter that is not a filter object // (a string, a number, a boolean, `''`, a `Map`, a `Date`) was // stepped around by every door below, each of which walks a filter // node's keys, and then dropped: the fork below reads it as "no @@ -16100,12 +16101,35 @@ export class ObjectQL implements IObjectQLEngine { // (`driver-sql`'s native aggregate answered a non-empty string // with a 501 instead). The wire door already refuses these shapes // through `AggregationNodeSchema`; this closes the in-process one. - // An array is left to the doors below, as `where` leaves one to - // its array branch. - if (!Array.isArray(aggFilter) && !isWhereFilterObject(aggFilter)) { + // + // An ARRAY is refused too, `[]` included — unlike `where`, which + // lowers the condition-array sugar in its array branch. The slot + // is declared `FilterConditionSchema` (an object, no array form), + // the wire door refuses every array here (`VALIDATION_FAILED`), + // and in-process a condition array counted no row (the walker + // read its index positions as column names) while `[]` read as + // no filter. Same ruling as `having`'s condition check (#20099). + if (Array.isArray(aggFilter)) { + let shown: string; + try { + shown = JSON.stringify(aggFilter) ?? String(aggFilter); + } catch { + shown = String(aggFilter); + } + if (shown.length > 80) shown = `${shown.slice(0, 77)}...`; + throw invalidFilterError( + `aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object, received ` + + `an array (${shown}). The condition-array form — [field, operator, value] tuples and ` + + `["and", …] groups — is input-only sugar lowered on 'where' alone; a per-aggregation ` + + `filter is declared as a filter condition object and does not take it, so an array here ` + + `was never applied as the filter it spells. Write the object form, ` + + `{ "FIELD": { "$gt": 100 } }, or omit 'filter' for no filter.`, + ); + } + if (!isWhereFilterObject(aggFilter)) { throw invalidFilterError( - `aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object or condition ` + - `array, received ${describeNonFilterWhere(aggFilter)}. It was not applied, and an ` + + `aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object, ` + + `received ${describeNonFilterWhere(aggFilter)}. It was not applied, and an ` + `unapplied filter would have aggregated every row of each group for that aggregation.`, ); }