Skip to content

Commit 71e3ae2

Browse files
committed
fix(objectql)!: a having key naming no column of the aggregated row is refused (#20123)
The aggregated row carries exactly the columns the query projects, so a `having` key outside them read "no value" in every group: a typo for an alias (`{ totl: { $gt: 100 } }`) kept no group, and a negated test on it kept every group, with no error either way. `assertHavingIsEvaluable` now judges every key, at any depth under `$and` / `$or` / `$not`, against the same `aggregatedRowColumns` set the `{ $field }` reference is judged against, once the rest of the clause has passed, and refuses the unknown ones together (INVALID_FILTER / 400), naming each, the first with its position, and listing the columns. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8662122 commit 71e3ae2

4 files changed

Lines changed: 172 additions & 7 deletions

File tree

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
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)
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) this change adds no transition to migrate. A `having` key that names no column of the aggregated row never had a meaning: the rows carry exactly the columns the query projects, so such a key could only ever read "no value". There is no accepted spelling it can be mechanically rewritten to — which of the query's columns the author meant is an authoring decision, and the refusal prints the list. `having` is a request-only key: no metadata type stores it, so there is no stored document for `objectstack migrate meta` to rewrite. The table below records the answer each shape had and has; it prescribes no rewrite. -->
10+
11+
**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.
12+
13+
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`:
14+
15+
| `having` | before | now |
16+
|:--|:--|:--|
17+
| `{ 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 |
18+
| `{ totl: { $ne: 1 } }`, `{ totl: { $exists: false } }`, or `{ $not: { totl: { $gt: 100 } } }` | EVERY group, no error | refused |
19+
| `{ $or: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }` | every group whose `total` is positive: the walk stopped at the branch that held | refused |
20+
| `{ $and: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }` | no group | refused |
21+
| `{ 'customer_id.name': 'c1' }` (a dotted path) | no group | refused |
22+
| `{ customer_id: 'c1' }` when the groupBy item is `{ field: 'customer_id', alias: 'cust' }` | no group: the row projects `cust` | refused; `{ cust: 'c1' }` answers |
23+
24+
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.
25+
26+
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.
27+
28+
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.

‎packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -667,3 +667,86 @@ describe('[#20099] having — a { $field } reference resolves against the aggreg
667667
}
668668
});
669669
});
670+
671+
// ───────────────────────────────────────────────────────────────────────────
672+
// [#20123] A `having` key names a column of the aggregated row
673+
// ───────────────────────────────────────────────────────────────────────────
674+
675+
describe('[#20123] having — a key naming no column of the aggregated row is refused, whatever the rows', () => {
676+
// `AGG_QUERY` projects exactly three columns.
677+
const COLUMNS = ['customer_id', 'order_count', 'total'];
678+
679+
// Before, measured through `engine.aggregate` on driver-memory and
680+
// driver-sql, both doors, and through `POST /data/:object/query`: no row
681+
// raised anything. A test for a value kept no group; a test for absence or
682+
// a negation — and a `$or` whose first branch held — kept EVERY group.
683+
const UNKNOWN_KEY: ReadonlyArray<readonly [string, () => Record<string, unknown>, string, string]> = [
684+
['a typo for an alias under $gt (the triage shape)', () => ({ totl: { $gt: 100 } }), 'totl', 'having.totl'],
685+
['a typo in the implicit-equality slot', () => ({ totl: 500 }), 'totl', 'having.totl'],
686+
['a typo under $ne', () => ({ totl: { $ne: 1 } }), 'totl', 'having.totl'],
687+
['a typo under $exists: false', () => ({ totl: { $exists: false } }), 'totl', 'having.totl'],
688+
['a typo nested in $and', () => ({ $and: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }), 'totl', 'having.$and[1].totl'],
689+
['a typo behind a $or branch that already held', () => ({ $or: [{ total: { $gt: 0 } }, { totl: { $gt: 100 } }] }), 'totl', 'having.$or[1].totl'],
690+
['a typo under $not', () => ({ $not: { totl: { $gt: 100 } } }), 'totl', 'having.$not.totl'],
691+
['a SOURCE column the aggregated row does not project', () => ({ amount: { $gt: 100 } }), 'amount', 'having.amount'],
692+
['a dotted path', () => ({ 'customer_id.name': 'c1' }), 'customer_id.name', 'having.customer_id.name'],
693+
['a key that also carries a { $field } reference', () => ({ totl: { $gt: { $field: 'total' } } }), 'totl', 'having.totl'],
694+
];
695+
696+
for (const [name, having, key, path] of UNKNOWN_KEY) {
697+
it(`${name}: refused at ${path}, naming the columns`, async () => {
698+
const message = await expectRowIndependentRefusal(offContract({ ...AGG_QUERY, having: having() }), ROWS);
699+
expect(message).toContain(`'${key}' at ${path}`);
700+
for (const column of COLUMNS) expect(message).toContain(column);
701+
});
702+
}
703+
704+
it('every unknown key is named — the first with its position, the rest after it', async () => {
705+
const message = await expectRowIndependentRefusal(
706+
offContract({ ...AGG_QUERY, having: { $and: [{ totl: 1 }, { cnt: { $gt: 1 } }, { totl: 2 }] } }), ROWS);
707+
expect(message).toContain("'totl' at having.$and[0].totl");
708+
expect(message).toContain('(also: cnt)');
709+
});
710+
711+
it('an operator refusal on an unknown column is still the operator\'s — the key is judged last', async () => {
712+
// The #20099 rule: a condition on a column the row does not carry is read
713+
// for its operator. Held here so the new check cannot jump ahead of it.
714+
const floor = syncRefusalOf(() => applyHaving([{ nope: 1 }], { nope: { $median: 1 } } as FilterCondition));
715+
const message = await expectRowIndependentRefusal(offContract({ ...AGG_QUERY, having: { nope: { $median: 1 } } }), ROWS);
716+
expect(message).toBe(floor!.message);
717+
});
718+
719+
it('the source name of an ALIASED groupBy projection is not a column — its alias is', async () => {
720+
// Before: `{ customer_id: 'c1' }` kept no group — the row projects `cust`.
721+
const query = (having: FilterCondition): EngineAggregateOptions => ({
722+
groupBy: [{ field: 'customer_id', alias: 'cust' }],
723+
aggregations: [{ function: 'count', alias: 'order_count' }],
724+
having,
725+
});
726+
const message = await expectRowIndependentRefusal(query({ customer_id: 'c1' }), ROWS);
727+
expect(message).toContain("'customer_id' at having.customer_id");
728+
expect(message).toContain('cust, order_count');
729+
// The alias itself answers, on the door that projects it (the stand-in
730+
// native driver groups by `customer_id` and ignores the alias).
731+
const { engine } = await makeEngine(ROWS, false);
732+
const rows = await engine.aggregate(OBJECT, query({ cust: 'c1' }));
733+
expect(rows).toEqual([{ cust: 'c1', order_count: 2 }]);
734+
});
735+
736+
// Every key names a column: answered exactly as before, both doors.
737+
const PASSING: ReadonlyArray<readonly [string, FilterCondition, readonly string[]]> = [
738+
['a groupBy column', { customer_id: 'c1' }, ['c1']],
739+
['an aggregation alias', { total: { $gt: 100 } }, ['c1', 'c2']],
740+
['a count alias', { order_count: { $gte: 2 } }, ['c1', 'c2']],
741+
['columns nested under $or / $and / $not', { $or: [{ total: { $gt: 1000 } }, { $and: [{ $not: { customer_id: 'c1' } }, { order_count: 1 }] }] }, ['c2', 'c3']],
742+
['a negation of a column', { total: { $ne: 500 } }, ['c2', 'c3']],
743+
];
744+
for (const [name, having, expected] of PASSING) {
745+
it(`${name} answers ${JSON.stringify(expected)} on both doors`, async () => {
746+
for (const [door, native] of DOORS) {
747+
const { engine } = await makeEngine(ROWS, native);
748+
expect(groups(await engine.aggregate(OBJECT, { ...AGG_QUERY, having })), door).toEqual([...expected]);
749+
}
750+
});
751+
}
752+
});

‎packages/objectql/src/engine.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16070,7 +16070,10 @@ export class ObjectQL implements IObjectQLEngine {
1607016070
// position and name, judged against the aggregated row's column set
1607116071
// read off THIS query (`assertHavingIsEvaluable`). The walker raised
1607216072
// them per aggregated row, so an empty grouped set answered `200 []`
16073-
// for a `having` a populated one refused.
16073+
// for a `having` a populated one refused. [#20123] The same column
16074+
// set judges every KEY too: a key naming no column read "no value"
16075+
// in every group, so a typo for an alias kept no group (every group
16076+
// under a negation) with no error.
1607416077
// A reference that passes is then RESOLVED against each aggregated row
1607516078
// (having-filter.ts `compareWithReference`) on both doors below.
1607616079
assertHavingIsFilterCondition(query.having);

‎packages/objectql/src/having-filter.ts‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,43 @@ function malformedFieldReferenceError(path: string, detail: string): Error {
414414
return invalidFilterError(`The { "$field" } reference at ${path} is malformed: ${detail}`);
415415
}
416416

417+
/**
418+
* [#20123] A `having` KEY that names no column of the aggregated row — the
419+
* twin of {@link unresolvedFieldReferenceError} for the other place a column
420+
* is named.
421+
*
422+
* Refused on the filter, never answered per row: {@link checkCondition} reads
423+
* such a key as a column with NO VALUE in every group, so a typo for an alias
424+
* (`{ totl: { $gt: 100 } }`) kept no group and a negated test on it
425+
* (`$ne`, `$exists: false`, a `$not`) kept every group — an answer
426+
* indistinguishable from a real one. It is the same mistake the REST
427+
* ingress refuses on `where` for a field the object does not have, and the
428+
* opening words follow that refusal's ("filters on 'X', which is not a …";
429+
* "the query was refused instead of answered"). The code stays this face's
430+
* own `INVALID_FILTER`: the name is a column of the QUERY's own projection,
431+
* not a field of the object, which is what `INVALID_FIELD` answers about.
432+
*
433+
* Every unknown key is named, the first with its position, and the column
434+
* list is printed because it is the author's own query, and it is the answer.
435+
*/
436+
function unknownHavingColumnError(
437+
unknown: ReadonlyArray<{ key: string; path: string }>,
438+
columns: readonly string[],
439+
): Error {
440+
const [first] = unknown;
441+
const others = [...new Set(unknown.map((u) => u.key))].filter((key) => key !== first.key);
442+
const also = others.length ? ` (also: ${others.join(', ')})` : '';
443+
const known = columns.length ? columns.join(', ') : '(none — the query projects no column)';
444+
return invalidFilterError(
445+
`\`having\` filters on '${first.key}' at ${first.path}, which is not a column of the aggregated `
446+
+ `row${also}. A condition on a column the row does not carry is answered as if that column had no `
447+
+ `value in every group — a test for a value keeps no group, and a test for its absence or a `
448+
+ `negation keeps every group — so the query was refused instead of answered. \`having\` filters `
449+
+ `the aggregated row's own columns — the groupBy projections and the aggregation aliases — which `
450+
+ `here are: ${known}.`,
451+
);
452+
}
453+
417454
/**
418455
* [#5905] Operators whose answer for a column with NO VALUE is decided by the
419456
* operator's own arm below, not by the early exit in {@link checkCondition}.
@@ -496,13 +533,22 @@ export function assertHavingIsFilterCondition(having: unknown): void {
496533
* - a reference outside the six scalar comparisons —
497534
* {@link fieldReferencePositionError};
498535
* - a reference its declaration refuses (a malformed `addDays`), or one naming
499-
* no column of the aggregated row — {@link unresolvedFieldReferenceError}.
536+
* no column of the aggregated row — {@link unresolvedFieldReferenceError};
537+
* - [#20123] and, once the whole clause has passed those, a KEY naming no
538+
* column of the aggregated row, at any depth —
539+
* {@link unknownHavingColumnError}. Last on purpose: a condition on a column
540+
* the row does not carry is still read for its operator first (the #20099
541+
* rule above), so the refusal an author meets for `{ nope: { $median: 1 } }`
542+
* is the operator's, and fixing it does not reveal a second one they could
543+
* have been told about already.
500544
*
501545
* Read-only; `having` is assumed to have passed
502546
* {@link assertHavingIsFilterCondition}.
503547
*/
504548
export function assertHavingIsEvaluable(having: unknown, columns: readonly string[]): void {
505-
assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns });
549+
const unknownKeys: Array<{ key: string; path: string }> = [];
550+
assertNodeIsEvaluable(having, 'having', { clause: HAVING_CLAUSE, columns, unknownKeys });
551+
if (unknownKeys.length > 0) throw unknownHavingColumnError(unknownKeys, columns);
506552
}
507553

508554
/**
@@ -537,16 +583,18 @@ export function assertAggregationFilterIsEvaluable(filter: unknown, index: numbe
537583
/**
538584
* [#20122] What one row-independent walk judges against: the clause whose
539585
* words its refusals use, and — where the position has a closed namespace —
540-
* the column set a `{ $field }` reference must name.
586+
* the column set a key and a `{ $field }` reference must name.
541587
*/
542588
interface EvaluableScope {
543589
clause: FilterClause;
544590
/**
545-
* The names a `{ $field }` reference may take. `undefined` when the position
546-
* has no closed column set to judge a name against (the per-aggregation
547-
* filter reads the object's raw columns).
591+
* The names a key and a `{ $field }` reference may take. `undefined` when
592+
* the position has no closed column set to judge a name against (the
593+
* per-aggregation filter reads the object's raw columns).
548594
*/
549595
columns?: readonly string[];
596+
/** [#20123] Keys naming none of `columns`, collected in walk order. */
597+
unknownKeys?: Array<{ key: string; path: string }>;
550598
}
551599

552600
/** One node: the `$and` / `$or` / `$not` walk {@link matchesHaving} takes. */
@@ -565,6 +613,9 @@ function assertNodeIsEvaluable(cond: unknown, path: string, scope: EvaluableScop
565613
}
566614
if (key.startsWith('$')) throw unknownOperator(key, 'logical', [], scope.clause);
567615
assertConditionIsEvaluable(value, key, here, scope);
616+
// [#20123] Rows carry exactly the columns the query projects, so a key
617+
// outside them can only ever read "no value" — see unknownHavingColumnError.
618+
if (scope.columns && !scope.columns.includes(key)) scope.unknownKeys?.push({ key, path: here });
568619
}
569620
}
570621

0 commit comments

Comments
 (0)