diff --git a/.changeset/20122-aggregation-filter-row-independent.md b/.changeset/20122-aggregation-filter-row-independent.md new file mode 100644 index 00000000000..40b648f72c2 --- /dev/null +++ b/.changeset/20122-aggregation-filter-row-independent.md @@ -0,0 +1,34 @@ +--- +"@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 shape gate and 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 | +| 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 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` 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/.changeset/20123-having-unknown-column-refused.md b/.changeset/20123-having-unknown-column-refused.md new file mode 100644 index 00000000000..25a54bdf153 --- /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/.changeset/20127-having-adddays-temporal-pair.md b/.changeset/20127-having-adddays-temporal-pair.md new file mode 100644 index 00000000000..a47a3502a01 --- /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 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 | +|:--|:--|:--| +| `{ 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-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index 474a8c6b71e..66c475b589a 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,281 @@ 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 }]); + } + }); + } +}); + +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, ${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 }]); + } + } + }); + + // [#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-aggregate-having-comparand-shape.test.ts b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts index 62bfb2cc6e0..9b96dad2d64 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 }; @@ -667,3 +671,196 @@ 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]); + } + }); + } +}); + +// ─────────────────────────────────────────────────────────────────────────── +// [#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 4f5d28170b8..45301fa9e47 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -259,6 +259,8 @@ import { import { applyHaving, aggregatedRowColumns, + aggregatedRowColumnClasses, + assertAggregationFilterIsEvaluable, assertHavingIsEvaluable, assertHavingIsFilterCondition, } from './having-filter.js'; @@ -16082,17 +16084,85 @@ 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; + // [#20122] The SHAPE gate `where` takes first on its own seam + // (`lowerWhereFilterArray`, #20121) — first here too, with its + // 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 + // 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 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, ` + + `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 + // `$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. + 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 — @@ -16137,14 +16207,29 @@ 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); 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 7d9f8ace642..876293a1e91 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -80,6 +80,19 @@ // 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}. +// +// [#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 @@ -89,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 @@ -407,6 +430,109 @@ 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}.`, + ); +} + +/** + * [#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}. @@ -450,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 @@ -489,31 +696,102 @@ 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}; + * - [#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 + * 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', columns); +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, classes, unknownKeys }); + if (unknownKeys.length > 0) throw unknownHavingColumnError(unknownKeys, 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 key and a `{ $field }` reference must name. + */ +interface EvaluableScope { + clause: FilterClause; + /** + * 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[]; + /** + * [#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 }>; } /** 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); + // [#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 }); } } @@ -522,7 +800,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 +820,15 @@ 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); + if (scope.classes && target.addDays !== undefined) { + assertOffsetPairIsTemporal(field, op, target, `${path}.${op}`, scope.classes); + } + } continue; } if (Array.isArray(target)) { @@ -561,16 +844,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) {