Skip to content

Commit 8662122

Browse files
committed
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 <noreply@anthropic.com>
1 parent f09d412 commit 8662122

4 files changed

Lines changed: 368 additions & 22 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
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)
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (already-registered filter-between-field-reference-endpoint-refused, filter-icontains-comparand-refused-at-parse, filter-regex-options-retired) this change adds no new transition. Each refusal below was already made by the per-aggregation filter's walker on a populated table, or is a shared door `where` already takes, run unmodified on one more position: the first id's replacement states that a { $field } reference is legal as the WHOLE comparand of $eq / $ne / $gt / $gte / $lt / $lte only, which is the position refusal the walker now makes whatever the rows; the other two register the $icontains comparand and the retired $regex / $options refusals it already made on a populated table. The comparand-type set was ruled at the shared face with no ledger entry. An unknown operator has no meaning to migrate to. A stored measure filter reaches this position through the analytics service, which lowers it; the refused inputs are ones that position never evaluated as written, so there is no rewrite for `objectstack migrate meta` to perform. The table below is the author-facing remedy for each row, not a mechanical rewrite. -->
10+
11+
**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.
12+
13+
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 }`:
14+
15+
| you wrote in `aggregations[i].filter` | what it did before | write instead |
16+
|:--|:--|:--|
17+
| 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 |
18+
| 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 |
19+
| 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 |
20+
| `{ 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` |
21+
| 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 |
22+
| 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` |
23+
| `{ 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` |
24+
| 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 |
25+
26+
Not refused, but answering differently:
27+
28+
- **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.
29+
30+
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.
31+
32+
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.

‎packages/objectql/src/engine-aggregate-filter.test.ts‎

Lines changed: 229 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,14 @@
2424
// ADR-0112 `INVALID_FILTER`/400 envelope, naming the aggregation position
2525
// — ignoring it would silently answer the unfiltered aggregate, which is
2626
// the very defect this key closes.
27+
// * [#20122] every such refusal is the FILTER's, not the data's: raised once,
28+
// before any driver is asked for a row, identically on an empty and on a
29+
// populated table (the last block of this file).
2730

2831
import { describe, it, expect } from 'vitest';
29-
import type { EngineAggregateOptions } from '@objectstack/spec/data';
32+
import { normalizeFilterComparandTypes, type EngineAggregateOptions } from '@objectstack/spec/data';
3033
import { ObjectQL } from './engine.js';
34+
import { matchesAggregationFilter } from './having-filter.js';
3135

3236
// The #10413 measurement's dataset shape: opportunities with a stage and an
3337
// amount. 6 rows, 2 closed_won worth 700 total.
@@ -295,3 +299,227 @@ describe('engine.aggregate — per-aggregation filter (#10576, the #10413 contra
295299
expect(thrown!.message).toContain('aggregations[0].filter');
296300
});
297301
});
302+
303+
// ───────────────────────────────────────────────────────────────────────────
304+
// [#20122] The verdict belongs to the filter, not to the data
305+
// ───────────────────────────────────────────────────────────────────────────
306+
307+
/**
308+
* A driver that records every read, with or without a native `aggregate()`.
309+
* A per-aggregation filter forces the in-memory fallback on both kinds (the
310+
* #10576 fork), so a refusal raised AFTER a read shows up as `find: 1`.
311+
*/
312+
function makeCountingDriver(rows: ReadonlyArray<Record<string, unknown>>, native: boolean) {
313+
const calls = { aggregate: 0, find: 0 };
314+
const driver: any = {
315+
name: native ? 'native-agg-recorder' : 'raw-recorder',
316+
version: '0.0.0',
317+
supports: {},
318+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
319+
async find() { calls.find += 1; return rows.map((r) => ({ ...r })); },
320+
async findOne() { return rows[0] ?? null; },
321+
async create(_o: string, d: any) { return d; },
322+
async update(_o: string, _id: string, d: any) { return d; },
323+
async delete() { return true; },
324+
async count() { return rows.length; },
325+
async bulkCreate(_o: string, r: any[]) { return r; },
326+
async bulkUpdate() { return []; }, async bulkDelete() {},
327+
async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; },
328+
async commit() {}, async rollback() {},
329+
};
330+
if (native) driver.aggregate = async () => { calls.aggregate += 1; return []; };
331+
return { driver, calls };
332+
}
333+
334+
interface Refusal extends Error { code?: unknown; status?: unknown }
335+
336+
async function refusalOf(run: () => Promise<unknown>): Promise<Refusal> {
337+
let out: unknown;
338+
try {
339+
out = await run();
340+
} catch (e) {
341+
return e as Refusal;
342+
}
343+
throw new Error(`expected a refusal, but it answered ${JSON.stringify(out)}`);
344+
}
345+
346+
function syncRefusalOf(run: () => unknown): Refusal | undefined {
347+
try {
348+
run();
349+
} catch (e) {
350+
return e as Refusal;
351+
}
352+
return undefined;
353+
}
354+
355+
/**
356+
* The filter sits on the SECOND aggregation, so every refusal must name
357+
* `aggregations[1].filter` — which aggregation carries it, not merely that one
358+
* does.
359+
*/
360+
function withFilter(filter: unknown, groupBy?: string[]): EngineAggregateOptions {
361+
return {
362+
...(groupBy ? { groupBy } : {}),
363+
aggregations: [
364+
{ function: 'count', alias: 'opp_count' },
365+
{ function: 'count', alias: 'picked', filter },
366+
],
367+
} as unknown as EngineAggregateOptions;
368+
}
369+
370+
const AT = 'aggregations[1].filter';
371+
372+
/**
373+
* Refused on both driver kinds, on an EMPTY and a populated table, grouped and
374+
* ungrouped, with one message and no driver read. Returns the message.
375+
*
376+
* Before: every empty-table cell answered — `[]` grouped, `[{ opp_count: 0,
377+
* picked: 0 }]` ungrouped — and the populated cells refused only after the
378+
* driver had been read (`find: 1`), when they refused at all.
379+
*/
380+
async function expectFilterRefusal(filter: () => unknown): Promise<string> {
381+
let message: string | undefined;
382+
for (const native of [true, false]) {
383+
for (const [population, rows] of [['empty', []], ['populated', OPPORTUNITIES]] as const) {
384+
for (const groupBy of [undefined, ['region']]) {
385+
const cell = `${native ? 'native-capable' : 'raw'} driver, ${population}, ${groupBy ? 'grouped' : 'ungrouped'}`;
386+
const { driver, calls } = makeCountingDriver(rows, native);
387+
const engine = await makeEngine(driver);
388+
const err = await refusalOf(() => engine.aggregate('crm_opportunity', withFilter(filter(), groupBy)));
389+
expect(err, cell).toBeInstanceOf(Error);
390+
expect(err.code, cell).toBe('INVALID_FILTER');
391+
expect(err.status, cell).toBe(400);
392+
expect(calls, cell).toEqual({ aggregate: 0, find: 0 });
393+
message ??= err.message;
394+
expect(err.message, cell).toBe(message);
395+
}
396+
}
397+
}
398+
return message!;
399+
}
400+
401+
describe('[#20122] per-aggregation filter — the walker\'s refusals belong to the filter, not to the data', () => {
402+
// Each row: the filter, and a SOURCE row that walks the per-row evaluator
403+
// INTO the refused arm — the floor whose words the engine-level refusal
404+
// keeps. Before, measured through `engine.aggregate` on driver-memory and
405+
// driver-sql (and through `POST /data/:object/query`): the populated table
406+
// refused, the empty one answered; the column-absent row counted ZERO on a
407+
// populated table too (the no-value exit sat before the operator was read);
408+
// and the `$or` row counted EVERY row, its first branch having held.
409+
const WALKER_REFUSED: ReadonlyArray<readonly [string, () => Record<string, unknown>, Record<string, unknown>]> = [
410+
['an unknown condition operator (the triage shape)', () => ({ amount: { $median: 1 } }), { amount: 1 }],
411+
['an unknown logical operator', () => ({ $nand: [{ amount: 1 }] }), { amount: 1 }],
412+
['a retired operator', () => ({ stage: { $regex: 'won' } }), { stage: 'closed_won' }],
413+
['a retired operator with its retired sibling', () => ({ stage: { $regex: 'won', $options: 'i' } }), { stage: 'closed_won' }],
414+
['an operator of `where` this walker does not evaluate ($like)', () => ({ stage: { $like: 'closed%' } }), { stage: 'closed_won' }],
415+
['a $like with a dangling escape', () => ({ stage: { $like: 'closed\\' } }), { stage: 'closed_won' }],
416+
['an empty $icontains', () => ({ stage: { $icontains: '' } }), { stage: 'closed_won' }],
417+
['a non-string $icontains', () => ({ stage: { $icontains: 5 } }), { stage: 'closed_won' }],
418+
['a non-$ key beside an operator', () => ({ amount: { $gt: 1, foo: 2 } }), { amount: 5 }],
419+
['an unknown operator on a column the row does not carry', () => ({ nope: { $median: 1 } }), { nope: 1 }],
420+
['an unknown operator behind a $or branch that already held', () => ({ $or: [{ amount: { $gt: 0 } }, { amount: { $median: 1 } }] }), { amount: -1 }],
421+
['an unknown operator under $not', () => ({ $not: { amount: { $median: 1 } } }), { amount: 1 }],
422+
];
423+
424+
for (const [name, filter, floorRow] of WALKER_REFUSED) {
425+
it(`${name}: refused on an empty and a populated table, in the walker's own words`, async () => {
426+
const floor = syncRefusalOf(() => matchesAggregationFilter(floorRow, filter() as never, 1));
427+
expect(floor, 'the per-row walker must refuse this row when it reaches it').toBeDefined();
428+
const message = await expectFilterRefusal(filter);
429+
expect(message).toBe(floor!.message);
430+
expect(message).toContain(AT);
431+
});
432+
}
433+
434+
// Before: each row counted nothing in any group (`$nin` and `$exists`
435+
// counted every row), or — the bare form — was refused on a populated table
436+
// only, as an unsupported `$field` operator.
437+
const REFERENCE_REFUSED: ReadonlyArray<readonly [string, () => Record<string, unknown>, string]> = [
438+
['a bare reference with no operator', () => ({ amount: { $field: 'amount' } }), `${AT}.amount`],
439+
['a reference as an $in member', () => ({ amount: { $in: [{ $field: 'amount' }] } }), `${AT}.amount.$in`],
440+
['a reference as a $nin member', () => ({ amount: { $nin: [1, { $field: 'amount' }] } }), `${AT}.amount.$nin`],
441+
['a reference as a $contains pattern', () => ({ stage: { $contains: { $field: 'region' } } }), `${AT}.stage.$contains`],
442+
['a reference under $exists', () => ({ amount: { $exists: { $field: 'amount' } } }), `${AT}.amount.$exists`],
443+
['a fractional addDays', () => ({ amount: { $lte: { $field: 'amount', addDays: 1.5 } } }), `${AT}.amount.$lte`],
444+
['a string addDays', () => ({ amount: { $lte: { $field: 'amount', addDays: '7' } } }), `${AT}.amount.$lte`],
445+
];
446+
447+
for (const [name, filter, path] of REFERENCE_REFUSED) {
448+
it(`${name}: refused at ${path}, whatever the rows`, async () => {
449+
expect(await expectFilterRefusal(filter)).toContain(path);
450+
});
451+
}
452+
});
453+
454+
describe('[#20122] per-aggregation filter — the comparand-TYPE door `where` takes', () => {
455+
// Before: each row counted no row (`$ne` every row) on a populated table, and
456+
// a Symbol under an ordering operator threw a raw `TypeError` with no `code`
457+
// and no `status` — populated tables only; an empty one answered.
458+
const TYPE_REFUSED: ReadonlyArray<readonly [string, () => Record<string, unknown>]> = [
459+
['a plain object under $eq', () => ({ amount: { $eq: { v: 1 } } })],
460+
['undefined in the implicit-equality slot', () => ({ amount: undefined })],
461+
['a Map under $eq', () => ({ amount: { $eq: new Map() } })],
462+
['a function under $gt', () => ({ amount: { $gt: () => 1 } })],
463+
['a Symbol under $gt', () => ({ amount: { $gt: Symbol('x') } })],
464+
['a Symbol under $ne', () => ({ amount: { $ne: Symbol('x') } })],
465+
['an undefined $in member', () => ({ amount: { $in: [undefined] } })],
466+
['a bigint beyond 2^53', () => ({ amount: { $gt: 2n ** 60n } })],
467+
['a { $field } whose $field is not a string', () => ({ amount: { $gt: { $field: 5 } } })],
468+
['a plain object nested in $or', () => ({ $or: [{ amount: 1 }, { amount: { $eq: { v: 1 } } }] })],
469+
];
470+
471+
for (const [name, filter] of TYPE_REFUSED) {
472+
it(`${name}: refused in the type door's own words, rooted at the aggregation, whatever the rows`, async () => {
473+
const door = syncRefusalOf(() => normalizeFilterComparandTypes(filter(), "aggregate('crm_opportunity')", AT));
474+
expect(door, 'the type door must refuse this row directly').toBeDefined();
475+
expect(await expectFilterRefusal(filter)).toBe(door!.message);
476+
});
477+
}
478+
479+
it('an exact-range bigint is NARROWED, as it is in where — and the caller\'s entry is not edited', async () => {
480+
// Before: `{ $in: [500n, 200n] }` counted no row — `[500n].includes(500)`
481+
// is false — while the same list in `where` is narrowed to numbers first.
482+
for (const native of [true, false]) {
483+
const { driver } = makeCountingDriver(OPPORTUNITIES, native);
484+
const engine = await makeEngine(driver);
485+
const query = withFilter({ amount: { $in: [500n, 200n] } });
486+
const rows = await engine.aggregate('crm_opportunity', query);
487+
expect(rows).toEqual([{ opp_count: 6, picked: 2 }]);
488+
expect((query.aggregations![1] as { filter?: unknown }).filter).toEqual({ amount: { $in: [500n, 200n] } });
489+
}
490+
});
491+
});
492+
493+
describe('[#20122] per-aggregation filter — what the walk leaves alone answers exactly as before', () => {
494+
// Measured identical at base and head through `engine.aggregate` on
495+
// driver-memory and driver-sql. Each answers on both driver kinds; the empty
496+
// table answers the ruled empty values.
497+
const PASSING: ReadonlyArray<readonly [string, () => Record<string, unknown>, number]> = [
498+
['implicit equality', () => ({ stage: 'closed_won' }), 2],
499+
['a scalar ordering bound', () => ({ amount: { $gt: 100 } }), 4],
500+
['a list under $in', () => ({ amount: { $in: [500, 20] } }), 2],
501+
['a list under $nin', () => ({ stage: { $nin: ['open'] } }), 4],
502+
['a [min, max] pair under $between', () => ({ amount: { $between: [50, 300] } }), 3],
503+
['a case-insensitive $icontains', () => ({ stage: { $icontains: 'WON' } }), 2],
504+
['$startsWith', () => ({ stage: { $startsWith: 'closed' } }), 4],
505+
['$ne: null', () => ({ region: { $ne: null } }), 6],
506+
['$exists', () => ({ region: { $exists: true } }), 6],
507+
['composed under $or / $not', () => ({ $or: [{ amount: { $gt: 800 } }, { $not: { stage: { $ne: 'closed_lost' } } }] }), 3],
508+
['a { $field } reference in a scalar comparison', () => ({ amount: { $gte: { $field: 'amount' } } }), 6],
509+
['the zero-row filter analytics lowers FALSE to', () => ({ $not: {} }), 0],
510+
['an exact bigint in the implicit slot', () => ({ amount: 500n }), 1],
511+
];
512+
513+
for (const [name, filter, populatedCount] of PASSING) {
514+
it(`${name} counts ${populatedCount} of 6, and 0 on an empty table`, async () => {
515+
for (const native of [true, false]) {
516+
const { driver } = makeCountingDriver(OPPORTUNITIES, native);
517+
const populated = await makeEngine(driver);
518+
expect(await populated.aggregate('crm_opportunity', withFilter(filter()))).toEqual([{ opp_count: 6, picked: populatedCount }]);
519+
const { driver: emptyDriver } = makeCountingDriver([], native);
520+
const empty = await makeEngine(emptyDriver);
521+
expect(await empty.aggregate('crm_opportunity', withFilter(filter()))).toEqual([{ opp_count: 0, picked: 0 }]);
522+
}
523+
});
524+
}
525+
});

0 commit comments

Comments
 (0)