diff --git a/.changeset/20981-aggregation-flag-comparand-refused.md b/.changeset/20981-aggregation-flag-comparand-refused.md new file mode 100644 index 00000000000..c93e98e3b04 --- /dev/null +++ b/.changeset/20981-aggregation-flag-comparand-refused.md @@ -0,0 +1,21 @@ +--- +'@objectstack/objectql': minor +--- + +fix(objectql)!: a per-aggregation `filter` and a `having` refuse a non-boolean `$exists` / `$null` with `INVALID_FILTER` / 400, in the words every driver's `where` refuses it in, instead of reading `$exists` by truthiness and dropping `$null` (#20981) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what `aggregate` accepts in two positions, `aggregations[i].filter` and `having`, on every driver. A `$exists` or `$null` comparand that is not a boolean (a string such as `"false"`, a number, `null`, an array) is now refused with `INVALID_FILTER` / 400, before any driver is asked for a row, so an empty table refuses it too, at any depth under `$and` / `$or` / `$not`. A plain object or `undefined` there is refused first by the comparand-type check, in its own words, as before; a `{ $field }` reference there, already refused as a reference outside a scalar comparison, is now refused in this entry's words. The published `applyInMemoryAggregation(rows, ast, timezone, fields)` narrows the same way, per row: it throws the same refusal for a row its per-aggregation filter judges on the flag, with or without a `fields` map (an empty `rows` array, or a row a `$or` branch settles first, is not judged there; `engine.aggregate` judges the whole filter once before any row). It ships as `minor` under the launch-window convention for accept-set narrowings. No export or published type changes. + +**Why a refusal.** `FieldOperatorsSchema` declares both flags as booleans, and every driver's `where` refuses any other comparand. The engine evaluates a per-aggregation `filter` and a `having` itself, and it read one anyway. Measured through `engine.aggregate` on the in-memory driver and on `SqlDriver` (SQLite), with identical answers: `$exists` was read by truthiness, so `"yes"`, `1` and the string `"false"` selected the rows and groups WITH a value, and `0` / `null` the ones without; and `$null` tested only `true` / `false`, so any other value constrained nothing, and every row and every group came back. + +**What an author sees now.** The message `driver-sql` gives the same flag, beginning `Operator "$exists" on field "FIELD" requires a boolean comparand (true or false).`, naming what arrived and the position (`aggregations[1].filter.stage.$exists`, `having.stage.$null`). Unlike a `where` on `SqlDriver`, the field and the value are not withheld: a per-aggregation `filter` and a `having` never carry a merged read scope. + +**What to write instead.** Write the boolean itself. `"$exists": true` and `"$null": false` match a field that has a value; `"$exists": false` and `"$null": true` match one that has none. + +**Who is affected.** A caller that reaches `engine.aggregate` without the REST query door's schema parse (server-side code, a flow or hook, the analytics bridge that lowers a dataset measure's filter into an aggregation filter, a host calling `applyInMemoryAggregation` directly) and read the count as a real answer. `POST /api/v1/data/:object/query` already refused all three positions with 400 `VALIDATION_FAILED` before the request reached the engine, and still does. + +**Unchanged.** `$exists: true` / `false` and `$null: true` / `false` answer exactly as before, on both positions. `$empty` and every other operator, and `where`. diff --git a/packages/objectql/src/engine-aggregate-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index f9700d89a4c..8e4242304f0 100644 --- a/packages/objectql/src/engine-aggregate-filter.test.ts +++ b/packages/objectql/src/engine-aggregate-filter.test.ts @@ -439,6 +439,8 @@ describe('[#20122] per-aggregation filter — the walker\'s refusals belong to t ['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`], + // [#20981] Refused at the same path, as a non-boolean flag first now — the + // words are pinned in engine-aggregate-flag-comparand-refusal.test.ts. ['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`], diff --git a/packages/objectql/src/engine-aggregate-flag-comparand-refusal.test.ts b/packages/objectql/src/engine-aggregate-flag-comparand-refusal.test.ts new file mode 100644 index 00000000000..9b833b2b3cf --- /dev/null +++ b/packages/objectql/src/engine-aggregate-flag-comparand-refusal.test.ts @@ -0,0 +1,284 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#20981] A per-aggregation `filter` and a `having` REFUSE a non-boolean +// `$exists` / `$null` with `INVALID_FILTER` / 400, in the words every driver's +// `where` refuses it in, before any driver is asked. `true` / `false` answer +// exactly as before. +// +// Measured before this change (`origin/main` `7a606a9a3`) through +// `engine.aggregate` on driver-memory AND driver-sql (better-sqlite3), over a +// text column `name` holding `'won'` on one row (amount 10) and no value on two +// (amounts 1 and 100). The engine evaluates both clauses itself, so the two +// drivers answered identically: +// +// | clause · comparand | `$exists` | `$null` | +// |:--|:--|:--| +// | filter · `"yes"` / `1` / `"false"` | sum 10 (the valued row) | sum 111 (every row) | +// | filter · `0` / `null` | sum 101 (the no-value rows) | sum 111 (every row) | +// | having · `"yes"` / `1` / `"false"` | the `won` group | both groups | +// | having · `0` / `null` | the null group | both groups | +// +// while the same flag in a `where` is refused 400 on both drivers. After it, +// every cell above is that 400, and the `true` / `false` control keeps the +// numbers below. +// +// This file is the engine-level cell, over a driver of each `having` path's +// shape — `rows` (no `aggregate()`: the engine reads rows and lowers in memory, +// the face that evaluates `aggregations[i].filter`) and `native` (the driver +// aggregates, the engine applies `having`). Every refusal precedes the driver, +// so the backend does not enter into it; the same answers were measured on +// driver-memory and driver-sql above. This package does not depend on either +// driver, and the in-memory driver's test consumers are a closed census +// (`check:driver-memory-census`), so the SQLite cell — beside its live `where` +// twin, through `POST /data/:object/query` — is `packages/rest`'s +// `aggregation-flag-comparand-refusal.test.ts`. + +import { describe, it, expect } from 'vitest'; +import type { EngineAggregateOptions, FilterCondition } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; +import { applyHaving, matchesAggregationFilter } from './having-filter.js'; +import { applyInMemoryAggregation } from './in-memory-aggregation.js'; + +const OBJECT = 'os20981_deal'; + +const FIELDS = { + name: { type: 'text' }, + amount: { type: 'number' }, +}; + +// `a` holds a value; `b` holds null and `c` does not carry the column — both +// have NO value, the reading `$exists` and `$null` share (#5298). +const ROWS = [ + { id: 'a', name: 'won', amount: 10 }, + { id: 'b', name: null, amount: 1 }, + { id: 'c', amount: 100 }, +]; + +type Path = 'native' | 'rows'; + +/** A driver of one `having` path's shape, counting every read of the object. */ +function makeDriver(path: Path, rows: ReadonlyArray>) { + const reads = { aggregate: 0, find: 0 }; + const driver: any = { + name: `${path}-recorder`, + version: '0.0.0', + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { reads.find += 1; return rows.map((r) => ({ ...r })); }, + async findOne() { return 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 { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + if (path === 'native') { + driver.aggregate = async (_o: string, ast: any) => { reads.aggregate += 1; return applyInMemoryAggregation([...rows], ast); }; + } + return { driver, reads }; +} + +async function makeEngine(path: Path, rows: ReadonlyArray> = ROWS) { + const { driver, reads } = makeDriver(path, rows); + const engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + (engine.registry as any).registerObject({ name: OBJECT, fields: FIELDS }); + return { engine, reads }; +} + +/** `s` sums `amount` over the rows `filter` selects; `n` counts every row. Always the rows path. */ +const filterQuery = (filter: unknown): EngineAggregateOptions => ({ + aggregations: [ + { function: 'count', alias: 'n' }, + { function: 'sum', field: 'amount', alias: 's', filter: filter as FilterCondition }, + ], +}); + +/** Groups by `name`; on the rows path a filtered aggregation forces the engine's own lowering. */ +const havingQuery = (path: Path, having: unknown): EngineAggregateOptions => ({ + groupBy: ['name'], + aggregations: path === 'native' + ? [{ function: 'count', alias: 'n' }] + : [{ function: 'count', alias: 'n' }, { function: 'count', alias: 'all', filter: { amount: { $gte: 0 } } }], + having: having as FilterCondition, +}); + +interface Refusal extends Error { code?: unknown; status?: unknown } + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e as Refusal; + } + throw new Error('expected engine.aggregate to refuse this clause, but it answered'); +} + +function syncRefusalOf(run: () => unknown): Refusal { + try { + run(); + } catch (e) { + return e as Refusal; + } + throw new Error('expected a refusal, but the clause was answered'); +} + +/** + * The comparand doors' refusal: the ADR-0112 identity, then the drivers' first + * three sentences — the operator, the field, what arrived (in the drivers' + * `describeFilterOperand (safeShapePreview)` rendering) and where, and the + * declaration — then the reason that names the backends. + */ +function expectFlagRefusal(err: Refusal, op: '$exists' | '$null', received: string, at: string): void { + expect(err.code, err.message).toBe('INVALID_FILTER'); + expect(err.status, err.message).toBe(400); + expect(err.message.startsWith( + `Operator "${op}" on field "name" requires a boolean comparand (true or false). ` + + `Received ${received} at ${at}. ` + + `@objectstack/spec FieldOperatorsSchema declares ${op} as a boolean. It is refused rather than coerced `, + ), err.message).toBe(true); + expect(err.message).toContain('OPPOSITE directions'); + expect(err.message).toContain('Note "false" the STRING is truthy'); +} + +const OPS = ['$exists', '$null'] as const; + +/** The card's five non-booleans, each with the rendering the drivers give it. */ +const NON_BOOLEANS: ReadonlyArray = [ + ['"yes"', 'yes', 'string ("yes")'], + ['1', 1, 'number (1)'], + ['"false" (truthy)', 'false', 'string ("false")'], + ['0', 0, 'number (0)'], + ['null', null, 'null (null)'], + // Not one of the card's five, but reaching the same gate: no earlier door + // refuses a list here, so it met the old `!!target` read like any other value. + ['[true] (a list)', [true], 'array ([true])'], +]; + +describe('[#20981] a non-boolean $exists / $null — refused before any driver read, on both positions', () => { + for (const op of OPS) { + for (const [label, comparand, received] of NON_BOOLEANS) { + it(`aggregations[1].filter ${op}: ${label} — 400, the drivers' words, on an empty and a populated table`, async () => { + for (const rows of [[], ROWS]) { + const { engine, reads } = await makeEngine('rows', rows); + const err = await refusalOf(() => engine.aggregate(OBJECT, filterQuery({ name: { [op]: comparand } }))); + expectFlagRefusal(err, op, received, `aggregations[1].filter.name.${op}`); + expect(reads, 'the verdict is the filter\'s: no row was read').toEqual({ aggregate: 0, find: 0 }); + } + }); + + it(`having ${op}: ${label} — 400, the drivers' words, on both having paths`, async () => { + for (const path of ['native', 'rows'] as const) { + for (const rows of [[], ROWS]) { + const { engine, reads } = await makeEngine(path, rows); + const err = await refusalOf(() => engine.aggregate(OBJECT, havingQuery(path, { name: { [op]: comparand } }))); + expectFlagRefusal(err, op, received, `having.name.${op}`); + expect(reads, `${path}: no row was read`).toEqual({ aggregate: 0, find: 0 }); + } + } + }); + } + } + + // Where the flag sits must not change the verdict: the walk is row-independent, + // so a branch the per-row walk would short-circuit past is judged too. + const POSITIONS: ReadonlyArray) => Record, string]> = [ + ['nested in $and', (flag) => ({ $and: [{ amount: { $gte: 0 } }, { name: flag }] }), '.$and[1].name'], + ['behind a $or branch that already holds', (flag) => ({ $or: [{ amount: { $gte: 0 } }, { name: flag }] }), '.$or[1].name'], + ['under $not', (flag) => ({ $not: { name: flag } }), '.$not.name'], + ['beside a boolean sibling flag', (flag) => ({ name: { $empty: false, ...flag } }), '.name'], + ]; + + for (const [name, wrap, tail] of POSITIONS) { + it(`${name}: refused at that position, in the per-aggregation filter and in having`, async () => { + const filterEngine = await makeEngine('rows'); + expectFlagRefusal( + await refusalOf(() => filterEngine.engine.aggregate(OBJECT, filterQuery(wrap({ $exists: 'yes' })))), + '$exists', 'string ("yes")', `aggregations[1].filter${tail}.$exists`, + ); + // `having` reads the aggregated row, which carries `name` (the groupBy) + // but not `amount` — so its wrapper's other branch tests `n` instead. + const having = JSON.parse(JSON.stringify(wrap({ $null: 1 })).replaceAll('"amount"', '"n"')); + const havingEngine = await makeEngine('native'); + expectFlagRefusal( + await refusalOf(() => havingEngine.engine.aggregate(OBJECT, havingQuery('native', having))), + '$null', 'number (1)', `having${tail}.$null`, + ); + expect(filterEngine.reads.find + havingEngine.reads.aggregate).toBe(0); + }); + } + + it('a { $field } reference in a flag\'s slot is a non-boolean first, as in $empty\'s', async () => { + const { engine } = await makeEngine('rows'); + expectFlagRefusal( + await refusalOf(() => engine.aggregate(OBJECT, filterQuery({ name: { $exists: { $field: 'amount' } } }))), + '$exists', 'object ({"$field":"amount"})', 'aggregations[1].filter.name.$exists', + ); + const native = await makeEngine('native'); + expectFlagRefusal( + await refusalOf(() => native.engine.aggregate(OBJECT, havingQuery('native', { name: { $null: { $field: 'n' } } }))), + '$null', 'object ({"$field":"n"})', 'having.name.$null', + ); + }); +}); + +describe('[#20981] the control — true / false answer exactly as before, on both positions and both paths', () => { + // `a` (10) has a value; `b` (1) and `c` (100) have none. + const SUMS: ReadonlyArray, number]> = [ + ['$exists: true', { $exists: true }, 10], + ['$exists: false', { $exists: false }, 101], + ['$null: true', { $null: true }, 101], + ['$null: false', { $null: false }, 10], + ]; + + for (const [name, flag, sum] of SUMS) { + it(`aggregations[1].filter ${name} sums ${sum}`, async () => { + const { engine, reads } = await makeEngine('rows'); + expect(await engine.aggregate(OBJECT, filterQuery({ name: flag }))).toEqual([{ n: 3, s: sum }]); + expect(reads.find).toBe(1); + }); + + const kept = sum === 10 ? ['won'] : ['null']; + it(`having ${name} keeps the ${kept[0]} group on both paths`, async () => { + for (const path of ['native', 'rows'] as const) { + const { engine } = await makeEngine(path); + const groups = await engine.aggregate(OBJECT, havingQuery(path, { name: flag })); + expect(groups.map((g: any) => String(g.name)), path).toEqual(kept); + } + }); + } +}); + +describe('[#20981] the published row evaluators refuse per row — the floor for a caller that evaluates rows itself', () => { + it('applyInMemoryAggregation with fields: $exists "yes" and $null 1 are refused, true / false answer', () => { + for (const [op, comparand, received] of [['$exists', 'yes', 'string ("yes")'], ['$null', 1, 'number (1)']] as const) { + const err = syncRefusalOf(() => applyInMemoryAggregation( + ROWS.map((r) => ({ ...r })), + filterQuery({ name: { [op]: comparand } }), + undefined, + FIELDS, + )); + expectFlagRefusal(err, op, received, `aggregations[1].filter.name.${op}`); + } + expect(applyInMemoryAggregation(ROWS.map((r) => ({ ...r })), filterQuery({ name: { $exists: true } }), undefined, FIELDS)) + .toEqual([{ n: 3, s: 10 }]); + expect(applyInMemoryAggregation(ROWS.map((r) => ({ ...r })), filterQuery({ name: { $null: true } }), undefined, FIELDS)) + .toEqual([{ n: 3, s: 101 }]); + }); + + it('applyHaving and matchesAggregationFilter refuse a row that reaches the flag, with or without a value', () => { + const groups = [{ name: 'won', n: 1 }, { name: null, n: 2 }]; + for (const row of groups) { + expectFlagRefusal(syncRefusalOf(() => applyHaving([row], { name: { $exists: 'false' } } as never)), + '$exists', 'string ("false")', 'having.name.$exists'); + expectFlagRefusal(syncRefusalOf(() => matchesAggregationFilter(row, { name: { $null: 0 } } as never, 2)), + '$null', 'number (0)', 'aggregations[2].filter.name.$null'); + } + expect(applyHaving(groups, { name: { $null: false } })).toEqual([{ name: 'won', n: 1 }]); + expect(applyHaving(groups, { name: { $exists: false } })).toEqual([{ name: null, n: 2 }]); + }); +}); 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 9b96dad2d64..cfc9f86cf01 100644 --- a/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts +++ b/packages/objectql/src/engine-aggregate-having-comparand-shape.test.ts @@ -646,6 +646,8 @@ describe('[#20099] having — a { $field } reference resolves against the aggreg ['a reference as a $nin member', () => ({ total: { $nin: [1, { $field: 'max_cap' }] } }), 'having.total.$nin'], ['a reference as a $contains pattern', () => ({ customer_id: { $contains: { $field: 'customer_id' } } }), 'having.customer_id.$contains'], ['a reference as a $startsWith pattern', () => ({ customer_id: { $startsWith: { $field: 'customer_id' } } }), 'having.customer_id.$startsWith'], + // [#20981] These two are refused at the same path as a non-boolean flag + // first now — the words are pinned in engine-aggregate-flag-comparand-refusal.test.ts. ['a reference under $exists', () => ({ total: { $exists: { $field: 'max_cap' } } }), 'having.total.$exists'], ['a reference under $null', () => ({ total: { $null: { $field: 'max_cap' } } }), 'having.total.$null'], ['a reference naming no column', () => ({ total: { $gt: { $field: 'nope' } } }), 'having.total.$gt'], diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index 09c7ccc4126..1a4df8715a8 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -130,6 +130,13 @@ // `{ owners: { $in: ['u1'] } }` counted 0 and `{ owners: { $nin: ['u1'] } }` // counted the very rows holding `u1`, where the same `where` is a 400 on every // SQL dialect. See {@link assertAggregationFilterSparesJsonStoredFields}. +// +// [#20981] …and on BOTH positions a non-boolean `$exists` / `$null` is refused +// `INVALID_FILTER` / 400 in the words every driver's `where` refuses it in, +// judged once before any row and per row as the floor, beside `$empty`'s gate. +// Before, `$exists` was read by truthiness (`"false"` kept the valued rows) and +// a third `$null` value constrained nothing. See +// {@link nonBooleanFlagComparandError}. import type { FilterCondition } from '@objectstack/spec/data'; // [#20099] The reference's own declaration, so a malformed `addDays` is refused @@ -258,7 +265,8 @@ const CONDITION_OPERATORS = [ * positions the SQL family compiles one in. Every other position (a list * member, a text pattern, `$exists` / `$null`) is refused by * {@link assertHavingIsEvaluable} rather than compared against the reference - * OBJECT, which matched nothing. + * OBJECT, which matched nothing. [#20981] The two flags' slot is refused as a + * non-boolean first ({@link nonBooleanFlagComparandError}), as `$empty`'s is. */ const REFERENCE_COMPARISON_OPERATORS: ReadonlySet = new Set([ '$eq', '$ne', '$gt', '$gte', '$lt', '$lte', @@ -361,8 +369,9 @@ function unknownOperator( * `FieldOperatorsSchema` declares `$empty: z.boolean()`: `true` asks for the * empty rows, `false` for their exact complement. A third value is refused * rather than read — this face refuses the malformations it can see, where a - * two-branch reading would silently constrain nothing (the lenient `$null` - * arm's standing hazard). + * two-branch reading would silently constrain nothing (the hazard the `$null` + * arm carried until [#20981] refused its third value too — see + * {@link nonBooleanFlagComparandError}). */ function emptyFlagComparandError(field: string, value: unknown, path: string): Error { const shown = JSON.stringify(value) ?? String(value); @@ -373,6 +382,94 @@ function emptyFlagComparandError(field: string, value: unknown, path: string): E ); } +/** + * [#20981] `$exists` or `$null` received a comparand that is not a boolean. + * + * `FieldOperatorsSchema` declares both flags `z.boolean()`, and every driver's + * `where` refuses a third value — the #5347 ruling for `$null`, applied to + * `$exists` by the 2026-08-06 ruling on #5298: `driver-sql` (with + * `driver-sqlite-wasm` and Turso local), Turso's remote transport, + * `driver-memory`, `driver-mongodb` and `service-analytics`. This face read one + * anyway, and the engine evaluates it itself, so the answer was the same on + * every driver. Measured through `engine.aggregate` on driver-memory and + * driver-sql (better-sqlite3), `origin/main` `7a606a9a3`, over a text column + * holding `'won'` on one row and no value on two: + * + * | comparand | `$exists` (the old `!!target` read) | `$null` (the old two-branch read) | + * |:--|:--|:--| + * | `"yes"`, `1`, `"false"` | the VALUED rows / the `won` group | every row and every group | + * | `0`, `null` | the no-value rows / the null group | every row and every group | + * + * So `$exists` answered by truthiness — `"false"` the string kept the valued + * side — and `$null` constrained nothing at all: the widening direction. + * + * ## The words are the comparand doors', not this face's + * + * Verbatim the diagnostic `driver-sql`'s `nonBooleanExistsComparandError` / + * `nonBooleanNullComparandError` build — the text `driver-memory` and + * `driver-mongodb` give on the wire — with its "this driver" clause re-aimed + * at the backend it names, as `driver-memory`'s copy re-aims it. One condition, + * one wording (#5240). The text has no importable home: each face spells it, + * this package cannot depend on a driver, and neither `@objectstack/core` nor + * the spec exports it — so this is a declared verbatim copy, the drivers' + * `describeFilterOperand` / `safeShapePreview` rendering of the received value + * included ({@link describeFlagOperand}), held to the drivers' first sentence + * by `packages/rest`'s `aggregation-flag-comparand-refusal.test.ts` beside the + * `where` twin rather than by an import. + * + * Unlike `driver-sql`'s `where`, the message is not withheld: no read scope is + * merged into a per-aggregation `filter` or a `having`, and this face's other + * comparand refusals (`$empty`, `$icontains`) name the field and the value too. + */ +function nonBooleanFlagComparandError(op: '$exists' | '$null', field: string, value: unknown, path: string): Error { + const head = `Operator "${op}" on field "${field}" requires a boolean comparand (true or false). ` + + `Received ${describeFlagOperand(value)} at ${path}. ` + + `@objectstack/spec FieldOperatorsSchema declares ${op} as a boolean. `; + if (op === '$exists') { + return invalidFilterError( + head + + `It is refused rather than coerced for the same reason $null is: a non-boolean lands on whichever side ` + + `the backend's two-branch conditional happens to default to, and those defaults point in ` + + `OPPOSITE directions — driver-sql's \`=== false\` test compiles IS NOT NULL for anything ` + + `but false, a \`=== true\` test compiles IS NULL for anything but true. Note "false" the ` + + `STRING is truthy, so it lands on the side opposite the false it was written to mean.`, + ); + } + return invalidFilterError( + head + + `It is refused rather than coerced because the backends read a non-boolean in OPPOSITE directions — ` + + `driver-sql compiled IS NULL (anything but false), driver-memory's query path and driver-mongodb ` + + `compiled IS NOT NULL (anything but true), and driver-memory's matcher dropped the ` + + `constraint entirely. Note "false" the STRING is truthy, so it landed on the side opposite ` + + `the false it was written to mean.`, + ); +} + +/** + * [#20981] The drivers' rendering of a received comparand — `describeFilterOperand` + * then `safeShapePreview` in parentheses (`string ("yes")`, `number (1)`, + * `null (null)`), verbatim, so {@link nonBooleanFlagComparandError} reads byte + * for byte as the `where` refusal of the same flag does. + */ +function describeFlagOperand(value: unknown): string { + let kind: string; + if (value === null) kind = 'null'; + else if (Array.isArray(value)) kind = 'array'; + else if (typeof value !== 'object') kind = typeof value; + else { + const ctor = (value as { constructor?: { name?: string } }).constructor; + kind = ctor?.name && ctor.name !== 'Object' ? ctor.name : 'object'; + } + let shown: string; + try { + const json = JSON.stringify(value); + shown = typeof json !== 'string' ? typeof value : json.length > 80 ? `${json.slice(0, 77)}...` : json; + } catch { + shown = typeof value; + } + return `${kind} (${shown})`; +} + /** * [#7158] `$icontains` received a comparand that is not a non-empty string. * @@ -470,7 +567,9 @@ function bareFieldReferenceError(field: string, spec: Record, p /** * [#20099] A `{ $field }` reference outside the six scalar comparisons — a - * `$in` / `$nin` member, a text pattern, an `$exists` / `$null` operand. + * `$in` / `$nin` member, a text pattern. [#20981] An `$exists` / `$null` + * operand no longer gets here: the flag gate refuses it as a non-boolean first + * ({@link nonBooleanFlagComparandError}), as `$empty`'s gate always did. * Before this, the walker compared the reference OBJECT itself and matched * nothing (`$nin` kept everything). `$between` endpoints never get here: the * shared comparand-shape face refuses them first, in its own words. @@ -922,6 +1021,9 @@ export function assertHavingIsFilterCondition(having: unknown): void { * own {@link unknownOperator} words; * - an `$icontains` comparand that is not a non-empty string — * {@link icontainsComparandError}; + * - [#20444] an `$empty` comparand that is not a boolean — + * {@link emptyFlagComparandError}; [#20981] and an `$exists` / `$null` one, + * in the drivers' words — {@link nonBooleanFlagComparandError}; * - a bare `{ field: { $field } }` — {@link bareFieldReferenceError}; * - a reference outside the six scalar comparisons — * {@link fieldReferencePositionError}; @@ -1308,6 +1410,12 @@ function assertConditionIsEvaluable( if (op === '$empty' && typeof target !== 'boolean') { throw emptyFlagComparandError(field, target, `${path}.${op}`); } + // [#20981] …and its two siblings, by the same declaration and for the same + // reason. Before the reference-position check below as well, as `$empty`'s + // gate is: a `{ $field }` in a flag's slot is a non-boolean first. + if ((op === '$exists' || op === '$null') && typeof target !== 'boolean') { + throw nonBooleanFlagComparandError(op, field, target, `${path}.${op}`); + } if (!(CONDITION_OPERATORS as readonly string[]).includes(op)) { throw unknownOperator(op, 'condition', keys, scope.clause); } @@ -1714,6 +1822,12 @@ function checkCondition( if (op === '$empty' && typeof target !== 'boolean') { throw emptyFlagComparandError(field, target, `${path}.${op}`); } + // [#20981] …and `$exists` / `$null` beside it: the floor under the one-time + // judgment (assertConditionIsEvaluable), for a caller evaluating rows + // directly. Above the no-value exit for the same reason. + if ((op === '$exists' || op === '$null') && typeof target !== 'boolean') { + throw nonBooleanFlagComparandError(op, field, target, `${path}.${op}`); + } // [#21007] A scalar comparison on a declared JSON-stored column — refused, // as `where` refuses it. The backstop under the one-time judgment // (assertAggregationFilterSparesJsonStoredFields), for a row that gets here. @@ -1769,14 +1883,16 @@ function checkCondition( } case '$in': if (!Array.isArray(target) || !listHolds(target.map(form), stored)) return false; break; case '$nin': if (Array.isArray(target) && listHolds(target.map(form), stored)) return false; break; - case '$exists': { - const exists = value !== undefined && value !== null; - if (exists !== !!target) return false; + // [#20981] Both flags are booleans here — the gate above refused every + // other comparand — so each arm reads the flag itself. `$exists` read + // `!!target` (truthiness: `"false"` asked for the valued rows), and `$null` + // tested `=== true` / `=== false` only, so a third value constrained + // nothing. `$exists` is "has a value" (#5298), the exact mirror of `$null`. + case '$exists': + if ((value !== undefined && value !== null) !== target) return false; break; - } case '$null': - if (target === true && value != null) return false; - if (target === false && value == null) return false; + if ((value === undefined || value === null) !== target) return false; break; // [#20444] The staged emptiness flag, judged BY VALUE — the aggregated // row carries no field declaration of its own, so this face takes the diff --git a/packages/rest/src/aggregation-flag-comparand-refusal.test.ts b/packages/rest/src/aggregation-flag-comparand-refusal.test.ts new file mode 100644 index 00000000000..b88b01f965e --- /dev/null +++ b/packages/rest/src/aggregation-flag-comparand-refusal.test.ts @@ -0,0 +1,222 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20981] A per-aggregation `filter` and a `having` evaluated by + * `ObjectQL.aggregate` over a real `SqlDriver` on SQLite REFUSE a non-boolean + * `$exists` / `$null` the way their `where` twin does — `INVALID_FILTER` / 400, + * in `driver-sql`'s words — and `true` / `false` select what the twin selects. + * + * Measured before this change (`origin/main` `7a606a9a3`) through + * `engine.aggregate` on driver-sql (better-sqlite3) and driver-memory, over the + * three rows below: every `where` twin was refused, while the per-aggregation + * `filter` summed the VALUED row for `$exists` `"yes"` / `1` / `"false"`, the + * no-value rows for `0` / `null`, and EVERY row for any non-boolean `$null`; + * `having` kept the matching groups the same way (both groups under `$null`). + * The engine evaluates both clauses itself, so the driver did not enter into + * it. The engine-level cell — both `having` paths, every position, the + * published row evaluators — is `@objectstack/objectql`'s + * `engine-aggregate-flag-comparand-refusal.test.ts`. + * + * ## Why the refusal cases call the engine, not the route + * + * `POST /api/v1/data/:object/query` never reached the defect: the route parses + * the request against the spec's query schema first, and that parse already + * refuses a non-boolean flag in `where`, in `aggregations[i].filter` and in + * `having` (400 `VALIDATION_FAILED`, measured on the same base, before and + * after this change). The engine's refusal is the floor for every caller that + * reaches `engine.aggregate` without that parse — server-side code, and the + * analytics bridge that lowers a measure's `filter` into an aggregation filter. + * So the refusal cases run on `engine.aggregate`; the route is asked only to + * show where its own parse stands and to carry the `true` / `false` control end + * to end. + * + * ## The words + * + * The engine's refusal is `driver-sql`'s own diagnostic for the same flag, with + * its "this driver" clause re-aimed at the backend it names and its location + * re-rooted at the clause — one condition, one wording, copied rather than + * imported because the engine cannot depend on a driver. This file is where the + * copy is held to its source: each case reads the `where` twin's withheld + * diagnostic off the thrown error (`withheldFilterDiagnosticOf`) and requires + * the engine's message to equal it under exactly those two edits. `driver-sql` + * withholds that diagnostic from a `where` response because a `where` can carry + * a merged read scope; a per-aggregation `filter` and a `having` never do, so + * the engine's message names the field and the value, as its `$empty` and + * `$icontains` refusals do. + * + * SQLite only, deliberately: the refusal precedes every driver read, so no + * dialect can answer it differently, and the `where` twin's wording is + * `driver-sql`'s on every dialect. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { EngineAggregateOptions, EngineQueryOptions } from '@objectstack/spec/data'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver, withheldFilterDiagnosticOf } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +const OBJECT = 'rest_agg_flag_comparand_20981'; + +const DOC = { + name: OBJECT, + label: 'Deal 20981', + fields: { + name: { name: 'name', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + }, +}; + +// `a` holds a value; `b` and `c` hold none. +const ROWS = [ + { id: 'a', name: 'won', amount: 10 }, + { id: 'b', name: null, amount: 1 }, + { id: 'c', amount: 100 }, +]; + +const OPS = ['$exists', '$null'] as const; + +const NON_BOOLEANS: ReadonlyArray = [ + ['"yes"', 'yes'], + ['1', 1], + ['"false"', 'false'], + ['0', 0], + ['null', null], +]; + +function createMockServer() { + const noop = () => {}; + return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; +} + +function makeRes() { + const res: any = { + write: () => true, end: () => {}, + header: () => res, + status: (code: number) => { res._status = code; return res; }, + json: (body: any) => { res._json = body; return res; }, + }; + return res; +} + +const perAggregation = (filter: unknown) => ({ + aggregations: [{ function: 'count', alias: 'n' }, { function: 'sum', field: 'amount', alias: 's', filter }], +}); +const grouped = (having: unknown) => ({ groupBy: ['name'], aggregations: [{ function: 'count', alias: 'n' }], having }); +const whereSum = (where: unknown) => ({ where, aggregations: [{ function: 'count', alias: 'n' }, { function: 'sum', field: 'amount', alias: 's' }] }); +const whereGrouped = (where: unknown) => ({ where, groupBy: ['name'], aggregations: [{ function: 'count', alias: 'n' }] }); + +describe('[#20981] a non-boolean $exists / $null in a per-aggregation filter or a having is refused as its where twin is — SqlDriver on sqlite', () => { + let engine: ObjectQL; + let post: (body: Record) => Promise<{ status: number; json: any }>; + + beforeAll(async () => { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as any); + engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + engine.registry.registerObject(DOC as any); + await engine.syncSchemas(); + for (const row of ROWS) await engine.insert(OBJECT, { ...row } as any); + vi.spyOn((engine as any).logger, 'warn').mockImplementation(() => undefined); + + const protocol = new ObjectStackProtocolImplementation(engine as any); + const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/data/:object/query'); + expect(route).toBeDefined(); + post = async (body) => { + const res = makeRes(); + await route!.handler({ params: { object: OBJECT }, body: JSON.parse(JSON.stringify(body)) } as any, res); + return { status: res._status ?? 200, json: res._json }; + }; + }, 60_000); + + afterAll(async () => { + await engine?.destroy().catch(() => {}); + }, 60_000); + + /** The `where` twin's refusal, thrown by `driver-sql`, and the diagnostic it keeps server-side. */ + async function whereTwinDiagnostic(op: string, comparand: unknown): Promise { + try { + // Deliberately off-contract: the flag is not a boolean. + await engine.find(OBJECT, { where: { name: { [op]: comparand } } } as unknown as EngineQueryOptions); + } catch (err) { + expect((err as { code?: string }).code).toBe('INVALID_FILTER'); + const diagnostic = withheldFilterDiagnosticOf(err); + expect(diagnostic, 'driver-sql keeps the full diagnostic on the thrown error').toEqual(expect.any(String)); + return diagnostic!; + } + throw new Error(`expected the where twin of ${op} to be refused`); + } + + /** The engine's full message, thrown before any row is read. */ + async function engineRefusal(body: Record): Promise { + try { + // Deliberately off-contract: the flag is not a boolean. + await engine.aggregate(OBJECT, body as unknown as EngineAggregateOptions); + } catch (err) { + return err as Error & { code?: string; status?: number }; + } + throw new Error('expected engine.aggregate to refuse this clause'); + } + + for (const op of OPS) { + for (const [label, comparand] of NON_BOOLEANS) { + it(`${op}: ${label} — engine.aggregate refuses it in the filter and in having, in the where twin's words`, async () => { + const diagnostic = await whereTwinDiagnostic(op, comparand); + const twinAt = diagnostic.match(/ at (\S+)\. @objectstack/)?.[1]; + expect(twinAt, diagnostic).toBe(`filter.name.${op}`); + + for (const [body, at] of [ + [perAggregation({ name: { [op]: comparand } }), `aggregations[1].filter.name.${op}`], + [grouped({ name: { [op]: comparand } }), `having.name.${op}`], + ] as const) { + // The copy, held to its source: driver-sql's diagnostic, re-rooted at + // the clause and with "this driver" re-aimed at driver-sql. + const err = await engineRefusal(body); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toBe( + diagnostic.trim().replace(` at ${twinAt}. `, ` at ${at}. `).replaceAll('this driver', 'driver-sql'), + ); + expect(err.message).toContain(`Operator "${op}" on field "name" requires a boolean comparand (true or false).`); + } + }, 60_000); + } + } + + it('the route refuses all three positions in its own schema parse first — 400 VALIDATION_FAILED, unchanged here', async () => { + for (const [body, field] of [ + [whereSum({ name: { $exists: 'yes' } }), 'query.where.name.$exists'], + [perAggregation({ name: { $exists: 'yes' } }), 'query.aggregations.1.filter.name.$exists'], + [grouped({ name: { $null: 1 } }), 'query.having.name.$null'], + ] as const) { + const res = await post(body); + expect(res.status, JSON.stringify(res.json)).toBe(400); + expect(res.json.code).toBe('VALIDATION_FAILED'); + expect(res.json.fields.map((f: { field: string }) => f.field)).toEqual([field]); + } + }, 60_000); + + // The control: a boolean flag answers, and the filter and having select the + // rows and groups their where twin does. + for (const op of OPS) { + for (const flag of [true, false]) { + it(`control — ${op}: ${flag} sums and groups as its where twin does`, async () => { + const twin = await post(whereSum({ name: { [op]: flag } })); + expect(twin.status, JSON.stringify(twin.json)).toBe(200); + const agg = await post(perAggregation({ name: { [op]: flag } })); + expect(agg.status, JSON.stringify(agg.json)).toBe(200); + expect(agg.json.records).toEqual([{ n: 3, s: twin.json.records[0].s }]); + + const twinGroups = await post(whereGrouped({ name: { [op]: flag } })); + const having = await post(grouped({ name: { [op]: flag } })); + expect(having.status, JSON.stringify(having.json)).toBe(200); + expect(having.json.records).toEqual(twinGroups.json.records); + expect(having.json.records).toHaveLength(1); + }, 60_000); + } + } +});