Skip to content

Commit fd30845

Browse files
committed
fix(objectql): applyInMemoryAggregation judges the JSON-column rule on each filter before any row, as engine.aggregate does
Given a field map, the published applyInMemoryAggregation reached the per-row backstop without the one-time gate, so an empty row set and a row the lowered filter's $null arm decided first answered 200 where engine.aggregate refuses 400. It now calls assertAggregationFilterSparesJsonStoredFields (exported from having-filter.ts, the same function) once per aggregations[i].filter, and takes an optional reportWithheld for the withheld diagnostic. The floor's docblock now says what it is: a backstop for a row that reaches the arm. Claude-Session: https://claude.ai/code/session_01Ujdtvqs7ree7WyQmEDwEnG Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2f38024 commit fd30845

3 files changed

Lines changed: 145 additions & 9 deletions

File tree

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,10 @@
2929
// `aggregation-filter-json-column-refusal.test.ts`.
3030

3131
import { describe, it, expect, vi } from 'vitest';
32+
import { lowerFilterCondition } from '@objectstack/spec/data';
3233
import { ObjectQL } from './engine.js';
3334
import { declaredJsonStoredFields, matchesAggregationFilter } from './having-filter.js';
35+
import { applyInMemoryAggregation } from './in-memory-aggregation.js';
3436

3537
const OBJECT = 'os21007_doc';
3638

@@ -249,3 +251,86 @@ describe('[#21007] the per-row floor — a caller evaluating rows directly meets
249251
expect(matchesAggregationFilter(ROWS[0], { owners: { $in: ['u1'] } } as never, 0)).toBe(false);
250252
});
251253
});
254+
255+
describe('[#21007] applyInMemoryAggregation — a direct caller with a field map meets the same refusal, before any row is judged', () => {
256+
// `applyInMemoryAggregation` is published (objectql's root and `./core`), so a
257+
// host can reach the walker with a field map and no engine in front of it.
258+
// It calls the engine's own gate once per filter; without that call the
259+
// per-row backstop answered both cells below 200 — an empty row set has no
260+
// row to reach it, and on the LOWERED filter (the shape every engine seam
261+
// hands the evaluator: spec `lowerFilterCondition`, rule 3) a row with no
262+
// value is decided by the `$null` arm before the comparison is walked.
263+
const NEGATIONS: ReadonlyArray<readonly [string, Record<string, unknown>, string]> = [
264+
['$ne', { meta: { $ne: 'a' } }, '$ne'],
265+
['$nin', { meta: { $nin: ['a', 'b'] } }, '$nin'],
266+
['$not $in', { $not: { meta: { $in: ['a'] } } }, '$in'],
267+
];
268+
const NULL_META_ROWS = ROWS.map((row) => ({ ...row, meta: null }));
269+
const ast = (filter: unknown, groupBy?: string[]) => ({
270+
...(groupBy ? { groupBy } : {}),
271+
aggregations: [
272+
{ function: 'count' as const, alias: 'n' },
273+
{ function: 'count' as const, alias: 'm', filter: filter as never },
274+
],
275+
});
276+
277+
function refusalFrom(run: () => unknown): Error & { code?: string; status?: number } {
278+
try {
279+
run();
280+
} catch (e) {
281+
return e as Error & { code?: string; status?: number };
282+
}
283+
throw new Error('expected applyInMemoryAggregation to refuse this filter, but it answered');
284+
}
285+
286+
async function engineMessageFor(filter: unknown): Promise<string> {
287+
const { engine } = await makeEngine([]);
288+
return (await refusalOf(() => engine.aggregate(OBJECT, perAggregation(filter)))).message;
289+
}
290+
291+
for (const [name, filter, op] of NEGATIONS) {
292+
const lowered = lowerFilterCondition(filter);
293+
294+
it(`${name} — the lowered filter carries the $null arm the walker short-circuits on`, () => {
295+
expect(JSON.stringify(lowered)).toContain('"$null"');
296+
});
297+
298+
const cells: ReadonlyArray<{ cell: string; rows: Record<string, unknown>[]; spelled: unknown; groupBy?: string[] }> = [
299+
{ cell: 'an EMPTY row set', rows: [], spelled: filter },
300+
{ cell: 'an EMPTY row set, grouped', rows: [], spelled: filter, groupBy: ['title'] },
301+
{ cell: 'meta null in every row, the lowered filter', rows: NULL_META_ROWS, spelled: lowered },
302+
{ cell: 'meta null in every row, the filter as written', rows: NULL_META_ROWS, spelled: filter },
303+
];
304+
for (const { cell, rows, spelled, groupBy } of cells) {
305+
it(`${name} on ${cell}: 400 INVALID_FILTER, engine.aggregate's very message, the diagnostic to reportWithheld`, async () => {
306+
const reported: string[] = [];
307+
const err = refusalFrom(() => applyInMemoryAggregation(
308+
rows.map((row) => ({ ...row })), ast(spelled, groupBy), undefined, FIELDS,
309+
(diagnostic) => { reported.push(diagnostic); },
310+
));
311+
expect(err.code).toBe('INVALID_FILTER');
312+
expect(err.status).toBe(400);
313+
expect(err.message).toBe(await engineMessageFor(filter));
314+
expect(err.message).toContain('{ "FIELD": { "$contains": "a" } }');
315+
expect(err.message).not.toContain('"meta"');
316+
expect(reported.join('\n')).toContain(`Operator "${op}" on field "meta" WAS NOT APPLIED`);
317+
expect(reported.join('\n')).toContain('At aggregations[1].filter.');
318+
});
319+
}
320+
}
321+
322+
it('no reportWithheld: the same refusal, the diagnostic dropped', () => {
323+
const err = refusalFrom(() => applyInMemoryAggregation([], ast({ owners: { $in: ['u1'] } }), undefined, FIELDS));
324+
expect(err.code).toBe('INVALID_FILTER');
325+
expect(err.status).toBe(400);
326+
});
327+
328+
it('no field map: nothing is judged, and the walker answers as it always did', () => {
329+
expect(applyInMemoryAggregation(ROWS.map((row) => ({ ...row })), ast({ owners: { $in: ['u1'] } }))).toEqual([{ n: 6, m: 0 }]);
330+
});
331+
332+
it('what still answers on a declared JSON-stored field answers here too', () => {
333+
expect(applyInMemoryAggregation(ROWS.map((row) => ({ ...row })), ast({ owners: { $contains: 'u1' } }), undefined, FIELDS))
334+
.toEqual([{ n: 6, m: 2 }]);
335+
});
336+
});

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

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,11 +1042,26 @@ export function assertAggregationFilterIsEvaluable(
10421042
* here meet only declared operators. A plain object with no `$` key is not a
10431043
* condition this rule reads; the engine's no-operator-object door refuses it
10441044
* earlier. No usable field map (a registry-less host) ⇒ nothing is judged.
1045+
*
1046+
* THIS is the complete door. The per-row arm in {@link checkCondition} is a
1047+
* backstop that fires only on a row that reaches it, and many never do: an
1048+
* empty table has no row, and the spec's filter lowering (rule 3) rewrites a
1049+
* negation into `$or: [{ f: { $null: true } }, { f: { $ne: v } }]` (and gives a
1050+
* `$not` operand an `{ f: { $null: false } }` conjunct), so on a row with no
1051+
* value the `$null` arm decides and the walker never reaches the comparison.
1052+
* Measured with this call removed: a `json` field null in every row answered
1053+
* `$ne` / `$nin` / `$not $in` with every row counted. Hence [#21007]'s second
1054+
* caller, `applyInMemoryAggregation`, which a host may call with a field map
1055+
* and no engine in front of it — it calls this same function, once per
1056+
* aggregation, before any row is judged.
1057+
*
1058+
* `declared` needs only the field map and where the diagnostic goes; the
1059+
* object's name is the reference rule's, not this one's.
10451060
*/
1046-
function assertAggregationFilterSparesJsonStoredFields(
1061+
export function assertAggregationFilterSparesJsonStoredFields(
10471062
filter: unknown,
10481063
root: string,
1049-
declared: AggregationFilterDeclaration,
1064+
declared: Pick<AggregationFilterDeclaration, 'fields' | 'reportWithheld'>,
10501065
): void {
10511066
const jsonStored = declaredJsonStoredFields(declared.fields);
10521067
if (jsonStored.size === 0) return;
@@ -1637,11 +1652,16 @@ function storedArrayHasMember(value: unknown, comparand: unknown): boolean {
16371652
*
16381653
* [#21007] …and a scalar comparison on such a column — implicit equality, or an
16391654
* operator in `JSON_COLUMN_INCOMPATIBLE_OPERATORS` — is REFUSED, in `where`'s
1640-
* withheld words. The engine judges that once on the filter
1641-
* ({@link assertAggregationFilterSparesJsonStoredFields}) before any row is
1642-
* read, and reports the diagnostic there; this arm is the floor for a caller
1643-
* that evaluates rows directly. Above the no-value exit, for the #7158 reason:
1644-
* it is the filter's verdict, not the row's.
1655+
* withheld words. The COMPLETE door is
1656+
* {@link assertAggregationFilterSparesJsonStoredFields}, which judges the filter
1657+
* once, before any row is read, and reports the diagnostic; `engine.aggregate`
1658+
* and `applyInMemoryAggregation` both call it. This arm is only the BACKSTOP
1659+
* for a row that reaches it — a caller evaluating rows directly through
1660+
* `matchesAggregationFilter`. It sits above the no-value exit, so a row with no
1661+
* value that reaches it is refused too; but its REACH is the row's: an empty
1662+
* row set never gets here, and neither does a row on which a short-circuited
1663+
* `$or` / `$and` branch has already decided (the spec lowering's rule 3 puts a
1664+
* `$null` arm ahead of every negation).
16451665
*/
16461666
function checkCondition(
16471667
value: any,
@@ -1695,7 +1715,8 @@ function checkCondition(
16951715
throw emptyFlagComparandError(field, target, `${path}.${op}`);
16961716
}
16971717
// [#21007] A scalar comparison on a declared JSON-stored column — refused,
1698-
// as `where` refuses it. The floor under the engine's one-time judgment.
1718+
// as `where` refuses it. The backstop under the one-time judgment
1719+
// (assertAggregationFilterSparesJsonStoredFields), for a row that gets here.
16991720
if (jsonStored && JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op)) {
17001721
throw invalidFilterError(jsonColumnOperatorRefusalText(field, op, false).message);
17011722
}

‎packages/objectql/src/in-memory-aggregation.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,13 @@
8686

8787
import { bucketDateKey, compensatedSum } from '@objectstack/core';
8888
import type { QueryAST, GroupByNode, AggregationNode, DateGranularityValue } from '@objectstack/spec/data';
89-
import { declaredFieldClasses, declaredJsonStoredFields, matchesAggregationFilter } from './having-filter.js';
89+
import {
90+
aggregationFilterClause,
91+
assertAggregationFilterSparesJsonStoredFields,
92+
declaredFieldClasses,
93+
declaredJsonStoredFields,
94+
matchesAggregationFilter,
95+
} from './having-filter.js';
9096

9197
/**
9298
* Group + aggregate raw rows according to the AST's `groupBy` /
@@ -106,18 +112,42 @@ import { declaredFieldClasses, declaredJsonStoredFields, matchesAggregationFilte
106112
* [#20873] …and `$contains` / `$notContains` on a declared JSON-stored field ask
107113
* MEMBERSHIP (having-filter.ts `declaredJsonStoredFields`), the reading the
108114
* same condition gets in a `where`. Absent ⇒ the substring reading, as before.
115+
*
116+
* [#21007] …and a scalar comparison on such a field (`$in`, `$nin`, `$eq`, an
117+
* ordering, `$between`, implicit equality) is REFUSED `INVALID_FILTER` / 400,
118+
* in the words `where` refuses it in — judged once per `aggregations[i].filter`
119+
* by having-filter.ts `assertAggregationFilterSparesJsonStoredFields`, the same
120+
* function `engine.aggregate` calls, BEFORE any row is judged. So an empty
121+
* `rows` and a row set the per-row walk would short-circuit past refuse alike;
122+
* the per-row arm is only the backstop. `reportWithheld` is where the withheld
123+
* half of that refusal goes — the field, the operator and the position, which
124+
* the message does not name. This entry point holds no logger of its own, so a
125+
* host that wants the diagnostic passes its log here; absent ⇒ it is dropped,
126+
* and the caller still gets the refusal's code, status and prescription.
127+
* `engine.aggregate` has judged the filter already (and logged it) before it
128+
* calls this, so it passes none.
109129
*/
110130
export function applyInMemoryAggregation(
111131
rows: any[],
112132
ast: Pick<QueryAST, 'groupBy' | 'aggregations'>,
113133
timezone?: string,
114134
fields?: Record<string, unknown>,
135+
reportWithheld?: (diagnostic: string) => void,
115136
): any[] {
116137
const groupBy = (ast.groupBy ?? []) as GroupByNode[];
117138
const aggregations = (ast.aggregations ?? []) as AggregationNode[];
118139
if (groupBy.length === 0 && aggregations.length === 0) return rows;
119140
// [#20176] Read once per call, and only when some aggregation carries a filter.
120141
const anyFilter = aggregations.some((a) => a?.filter && Object.keys(a.filter).length > 0);
142+
// [#21007] The JSON-column rule, judged on each FILTER before any row is —
143+
// the complete door, not the per-row backstop. See the docblock above.
144+
if (fields && anyFilter) {
145+
const declared = { fields, reportWithheld: reportWithheld ?? (() => undefined) };
146+
for (const [index, agg] of aggregations.entries()) {
147+
if (!agg?.filter || Object.keys(agg.filter).length === 0) continue;
148+
assertAggregationFilterSparesJsonStoredFields(agg.filter, aggregationFilterClause(index).root, declared);
149+
}
150+
}
121151
const filterClasses = fields && anyFilter ? declaredFieldClasses(fields) : undefined;
122152
// [#20873] Read once per call too, from the same declaration.
123153
const filterJsonStored = fields && anyFilter ? declaredJsonStoredFields(fields) : undefined;

0 commit comments

Comments
 (0)