Skip to content

Commit 211cfba

Browse files
committed
fix(objectql)!: a per-aggregation filter that is an array is refused, [] included (#20122)
Seat amendment 2 (ruling A on the PR's open question). The shape gate on `aggregations[i].filter` no longer exempts an array: the slot is declared `FilterConditionSchema`, which admits no array form, and the REST door already refuses every array there (VALIDATION_FAILED). In-process, a condition array counted no row (its index positions read as column names) and `[]` read as no filter. An array of any length is now refused with INVALID_FILTER / 400 before any driver call, in words naming the object form; `undefined`, `null`, a plain object and a null-prototype object are unchanged. The non-array refusal now reads "must be a filter object" as well, since a condition array is not accepted here. The held array control becomes three refusal rows. Claude-Session: https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 796b06a commit 211cfba

3 files changed

Lines changed: 55 additions & 27 deletions

File tree

‎.changeset/20122-aggregation-filter-row-independent.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine
66

77
Clause-②: no (narrowing)
88

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. -->
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. An array filter is refused because the slot's declared type, FilterConditionSchema, admits no array form, exactly as `having`'s condition check already refuses one, and the REST door already refused it. 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. -->
1010

1111
**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.
1212

@@ -22,12 +22,13 @@ The engine evaluates a per-aggregation filter itself, in the in-memory fallback,
2222
| 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` |
2323
| `{ 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` |
2424
| 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-
| a `filter` that is not a filter object: a string (`"stage = 'won'"`), a number, `true` / `false`, `''`, a `Map`, a `Date` or a `Set` | dropped: the aggregation read every row of its group, with no error. `driver-sql`'s native aggregate answered a non-empty string with `NOT_IMPLEMENTED` / 501. (The REST door already refused the JSON-expressible ones, through `AggregationNodeSchema`.) Now refused by the shape gate `where` takes, in `where`'s words with the aggregation named (`'aggregations[1].filter' must be a filter object or condition array, received …`) | a filter object, `{ stage: 'won' }` |
25+
| 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' }` |
26+
| 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 |
2627

2728
Not refused, but answering differently:
2829

2930
- **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.
3031

31-
Who is affected: a per-aggregation filter reaches `engine.aggregate` from a direct engine call, from the REST aggregate query, and from the analytics service, which lowers a dataset measure's own `filter` onto it. On a populated table the refusals in the first and fourth rows above were already raised; what changes there is that an empty table refuses them too. The shape row is reachable in-process only: the REST door already refuses a non-object `filter`. Callers in a deployment were NOT measured.
32+
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.
3233

33-
Not changed: implicit equality, scalar ordering bounds, `$in` / `$nin` lists, a two-bound `$between`, `$icontains` / `$startsWith` with a non-empty string, `$ne: null`, `$exists`, `$null`, `$or` / `$not` composition, a `{ $field }` reference as the whole comparand of a scalar comparison (with or without a whole-day `addDays`), an exact bigint in the implicit slot, and `{}`, on both drivers, measured identical before and after. The zero-row filter `{ $not: {} }`, which the analytics service lowers FALSE to, still counts no row (a unit pin, green against the base code too). `null`, an absent `filter`, a null-prototype filter object and an array are not refused by the shape gate, as they are not on `where`: `[]` is still read as no filter, and a condition array (`[['amount', '>', 100]]`) is still read by the walker as an object whose index keys are column names, so it counts no row, as before — `AggregationNodeSchema.filter` does not declare that sugar, and the REST door refuses it.
34+
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.

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

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,7 @@ describe('[#20122] per-aggregation filter — the shape gate `where` takes: a fi
542542
for (const [name, filter, received] of SHAPE_REFUSED) {
543543
it(`${name}: refused before any read, whatever the rows`, async () => {
544544
const message = await expectFilterRefusal(filter);
545-
expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object or condition array, ${received}.`);
545+
expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object, ${received}.`);
546546
expect(message).toContain('It was not applied');
547547
});
548548
}
@@ -557,20 +557,23 @@ describe('[#20122] per-aggregation filter — the shape gate `where` takes: a fi
557557
}
558558
});
559559

560-
it('an array is not this gate\'s to refuse, as it is not on `where` — `[]` and a FilterArray answer as before', async () => {
561-
// Held, not endorsed: the FilterArray's answer is the walker's reading of an
562-
// array (its index keys read as column names, so no row matches) — the
563-
// pre-existing reading, left for its own decision; `[]` is the vacuous
564-
// filter. What this pins is that the shape gate moves neither.
565-
for (const native of [true, false]) {
566-
const { driver } = makeCountingDriver(OPPORTUNITIES, native);
567-
const engine = await makeEngine(driver);
568-
expect(await engine.aggregate('crm_opportunity', withFilter([['amount', '>', 100]]))).toEqual([{ opp_count: 6, picked: 0 }]);
569-
}
570-
// `[]` is read as no filter and so keeps the native push-down; the stand-in's
571-
// native door computes nothing, so it is read on the fallback door here.
572-
const { driver } = makeCountingDriver(OPPORTUNITIES, false);
573-
const engine = await makeEngine(driver);
574-
expect(await engine.aggregate('crm_opportunity', withFilter([]))).toEqual([{ opp_count: 6, picked: 6 }]);
575-
});
560+
// [#20122, seat ruling A on the array question] An ARRAY is refused too, `[]`
561+
// included: the slot is declared `FilterConditionSchema`, which admits no
562+
// array form, and the REST door refuses every array there already
563+
// (`VALIDATION_FAILED`). Before, in-process, a condition array counted NO row
564+
// (the walker read its index positions as column names) and `[]` read as no
565+
// filter — neither is what the declaration allows.
566+
const ARRAY_REFUSED: ReadonlyArray<readonly [string, () => unknown, string]> = [
567+
['an empty array', () => [], 'received an array ([])'],
568+
['a condition array', () => [['amount', '>', 100]], 'received an array ([["amount",">",100]])'],
569+
['a logical-group array', () => ['and', ['stage', '=', 'closed_won'], ['amount', '>', 100]], 'received an array (["and",'],
570+
];
571+
572+
for (const [name, filter, received] of ARRAY_REFUSED) {
573+
it(`${name}: refused before any read, whatever the rows, naming the object form`, async () => {
574+
const message = await expectFilterRefusal(filter);
575+
expect(message).toContain(`aggregate('crm_opportunity'): '${AT}' must be a filter object, ${received}`);
576+
expect(message).toContain('input-only sugar');
577+
});
578+
}
576579
});

‎packages/objectql/src/engine.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16092,20 +16092,44 @@ export class ObjectQL implements IObjectQLEngine {
1609216092
if (aggFilter == null) continue;
1609316093
// [#20122] The SHAPE gate `where` takes first on its own seam
1609416094
// (`lowerWhereFilterArray`, #20121) — first here too, with its
16095-
// accept set and its words. A filter that is not a filter object
16095+
// object test (`isWhereFilterObject`) and its words, minus the
16096+
// array half (below). A filter that is not a filter object
1609616097
// (a string, a number, a boolean, `''`, a `Map`, a `Date`) was
1609716098
// stepped around by every door below, each of which walks a filter
1609816099
// node's keys, and then dropped: the fork below reads it as "no
1609916100
// filter", so the aggregation read every row of its group
1610016101
// (`driver-sql`'s native aggregate answered a non-empty string
1610116102
// with a 501 instead). The wire door already refuses these shapes
1610216103
// through `AggregationNodeSchema`; this closes the in-process one.
16103-
// An array is left to the doors below, as `where` leaves one to
16104-
// its array branch.
16105-
if (!Array.isArray(aggFilter) && !isWhereFilterObject(aggFilter)) {
16104+
//
16105+
// An ARRAY is refused too, `[]` included — unlike `where`, which
16106+
// lowers the condition-array sugar in its array branch. The slot
16107+
// is declared `FilterConditionSchema` (an object, no array form),
16108+
// the wire door refuses every array here (`VALIDATION_FAILED`),
16109+
// and in-process a condition array counted no row (the walker
16110+
// read its index positions as column names) while `[]` read as
16111+
// no filter. Same ruling as `having`'s condition check (#20099).
16112+
if (Array.isArray(aggFilter)) {
16113+
let shown: string;
16114+
try {
16115+
shown = JSON.stringify(aggFilter) ?? String(aggFilter);
16116+
} catch {
16117+
shown = String(aggFilter);
16118+
}
16119+
if (shown.length > 80) shown = `${shown.slice(0, 77)}...`;
16120+
throw invalidFilterError(
16121+
`aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object, received ` +
16122+
`an array (${shown}). The condition-array form — [field, operator, value] tuples and ` +
16123+
`["and", …] groups — is input-only sugar lowered on 'where' alone; a per-aggregation ` +
16124+
`filter is declared as a filter condition object and does not take it, so an array here ` +
16125+
`was never applied as the filter it spells. Write the object form, ` +
16126+
`{ "FIELD": { "$gt": 100 } }, or omit 'filter' for no filter.`,
16127+
);
16128+
}
16129+
if (!isWhereFilterObject(aggFilter)) {
1610616130
throw invalidFilterError(
16107-
`aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object or condition ` +
16108-
`array, received ${describeNonFilterWhere(aggFilter)}. It was not applied, and an ` +
16131+
`aggregate('${object}'): 'aggregations[${i}].filter' must be a filter object, ` +
16132+
`received ${describeNonFilterWhere(aggFilter)}. It was not applied, and an ` +
1610916133
`unapplied filter would have aggregated every row of each group for that aggregation.`,
1611016134
);
1611116135
}

0 commit comments

Comments
 (0)