diff --git a/.changeset/20099-having-where-doors.md b/.changeset/20099-having-where-doors.md index a94697169c1..ed810ccd76b 100644 --- a/.changeset/20099-having-where-doors.md +++ b/.changeset/20099-having-where-doors.md @@ -30,4 +30,4 @@ Not refused, but answering differently: 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 is a scalar comparison against an aggregation alias (`{ 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. -Not changed: scalars, `null` in the equality slot, `$in` / `$nin` lists, a two-bound `$between`, scalar ordering bounds, `{}`, `null` and an omitted `having`, on both paths. The `$like` / `$ilike` operators are still refused on `having` (they are staged out of `FILTER_OPERATORS`), and `$ne` with a list is still answered until the shared face judges it. A `having` key that names no column still keeps no group rather than being refused. +Not changed: scalars, `null` in the equality slot, `$in` / `$nin` lists, a two-bound `$between`, scalar ordering bounds, `{}`, `null` and an omitted `having`, on both paths. The `$like` / `$ilike` operators are still refused on `having` (they are staged out of `FILTER_OPERATORS`), and `$ne` with a list is still answered until the shared face judges it. A `having` key that names no column is not judged by this change; since #20123 it is refused (`INVALID_FILTER` / 400) rather than keeping no group. diff --git a/.changeset/20127-having-adddays-temporal-pair.md b/.changeset/20127-having-adddays-temporal-pair.md index a47a3502a01..6b191f66b9e 100644 --- a/.changeset/20127-having-adddays-temporal-pair.md +++ b/.changeset/20127-having-adddays-temporal-pair.md @@ -31,4 +31,4 @@ A column whose class the declaration cannot tell is not judged: an object with n Who is affected: `having` is a request-only key (`QuerySchema.having`, `EngineAggregateOptions.having`), and no metadata type stores it. No `having` in this repository's docs and published skills carries a `{ $field }` reference. Callers of `engine.aggregate` and of the REST aggregate query in a deployment were NOT measured. -Not changed, measured identical before and after on both paths: a `date` / `date` pair and a `datetime` / `datetime` pair, with a positive or negative whole-day literal or with an offset read from a numeric column (`max` of a number, or a `count`); a `day` date bucket against a `date` column; and any `{ $field }` comparison WITHOUT `addDays`, including a numeric pair and a numeric column against a `date` one. A per-aggregation `filter` (`aggregations[i].filter`) is not judged by this rule: it reads the object's raw columns, and this change classifies only the aggregated row's. +Not changed, measured identical before and after on both paths: a `date` / `date` pair and a `datetime` / `datetime` pair, with a positive or negative whole-day literal or with an offset read from a numeric column (`max` of a number, or a `count`); a `day` date bucket against a `date` column; and any `{ $field }` comparison WITHOUT `addDays`, including a numeric pair and a numeric column against a `date` one. A per-aggregation `filter` (`aggregations[i].filter`) is not judged by this change: it reads the object's raw columns, and this change classifies only the aggregated row's. Since #20148 it is judged by the same class rule, against the object's declared fields. diff --git a/.changeset/20148-aggregation-filter-keys-rest.md b/.changeset/20148-aggregation-filter-keys-rest.md new file mode 100644 index 00000000000..4d50a11e343 --- /dev/null +++ b/.changeset/20148-aggregation-filter-keys-rest.md @@ -0,0 +1,24 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata-protocol)!: the read door judges the keys inside each per-aggregation `filter` with the gate an explicit `where` meets — an unknown key is `INVALID_FIELD` / 400, not a count of zero (#20148) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows what `POST /data/:object/query` (`findData`) accepts in `aggregations[i].filter`. Each entry's filter now goes through the gate the explicit `where` meets, `assertFilterFieldsExist`: the same field set, the same verdicts, `INVALID_FIELD` / 400, with the entry's position as the parameter (`aggregations[1].filter`). The refusal is raised before the engine is called. It ships as `minor` under the launch-window convention for accept-set narrowings. + +Measured on the base through `POST /data/:object/query` on `driver-memory` and `driver-sql`, over six rows in three groups: + +| in `aggregations[i].filter` | before | now | +|:--|:--|:--| +| a key naming no field of the object (`{ nope: 1 }`), also under `$and` | `200`, that count 0 | `INVALID_FIELD` / 400, naming the key | +| the same key under `$ne`, `$not`, or behind a `$or` branch that holds | `200`, every row counted | `INVALID_FIELD` / 400 | +| an unknown key carrying an unknown operator (`{ nope: { $median: 1 } }`) | `INVALID_FILTER` / 400 from the engine | `INVALID_FIELD` / 400: the field gate runs first, as it does for the same key in `where` | +| a dotted key on a scalar head, or a key naming a `formula` field | `INVALID_FIELD` / 400 from the engine's own filter seam | `INVALID_FIELD` / 400 from this gate, in the words `where` gets | + +Run after the entry checks and the aggregated-field check, so an entry the spec cannot read keeps its shape refusal and an unknown aggregated `field` keeps its own. A filter that is not a plain object names no key here and is left to the engine's shape gate. + +Not changed: a filter on declared keys, on `id`, `created_at` or `updated_at`, and a relation head in the nested-object form reach the engine with the filter untouched. diff --git a/.changeset/20148-aggregation-filter-where-doors.md b/.changeset/20148-aggregation-filter-where-doors.md new file mode 100644 index 00000000000..2e8d050ab96 --- /dev/null +++ b/.changeset/20148-aggregation-filter-where-doors.md @@ -0,0 +1,29 @@ +--- +"@objectstack/objectql": minor +--- + +fix(objectql)!: a per-aggregation `filter` (`aggregations[i].filter`) on `engine.aggregate` takes the doors `where` takes — the temporal-comparand door, a `{ $field }` referent that must be a declared field, and the `addDays` class rule — and a `Date` bound is compared as an instant, as it is in `where` (#20148) + +Clause-②: no (narrowing) + + + +**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. It ships as `minor` under the launch-window convention for accept-set narrowings. + +Measured on the base through `engine.aggregate` and through `POST /data/:object/query`, on `driver-memory` and `driver-sql`, over six rows in three groups, with the same condition written as the call's `where` beside each: + +| in `aggregations[i].filter` | before | now | +|:--|:--|:--| +| a comparand a declared temporal field cannot read: `{ placed_on: { $gt: 'not-a-date' } }` on a `date`, the same as an `$in` member, a `$between` endpoint, in the implicit slot, behind a `$or` branch that holds or under `$not`; a preset name (`'last_30_days'`) on a `datetime`; `'noon'` on a `time` | counted as written: no row for the bad bound, only the readable members of the `$in`, every row for the `$between`, held-`$or` and `$not` shapes. The same condition as a `where` was refused | refused by the temporal-comparand door `where` takes, run unchanged on this position, in its words. A `{placeholder}` is stepped around and resolved, as there | +| a `{ $field }` naming no declared field of the object (`{ amount: { $gt: { $field: 'nope' } } }`), a dotted referent, or an `addDays` offset column the object does not declare | no row counted; every row under `$ne`, `$not` or a held `$or`. `driver-sql` refused the same comparison in a `where` | refused | +| `addDays` on a pair `FieldReferenceSchema.addDays` does not declare it for: two numeric fields (`{ amount: { $gt: { $field: 'cap', addDays: 1 } } }`), a numeric referent, a text or a time pair, a `date` against a `datetime`, or an offset read from a column that is not numeric | answered by the evaluator's epoch-millisecond coercion: no row on the fixture, 4 of 6 for the `date` / `datetime` pair. `driver-sql` refused the same pair in a `where` | refused | + +The two `{ $field }` rows are refused in the withholding posture `driver-sql` applies to the same comparison in a `where`: the message names the aggregation that carries the reference (`aggregations[1].filter`) and the rule, and withholds the fields, the operator and the specific reason. The engine logs the withheld diagnostic at `warn`, once per refusal. A referent is judged against the object's declared field map plus `id`, `created_at` and `updated_at`, the set the REST field gate reads; on a host whose registry holds no field map for the object, nothing is judged. + +These refusals run after the walker's own (an unknown operator, a malformed reference), so a filter carrying both gets the walker's refusal first. + +Not refused, but answering differently: + +- **A `Date` bound is compared as an instant.** `{ opened_at: { $gt: new Date('2026-02-01') } }` against the ISO text a `datetime` column holds counted no row: JS compared the `Date` with the string by coercion. `$ne` and `$nin` counted every row, and a `$between` of two `Date`s counted every row. The same bound in a `where` counted 4 of 6 on both drivers. The comparison now reads the pair through `@objectstack/spec/data`'s `utcInstantMs`, the lift `@objectstack/formula`'s evaluator applies, whenever one side is a `Date` and both sides denote an instant. Every `datetime` row and a UTC-midnight `Date` on a `date` field now count what the same bound counts in a `where`. `having` shares this comparison, so a `Date` bound in `having` keeps the groups its ISO spelling keeps on a `datetime` column; on a `date`-class column (`min` / `max` of a `date` field) a UTC-midnight `Date` follows the calendar-day reading a `where` gives (`{ $gte: new Date('2026-02-01') }` keeps the group whose max is `2026-02-01`, and so does `$eq`), which the ISO-instant text, compared as text, did not. A `Date` is not JSON, so this reaches in-process callers only. Two readings still differ from a `where`, because the per-row comparison holds no declaration: a `Date` carrying a time of day against a `date` field, which a `where` reads as that UTC calendar day, and a `Date` against a `time` field, which is not an instant and is left as before. + +Not changed, measured identical before and after on both drivers: every `where`, `groupBy` and `having` answer outside the `Date` shapes above, and a per-aggregation filter that uses implicit equality, ordering bounds, `$in`, `$between`, `$or`, `{}`, a `{ $field }` between two declared numeric fields, `addDays` between two `date` or two `datetime` fields (literal or read from a numeric column), and a reference to `created_at` or `id`. diff --git a/packages/metadata-protocol/src/protocol.aggregation-filter-fields.test.ts b/packages/metadata-protocol/src/protocol.aggregation-filter-fields.test.ts new file mode 100644 index 00000000000..13a6e9a0efc --- /dev/null +++ b/packages/metadata-protocol/src/protocol.aggregation-filter-fields.test.ts @@ -0,0 +1,139 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20148] The KEYS inside a per-aggregation `filter`, at the read door. + * + * `findData` judged a `where`'s keys against the object's field set (#7534, + * {@link ObjectStackProtocolImplementation} `assertFilterFieldsExist`) and an + * aggregation's `function` / `alias` / `field` (#4254), but never the keys of + * the filter an aggregation carries. Measured on the base through + * `POST /api/v1/data/:object/query` on driver-memory and driver-sql: + * + * ``` + * aggregations: [{ …, filter: { nope: 1 } }] -> 200, that count 0 + * aggregations: [{ …, filter: { nope: { $ne: 1 } } }] -> 200, EVERY row counted + * where: { nope: 1 } -> 400 INVALID_FIELD + * ``` + * + * Now each aggregation's filter goes through the `where` gate itself: the same + * field set, the same `unknown` > `dotted` > virtual ladder, the same + * `INVALID_FIELD` / 400, the caller's position (`aggregations[1].filter`) as + * the parameter — and the engine is never reached. Every pin below asserts + * `code` AND `status` AND that `engine.aggregate` was not called; the + * preservation half asserts what the engine received. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'invoice', + fields: { + name: { name: 'name', type: 'text' }, + status: { name: 'status', type: 'text' }, + amount: { name: 'amount', type: 'number' }, + doubled: { name: 'doubled', type: 'formula', expression: 'amount * 2' }, + owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' }, + }, +}; + +function makeProtocol() { + const find = vi.fn(async () => [] as unknown[]); + const aggregate = vi.fn(async () => [] as unknown[]); + const engine = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + find, + aggregate, + count: vi.fn(async () => 0), + }; + return { p: new ObjectStackProtocolImplementation(engine as any), find, aggregate }; +} + +/** The second of two aggregations carries `filter`, so the position must name WHICH one. */ +function withFilter(filter: unknown) { + return { + aggregations: [ + { function: 'count', alias: 'n' }, + { function: 'count', alias: 'm', filter }, + ], + }; +} + +async function refusalFor(query: Record) { + const { p, find, aggregate } = makeProtocol(); + let answered: unknown; + try { + answered = await p.findData({ object: 'invoice', query } as never); + } catch (e) { + expect(find, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + expect(aggregate, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + return e as Error & { status?: number; code?: string; param?: string; field?: string; fields?: string[] }; + } + throw new Error(`${JSON.stringify(query)} was ACCEPTED (answered ${JSON.stringify(answered)}) instead of refused`); +} + +describe('[#20148] findData — the keys inside a per-aggregation filter meet the where gate', () => { + const REFUSED: ReadonlyArray, string]> = [ + ['an unknown key (the card\'s row 4)', { nope: 1 }, 'nope'], + ['an unknown key under $ne', { nope: { $ne: 1 } }, 'nope'], + ['an unknown key under $and', { $and: [{ amount: { $gt: 0 } }, { nope: 1 }] }, 'nope'], + ['an unknown key behind a $or branch that holds', { $or: [{ amount: { $gt: 0 } }, { nope: 1 }] }, 'nope'], + ['an unknown key under $not', { $not: { nope: 1 } }, 'nope'], + ['an unknown key beside an unknown operator', { nope: { $median: 1 } }, 'nope'], + ]; + + for (const [name, filter, key] of REFUSED) { + it(`${name}: 400 INVALID_FIELD at the aggregation's position, as the same key in where is`, async () => { + const err = await refusalFor(withFilter(filter)); + expect(err.code).toBe('INVALID_FIELD'); + expect(err.status).toBe(400); + expect(err.field).toBe(key); + expect(err.param).toBe('aggregations[1].filter'); + // The twin: the same condition as the call's `where`. + const twin = await refusalFor({ where: filter, aggregations: [{ function: 'count', alias: 'n' }] }); + expect(twin.code).toBe('INVALID_FIELD'); + expect(twin.status).toBe(400); + expect(twin.field).toBe(key); + }); + } + + it('the gate\'s other two verdicts come with it — a dotted path and a virtual field, each as in where', async () => { + for (const [filter, field] of [[{ 'status.name': 'x' }, 'status.name'], [{ doubled: { $gt: 10 } }, 'doubled']] as const) { + const err = await refusalFor(withFilter(filter)); + const twin = await refusalFor({ where: filter, aggregations: [{ function: 'count', alias: 'n' }] }); + for (const e of [err, twin]) { + expect(e.code).toBe('INVALID_FIELD'); + expect(e.status).toBe(400); + expect(e.field).toBe(field); + } + expect(err.param).toBe('aggregations[1].filter'); + } + }); + + it('an unknown aggregated field keeps its own verdict ahead of a filter key', async () => { + const err = await refusalFor({ + aggregations: [{ function: 'sum', field: 'amout', alias: 't', filter: { nope: 1 } }], + }); + expect(err.code).toBe('INVALID_FIELD'); + expect(err.param).toBe('aggregations'); + expect(err.field).toBe('amout'); + }); + + const ADMITTED: ReadonlyArray = [ + ['declared keys, composed', { $or: [{ amount: { $gt: 100 } }, { status: 'paid' }] }], + ['the columns every row carries', { id: 'i1', created_at: { $ne: null }, updated_at: { $ne: null } }], + ['a relation head in the nested-object form', { owner_id: { $ne: null } }], + ['a filter that is not an object (left to the engine\'s shape gate)', 'status = paid'], + ['an array filter (left to the engine\'s shape gate)', [['amount', '>', 1]]], + ]; + + for (const [name, filter] of ADMITTED) { + it(`${name}: reaches engine.aggregate with the filter untouched`, async () => { + const { p, aggregate } = makeProtocol(); + await p.findData({ object: 'invoice', query: withFilter(filter) } as never); + expect(aggregate).toHaveBeenCalledTimes(1); + const opts = (aggregate.mock.calls[0] as unknown[])[1] as { aggregations: Array<{ filter?: unknown }> }; + expect(opts.aggregations[1].filter).toEqual(filter); + }); + } +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index ba5fbccde60..a9a77c8f6dc 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -10509,6 +10509,19 @@ export class ObjectStackProtocolImplementation implements * it answered `null`/`0` while looking like a served query. `count` with * no field (or the explicit `'*'` sentinel) is the one legitimate * field-less form and passes. + * + * [#20148] …and the KEYS inside each entry's `filter` (the per-aggregation + * filter, `AggregationNodeSchema.filter`), judged by the very gate the + * explicit `where` takes ({@link assertFilterFieldsExist}): the same field + * set, the same `unknown` > `dotted` > virtual ladder, the same + * `INVALID_FIELD` / 400, with the caller's own position as the parameter + * (`aggregations[1].filter`). Measured on the base through `POST + * /data/:object/query` on driver-memory and driver-sql: `{ nope: 1 }` in + * one aggregation's filter answered 200 with that count 0 (every count + * under `$ne`, a `$not`, or a `$or` branch that held), while the same key + * in `where` answered this 400. Run after the entry and field checks + * above, so an entry the spec cannot read keeps its shape verdict, and an + * unknown aggregated field keeps its own. */ private assertAggregationFieldsExist(object: string, aggregations: unknown): void { if (aggregations === undefined || aggregations === null) return; @@ -10597,7 +10610,15 @@ export class ObjectStackProtocolImplementation implements const gate = this.resolveQueryFields(object); if (!gate) return; const unknown = fieldsToCheck.filter((f) => !gate.known.has(f)); - if (unknown.length === 0) return; + if (unknown.length === 0) { + // [#20148] The filter keys, entry by entry, through `where`'s own + // gate — see this method's doc. A filter that is not a plain object + // names no key here and is left to the engine's shape gate. + aggregations.forEach((entry, i) => { + this.assertFilterFieldsExist(object, entry.filter, `aggregations[${i}].filter`); + }); + return; + } const first = unknown[0]; const dottedHint = first.includes('.') && gate.known.has(first.split('.')[0]) ? " Aggregation runs over this object's own columns; a related record's column cannot " diff --git a/packages/objectql/src/engine-aggregate-filter.test.ts b/packages/objectql/src/engine-aggregate-filter.test.ts index 66c475b589a..f9700d89a4c 100644 --- a/packages/objectql/src/engine-aggregate-filter.test.ts +++ b/packages/objectql/src/engine-aggregate-filter.test.ts @@ -577,3 +577,286 @@ describe('[#20122] per-aggregation filter — the shape gate `where` takes: a fi }); } }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#20148] The doors `where` has and the per-aggregation filter lacked +// ─────────────────────────────────────────────────────────────────────────── + +/** + * An order ledger with every temporal class and a numeric pair, so each door + * has a declared field to judge. `id` and `created_at` ride on every row, as a + * driver's rows carry them, for the system-column reference controls. + */ +const ORDERS: ReadonlyArray> = [ + { id: 'o1', customer_id: 'c1', amount: 100, cap: 50, placed_on: '2026-01-10', due_on: '2026-01-05', grace: 3, opened_at: '2026-01-01T10:00:00.000Z', closed_at: '2026-01-03T10:00:00.000Z', slot: '09:00:00', created_at: '2026-09-01T00:00:00.000Z' }, + { id: 'o2', customer_id: 'c1', amount: 400, cap: 10, placed_on: '2026-01-02', due_on: '2026-01-20', grace: 10, opened_at: '2026-01-02T10:00:00.000Z', closed_at: '2026-01-02T12:00:00.000Z', slot: '10:30:00', created_at: '2026-09-01T00:00:00.000Z' }, + { id: 'o3', customer_id: 'c2', amount: 900, cap: 5000, placed_on: '2026-03-01', due_on: '2026-01-01', grace: 1, opened_at: '2026-02-01T10:00:00.000Z', closed_at: '2026-02-10T10:00:00.000Z', slot: '11:00:00', created_at: '2026-09-01T00:00:00.000Z' }, + { id: 'o4', customer_id: 'c2', amount: 300, cap: 1, placed_on: '2026-02-01', due_on: '2026-02-01', grace: 1, opened_at: '2026-02-05T10:00:00.000Z', closed_at: '2026-02-05T11:00:00.000Z', slot: '12:00:00', created_at: '2026-09-01T00:00:00.000Z' }, + { id: 'o5', customer_id: 'c2', amount: 50, cap: 2, placed_on: '2026-01-15', due_on: '2026-03-01', grace: 1, opened_at: '2026-02-06T10:00:00.000Z', closed_at: '2026-02-06T10:00:00.000Z', slot: '13:00:00', created_at: '2026-09-01T00:00:00.000Z' }, + { id: 'o6', customer_id: 'c3', amount: 20, cap: 20, placed_on: '2026-02-01', due_on: '2026-01-31', grace: 0, opened_at: '2026-03-01T10:00:00.000Z', closed_at: '2026-03-01T10:00:00.000Z', slot: '14:00:00', created_at: '2026-09-01T00:00:00.000Z' }, +]; + +async function makeOrderEngine(driver: any) { + const engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + (engine.registry as any).registerObject({ + name: 'crm_order', + fields: { + customer_id: { type: 'text' }, + amount: { type: 'number' }, + cap: { type: 'number' }, + placed_on: { type: 'date' }, + due_on: { type: 'date' }, + grace: { type: 'number' }, + opened_at: { type: 'datetime' }, + closed_at: { type: 'datetime' }, + slot: { type: 'time' }, + }, + }); + return engine; +} + +/** + * Refused on both driver kinds, on an EMPTY and a populated table, grouped and + * ungrouped, with one message and no driver read — and the same filter as a + * `where` on the same engine (the twin), with its own answer. Returns the + * message and every `warn` line the engine logged while refusing. + */ +async function expectOrderFilterRefusal(filter: () => unknown): Promise<{ message: string; warnings: string[] }> { + let message: string | undefined; + const warnings: string[] = []; + for (const native of [true, false]) { + for (const [population, rows] of [['empty', []], ['populated', ORDERS]] as const) { + for (const groupBy of [undefined, ['customer_id']]) { + const cell = `${native ? 'native-capable' : 'raw'} driver, ${population}, ${groupBy ? 'grouped' : 'ungrouped'}`; + const { driver, calls } = makeCountingDriver(rows, native); + const engine = await makeOrderEngine(driver); + const logger = (engine as any).logger; + const warn = logger.warn; + logger.warn = (line: unknown) => { warnings.push(String(line)); }; + try { + const err = await refusalOf(() => engine.aggregate('crm_order', withFilter(filter(), groupBy))); + expect(err, cell).toBeInstanceOf(Error); + expect(err.code, cell).toBe('INVALID_FILTER'); + expect(err.status, cell).toBe(400); + expect(calls, cell).toEqual({ aggregate: 0, find: 0 }); + message ??= err.message; + expect(err.message, cell).toBe(message); + } finally { + logger.warn = warn; + } + } + } + } + return { message: message!, warnings }; +} + +/** The same condition as the call's `where` (the twin), on a populated table. */ +async function whereTwinOf(filter: unknown, native: boolean): Promise<{ answer?: unknown; err?: Refusal; calls: { aggregate: number; find: number } }> { + const { driver, calls } = makeCountingDriver(ORDERS, native); + const engine = await makeOrderEngine(driver); + try { + const answer = await engine.aggregate('crm_order', { + where: filter, + aggregations: [{ function: 'count', alias: 'n' }], + } as unknown as EngineAggregateOptions); + return { answer, calls }; + } catch (e) { + return { err: e as Refusal, calls }; + } +} + +describe('[#20148] per-aggregation filter — the temporal-comparand door `where` takes', () => { + // Before, measured through `engine.aggregate` and `POST /data/:object/query` + // on driver-memory and driver-sql: each counted NO row on a populated table + // (the `$between` and the `$or` / `$not` rows counted EVERY row), and the + // same condition as a `where` was refused 400 by this door. + const TEMPORAL_REFUSED: ReadonlyArray Record, string, string]> = [ + ['a bad date under $gt (the card\'s row 1)', () => ({ placed_on: { $gt: 'not-a-date' } }), 'placed_on', '"not-a-date"'], + ['a bad date as an $in member', () => ({ placed_on: { $in: ['2026-01-10', 'not-a-date'] } }), 'placed_on', '"not-a-date"'], + ['a bad date as a $between endpoint', () => ({ placed_on: { $between: ['2026-01-01', 'not-a-date'] } }), 'placed_on', '"not-a-date"'], + ['a bad date in the implicit-equality slot', () => ({ placed_on: 'not-a-date' }), 'placed_on', '"not-a-date"'], + ['a preset name on a datetime', () => ({ opened_at: { $gte: 'last_30_days' } }), 'opened_at', '"last_30_days"'], + ['a bad wall clock on a time', () => ({ slot: { $gt: 'noon' } }), 'slot', '"noon"'], + ['a bad date behind a $or branch that holds', () => ({ $or: [{ amount: { $gt: 0 } }, { placed_on: { $gt: 'not-a-date' } }] }), 'placed_on', '"not-a-date"'], + ['a bad date under $not', () => ({ $not: { placed_on: { $gt: 'not-a-date' } } }), 'placed_on', '"not-a-date"'], + ]; + + for (const [name, filter, field, shown] of TEMPORAL_REFUSED) { + it(`${name}: refused before any read, whatever the rows — as the where twin is`, async () => { + await expectFilterRefusalNamed(filter, field, shown); + for (const native of [true, false]) { + const twin = await whereTwinOf(filter(), native); + expect(twin.err?.code).toBe('INVALID_FILTER'); + expect(twin.err?.status).toBe(400); + expect(twin.calls).toEqual({ aggregate: 0, find: 0 }); + // One door, one verdict: the twin's refusal names the same field and value. + expect(twin.err?.message).toContain(`filter on '${field}'`); + expect(twin.err?.message).toContain(shown); + } + }); + } + + it('a {placeholder} is stepped around, as in where: `{ $lte: "{today}" }` counts every row', async () => { + for (const native of [true, false]) { + const { driver } = makeCountingDriver(ORDERS, native); + const engine = await makeOrderEngine(driver); + expect(await engine.aggregate('crm_order', withFilter({ placed_on: { $lte: '{today}' } }))) + .toEqual([{ opp_count: 6, picked: 6 }]); + } + }); +}); + +async function expectFilterRefusalNamed(filter: () => unknown, field: string, shown: string) { + const out = await expectOrderFilterRefusal(filter); + expect(out.message).toContain(`filter on '${field}'`); + expect(out.message).toContain(shown); + return out; +} + +describe('[#20148] per-aggregation filter — a { $field } names a declared field, and addDays pairs two temporal fields of one class', () => { + // Before, measured through `engine.aggregate` and `POST /data/:object/query` + // on driver-memory and driver-sql: each counted NO row on a populated table + // (every row under `$ne`, `$not`, or a `$or` branch that held; the + // date / datetime pair counted 4 of 6 by coercion), while the same condition + // as a `where` was refused 400 on driver-sql (its cross-field compiler) and + // counted no row on driver-memory. The rule set is driver-sql's; the words + // are its words for that refusal — the fields, the operator and the reason + // withheld, the diagnostic in the server log. + const REFERENCE_REFUSED: ReadonlyArray Record, string]> = [ + ['a referent the object does not declare (the card\'s row 3)', () => ({ amount: { $gt: { $field: 'nope' } } }), '"nope" is not a declared field of "crm_order"'], + ['an undeclared referent under $ne', () => ({ amount: { $ne: { $field: 'nope' } } }), '"nope" is not a declared field'], + ['an undeclared referent behind a $or branch that holds', () => ({ $or: [{ amount: { $gt: 0 } }, { amount: { $gt: { $field: 'nope' } } }] }), '"nope" is not a declared field'], + ['an undeclared referent under $not', () => ({ $not: { amount: { $gt: { $field: 'nope' } } } }), '"nope" is not a declared field'], + ['a dotted referent', () => ({ amount: { $gt: { $field: 'customer_id.name' } } }), '"customer_id.name" is not a declared field'], + ['an addDays offset column the object does not declare', () => ({ placed_on: { $lte: { $field: 'due_on', addDays: { $field: 'nope' } } } }), 'the addDays offset "nope" is not a declared field'], + ['addDays between two numeric fields (the card\'s row 2)', () => ({ amount: { $gt: { $field: 'cap', addDays: 1 } } }), 'addDays adds whole days to a date or datetime column, and "cap" is numeric'], + ['addDays behind a $or branch that holds', () => ({ $or: [{ amount: { $gt: 0 } }, { amount: { $gt: { $field: 'cap', addDays: 1 } } }] }), 'and "cap" is numeric'], + ['addDays from a date to a numeric field', () => ({ placed_on: { $lte: { $field: 'grace', addDays: 1 } } }), '"placed_on" is date but "grace" is numeric'], + ['addDays between a date and a datetime', () => ({ placed_on: { $lte: { $field: 'closed_at', addDays: 1 } } }), '"placed_on" is date but "closed_at" is datetime'], + ['addDays from a text to a date', () => ({ customer_id: { $lte: { $field: 'due_on', addDays: 1 } } }), '"customer_id" is text but "due_on" is date'], + ['addDays between two time fields', () => ({ slot: { $gte: { $field: 'slot', addDays: 1 } } }), 'and "slot" is time'], + ['an addDays offset read from a text field', () => ({ placed_on: { $lte: { $field: 'due_on', addDays: { $field: 'customer_id' } } } }), 'the addDays offset "customer_id" (text) is not a numeric column'], + ['an addDays offset read from a date field', () => ({ placed_on: { $lte: { $field: 'due_on', addDays: { $field: 'placed_on' } } } }), 'the addDays offset "placed_on" (date) is not a numeric column'], + ]; + + for (const [name, filter, diagnostic] of REFERENCE_REFUSED) { + it(`${name}: refused before any read, whatever the rows, the names withheld and logged`, async () => { + const { message, warnings } = await expectOrderFilterRefusal(filter); + expect(message).toContain(`\`${AT}\``); + expect(message).toContain('withheld from the message'); + // The withheld half: no field this filter names reaches the message… + for (const name of ['nope', 'cap', 'grace', 'closed_at', 'due_on', 'customer_id', 'placed_on', 'amount', 'slot']) { + expect(message).not.toContain(`"${name}"`); + } + // …and every refusal put it in the server log, once per refusal. + expect(warnings).toHaveLength(8); + for (const line of warnings) expect(line).toContain(diagnostic); + }); + } + + it('the where twin of the undeclared referent is not refused by the engine — it is driver-sql\'s compiler that refuses it', async () => { + // The control that places the door: the engine judges `where`'s + // references nowhere, so a mock driver answers the twin (a count of 0 here, + // the reference never resolving) — the refusal `where` gets lives one layer + // down, in the driver the REST pin runs (`aggregation-filter-where-doors`). + for (const native of [true, false]) { + const twin = await whereTwinOf({ amount: { $gt: { $field: 'nope' } } }, native); + expect(twin.err).toBeUndefined(); + } + }); + + // Measured identical at base and head through `engine.aggregate` on + // driver-memory and driver-sql: what the rules leave alone counts as before. + const REFERENCE_PASSING: ReadonlyArray Record, number]> = [ + ['a numeric pair with no addDays', () => ({ amount: { $gt: { $field: 'cap' } } }), 4], + ['addDays between two date fields', () => ({ placed_on: { $lte: { $field: 'due_on', addDays: 7 } } }), 5], + ['addDays read from a numeric column', () => ({ placed_on: { $lte: { $field: 'due_on', addDays: { $field: 'grace' } } } }), 3], + ['addDays between two datetime fields', () => ({ closed_at: { $gte: { $field: 'opened_at', addDays: 1 } } }), 2], + ['a reference to created_at, a column every row carries', () => ({ opened_at: { $lte: { $field: 'created_at' } } }), 6], + ['a reference to id, the primary key', () => ({ customer_id: { $ne: { $field: 'id' } } }), 6], + ]; + + for (const [name, filter, populatedCount] of REFERENCE_PASSING) { + it(`${name} counts ${populatedCount} of 6, and 0 on an empty table`, async () => { + for (const native of [true, false]) { + const { driver } = makeCountingDriver(ORDERS, native); + const populated = await makeOrderEngine(driver); + expect(await populated.aggregate('crm_order', withFilter(filter()))).toEqual([{ opp_count: 6, picked: populatedCount }]); + const { driver: emptyDriver } = makeCountingDriver([], native); + const empty = await makeOrderEngine(emptyDriver); + expect(await empty.aggregate('crm_order', withFilter(filter()))).toEqual([{ opp_count: 0, picked: 0 }]); + } + }); + } + + it('the walker\'s own refusals come first: an unknown operator beside an undeclared referent is the operator\'s refusal', async () => { + const { message, warnings } = await expectOrderFilterRefusal( + () => ({ $or: [{ amount: { $gt: { $field: 'nope' } } }, { amount: { $median: 1 } }] }), + ); + expect(message).toContain("Unsupported operator '$median'"); + expect(warnings).toEqual([]); + }); +}); + +describe('[#20148] a Date bound is compared as an instant — as the same bound in a where is', () => { + // Before, measured through `engine.aggregate` on driver-memory and + // driver-sql: every Date row below counted NO row (`$ne` / `$nin` / the + // `$between` counted every row), while the same bound in a `where` counted + // what the ISO-text spelling counts here — each driver reads a Date by the + // column's storage rule. The ISO spelling is the in-engine control: it + // answered correctly before and is unchanged. + const D = (s: string) => new Date(s); + const DATE_ROWS: ReadonlyArray Record, () => Record, number]> = [ + ['$gt (the card\'s row 5)', () => ({ opened_at: { $gt: D('2026-02-01T00:00:00.000Z') } }), () => ({ opened_at: { $gt: '2026-02-01T00:00:00.000Z' } }), 4], + ['$gte, an equal instant stored', () => ({ opened_at: { $gte: D('2026-02-01T10:00:00.000Z') } }), () => ({ opened_at: { $gte: '2026-02-01T10:00:00.000Z' } }), 4], + ['$lt', () => ({ opened_at: { $lt: D('2026-02-01T00:00:00.000Z') } }), () => ({ opened_at: { $lt: '2026-02-01T00:00:00.000Z' } }), 2], + ['$lte', () => ({ opened_at: { $lte: D('2026-02-01T10:00:00.000Z') } }), () => ({ opened_at: { $lte: '2026-02-01T10:00:00.000Z' } }), 3], + ['$eq', () => ({ opened_at: { $eq: D('2026-02-01T10:00:00.000Z') } }), () => ({ opened_at: { $eq: '2026-02-01T10:00:00.000Z' } }), 1], + ['$ne', () => ({ opened_at: { $ne: D('2026-02-01T10:00:00.000Z') } }), () => ({ opened_at: { $ne: '2026-02-01T10:00:00.000Z' } }), 5], + ['implicit equality', () => ({ opened_at: D('2026-02-01T10:00:00.000Z') }), () => ({ opened_at: '2026-02-01T10:00:00.000Z' }), 1], + ['an $in member', () => ({ opened_at: { $in: [D('2026-02-01T10:00:00.000Z')] } }), () => ({ opened_at: { $in: ['2026-02-01T10:00:00.000Z'] } }), 1], + ['a $nin member', () => ({ opened_at: { $nin: [D('2026-02-01T10:00:00.000Z')] } }), () => ({ opened_at: { $nin: ['2026-02-01T10:00:00.000Z'] } }), 5], + ['$between two Dates', () => ({ opened_at: { $between: [D('2026-02-01T00:00:00.000Z'), D('2026-02-06T00:00:00.000Z')] } }), () => ({ opened_at: { $between: ['2026-02-01T00:00:00.000Z', '2026-02-06T00:00:00.000Z'] } }), 2], + ['a UTC-midnight Date on a date field', () => ({ placed_on: { $gte: D('2026-02-01T00:00:00.000Z') } }), () => ({ placed_on: { $gte: '2026-02-01' } }), 3], + ]; + + for (const [name, dateFilter, isoFilter, populatedCount] of DATE_ROWS) { + it(`${name}: counts ${populatedCount} of 6, as the ISO spelling does`, async () => { + for (const native of [true, false]) { + const { driver } = makeCountingDriver(ORDERS, native); + const engine = await makeOrderEngine(driver); + expect(await engine.aggregate('crm_order', withFilter(isoFilter()))).toEqual([{ opp_count: 6, picked: populatedCount }]); + expect(await engine.aggregate('crm_order', withFilter(dateFilter()))).toEqual([{ opp_count: 6, picked: populatedCount }]); + } + }); + } + + it('having reads a Date bound the same way — the groups the ISO spelling keeps', async () => { + const { driver } = makeCountingDriver(ORDERS, false); + const engine = await makeOrderEngine(driver); + const run = (having: Record) => engine.aggregate('crm_order', { + groupBy: ['customer_id'], + aggregations: [{ function: 'min', field: 'opened_at', alias: 'first_opened' }], + having, + } as unknown as EngineAggregateOptions); + const groups = (rows: any[]) => rows.map((r) => r.customer_id).sort(); + for (const [dateHaving, isoHaving, kept] of [ + [{ first_opened: { $gt: D('2026-02-01T00:00:00.000Z') } }, { first_opened: { $gt: '2026-02-01T00:00:00.000Z' } }, ['c2', 'c3']], + [{ first_opened: { $eq: D('2026-02-01T10:00:00.000Z') } }, { first_opened: { $eq: '2026-02-01T10:00:00.000Z' } }, ['c2']], + [{ first_opened: { $ne: D('2026-02-01T10:00:00.000Z') } }, { first_opened: { $ne: '2026-02-01T10:00:00.000Z' } }, ['c1', 'c3']], + ] as const) { + expect(groups(await run(isoHaving))).toEqual(kept); + expect(groups(await run(dateHaving))).toEqual(kept); + } + }); + + it('the walker compares every other pair exactly as before (the lift needs a Date on one side)', () => { + // A wall clock is no instant, so a Date against one is left to `>` and + // answers as it always did; two numbers and two strings never lift. + expect(matchesAggregationFilter({ slot: '14:00:00' }, { slot: { $gt: D('2026-02-01T11:00:00.000Z') } } as never, 0)).toBe(false); + expect(matchesAggregationFilter({ amount: 5 }, { amount: { $in: [5] } } as never, 0)).toBe(true); + expect(matchesAggregationFilter({ note: '2026-02-01' }, { note: { $gte: '2026-02-01T00:00:00.000Z' } } as never, 0)).toBe(false); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index bc4f337220a..4e1ebbf2a4a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -16141,6 +16141,18 @@ export class ObjectQL implements IObjectQLEngine { // spoke on `where` alone would answer one mistake two ways within a // single verb. assertTextOperatorTargetsAreStringCapable(object, 'aggregate', this._registry.getObject(object), aggFilter); + // [#20148] …and the TEMPORAL-comparand door, fourth here as it is + // fourth on `where`'s seam (#8690) — the same function, against the + // object's declared fields, since this filter reads the object's + // raw rows. A comparand a declared date / datetime / time field + // cannot interpret (`'not-a-date'`, the preset name + // `'last_30_days'`) was compared as written: no row counted, a + // `$between` with one such endpoint counted EVERY row, and the same + // bound in a `where` was refused 400 — measured on the base on + // driver-memory and driver-sql, through the engine and REST. + // Before the token resolver below, as there, so a `{placeholder}` + // is stepped around and resolved (or refused) a moment later. + assertTemporalComparandsInterpretable(object, 'aggregate', this._registry.getObject(object), aggFilter); // [#20122] …and the two doors `having` took at its own entry // (#20099), so a refusal here is the FILTER's, never the data's: // 1. the comparand-TYPE door `where` takes in @@ -16155,8 +16167,23 @@ export class ObjectQL implements IObjectQLEngine { // walks the filter per SOURCE row, so `{ amount: { $median: 1 } }` // was a 400 on a populated table and a `200 []` on an empty one, // and a `$or` whose first branch held counted every row. + // [#20148] …then, against the object's DECLARED fields, the two + // cross-field rules `where` gets from `driver-sql`'s compiler: + // a `{ $field }` names a declared field, and an `addDays` pair + // follows `FieldReferenceSchema.addDays`' class rule. Both were + // answered by the fallback — no row counted — where `where` is + // refused. Refused in `where`'s words for that comparison, which + // withhold the fields, the operator and the reason; the + // withheld half goes to this log, as the driver writes its own. const typed = normalizeFilterComparandTypes(aggFilter, `aggregate('${object}')`, `aggregations[${i}].filter`); - assertAggregationFilterIsEvaluable(typed, i); + assertAggregationFilterIsEvaluable(typed, i, { + object, + fields: (this._registry.getObject(object) as { fields?: unknown } | undefined)?.fields, + reportWithheld: (diagnostic) => this.logger.warn( + `aggregate('${object}'): INVALID_FILTER — refusal detail withheld from the response, as it ` + + `is for the same cross-field comparison in a where. Full diagnostic: ${diagnostic}`, + ), + }); if (typed !== aggFilter) { typedAggregations ??= [...aggregations]; typedAggregations[i] = { ...(agg as object), filter: typed } as typeof agg; diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index 876293a1e91..ab6ba1df005 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -93,6 +93,13 @@ // the same class (date/date, datetime/datetime)", the offset column numeric — // against each aggregated column's class, read statically off the query and // the object's declaration. See {@link aggregatedRowColumnClasses}. +// +// [#20148] In the per-aggregation `filter`, a `{ $field }` names a field the +// object declares and an `addDays` pair follows that same class rule, judged +// against the object's declared fields and refused in the words `where` gets +// for the same comparison ({@link assertAggregationFilterReferencesAreDeclared}). +// And on both positions a `Date` bound is compared as an instant +// ({@link instantsOf}), as the same bound in a `where` is. import type { FilterCondition } from '@objectstack/spec/data'; // [#20099] The reference's own declaration, so a malformed `addDays` is refused @@ -121,6 +128,10 @@ import { RETIRED_FILTER_OPERATORS } from '@objectstack/spec/data'; // of one vocabulary, and it reads the fold from the spec for the same reason it // reads the retirement prescriptions from there: one rule, one definition. import { asciiCaseInsensitiveContains } from '@objectstack/spec/data'; +// [#20148] The spec's reading of a `Date` against stored text for a TYPE-BLIND +// evaluator — the lift `@objectstack/formula` applies to the same pairing — so a +// `Date` bound here compares the way the same bound in a `where` does. +import { utcInstantMs } from '@objectstack/spec/data'; // [#7047] The ADR-0112 envelope this face's refusals used to omit. Shared with // `filter-comparand-shape.ts` rather than re-declared here — see the note on // {@link invalidFilterError} and on {@link unknownOperator} below. @@ -508,29 +519,45 @@ function assertOffsetPairIsTemporal( path: string, classes: ReadonlyMap, ): void { + const reason = offsetPairViolation(field, reference, classes); + if (reason !== undefined) throw offsetPairError(field, op, String(reference.$field), path, reason); +} + +/** + * [#20148] The rule {@link assertOffsetPairIsTemporal} applies, returned as the + * reason a pair breaks it (`driver-sql`'s sentence for the same pair on + * `where`, with "is stored as" read as "is"), or `undefined` when it holds. + * One rule, two positions: `having` judges it against the aggregated row's + * classes and names the columns; the per-aggregation `filter` judges it against + * the object's declared fields and withholds them + * ({@link assertAggregationFilterReferencesAreDeclared}). + */ +function offsetPairViolation( + field: string, + reference: Record, + classes: ReadonlyMap, +): string | undefined { const ref = String(reference.$field); const refClass = classes.get(ref); const targetClass = classes.get(field); if (refClass !== undefined && targetClass !== undefined && refClass !== targetClass) { - throw offsetPairError(field, op, ref, path, - `"${field}" is ${targetClass} but "${ref}" is ${refClass}, and a cross-class comparison answers ` + return `"${field}" is ${targetClass} but "${ref}" is ${refClass}, and a cross-class comparison answers ` + `differently in SQL (storage-class ordering) than in memory (JS coercion) — compare same-class ` - + `columns.`); + + `columns.`; } if (refClass !== undefined && refClass !== 'date' && refClass !== 'datetime') { - throw offsetPairError(field, op, ref, path, - `addDays adds whole days to a date or datetime column, and "${ref}" is ${refClass} — an offset ` - + `has no meaning on it.`); + return `addDays adds whole days to a date or datetime column, and "${ref}" is ${refClass} — an offset ` + + `has no meaning on it.`; } const offset = reference.addDays; - if (!isFieldReferenceShape(offset)) return; + if (!isFieldReferenceShape(offset)) return undefined; const offsetRef = String(offset.$field); const offsetClass = classes.get(offsetRef); if (offsetClass !== undefined && offsetClass !== 'numeric') { - throw offsetPairError(field, op, ref, path, - `the addDays offset "${offsetRef}" (${offsetClass}) is not a numeric column, and a day offset ` - + `must be a number of days.`); + return `the addDays offset "${offsetRef}" (${offsetClass}) is not a numeric column, and a day offset ` + + `must be a number of days.`; } + return undefined; } /** @@ -738,17 +765,166 @@ export function assertHavingIsEvaluable( * `ObjectQL.aggregate` calls this in its per-aggregation loop, after the * shared doors `where` also takes there and the comparand-TYPE door, and * before `getDriver`, the middleware chain and the fallback that evaluates the - * filter. What it refuses is exactly {@link assertHavingIsEvaluable}'s list - * minus the name check: the filter reads the object's RAW columns, whose names - * the engine does not judge on `where` either (its registry-less tolerance — - * see `assertFilterIsMaterializable`), so a `{ $field }` here is judged for its - * position and its declaration, never for its name. + * filter. The walk refuses exactly {@link assertHavingIsEvaluable}'s list + * minus the name checks: the filter reads the object's RAW columns, not the + * aggregated row's, so a KEY is not judged here (the REST door refuses an + * unknown one, as it does in `where`) and a `{ $field }` is judged by the walk + * for its position and its declaration only. + * + * [#20148] Its NAME, and an `addDays` pair's classes, are judged after the + * walk when the caller hands over the object's declaration — the two + * cross-field rules `where` gets from `driver-sql`'s compiler, against the + * object's DECLARED fields: a `{ $field }` (and an `addDays` offset's nested + * `{ $field }`) names a field the object declares, and an `addDays` pair + * follows the class rule `FieldReferenceSchema.addDays` declares + * ({@link offsetPairViolation}). Measured on the base through + * `engine.aggregate` and `POST /data/:object/query`, on driver-memory and + * driver-sql, each such filter counted no row (every row under `$ne`, a + * `$not`, or a `$or` branch that held), while the same comparison in `where` + * is refused on driver-sql. After the walk on purpose — the order `having`'s + * key check takes (#20123): an author meets an operator's refusal first, and + * fixing it does not reveal one they could have been told about already. + * {@link assertAggregationFilterReferencesAreDeclared} carries the words. * * Read-only. */ -export function assertAggregationFilterIsEvaluable(filter: unknown, index: number): void { +export function assertAggregationFilterIsEvaluable( + filter: unknown, + index: number, + declared?: AggregationFilterDeclaration, +): void { const clause = aggregationFilterClause(index); assertNodeIsEvaluable(filter, clause.root, { clause }); + if (declared) assertAggregationFilterReferencesAreDeclared(filter, clause.root, declared); +} + +/** + * [#20148] What the per-aggregation `filter`'s reference rules judge against: + * the object's declared field map, the object's name for the server-side + * diagnostic, and where that diagnostic goes. + */ +export interface AggregationFilterDeclaration { + /** The object the filter reads — named in the server-side diagnostic only. */ + object: string; + /** + * The object's declared field map. Not a usable map (absent, an array, + * empty) ⇒ nothing is judged: a registry-less host must not invent a verdict + * about a declaration it cannot see, the direction every declared-type door + * of the engine takes. + */ + fields: unknown; + /** + * Handed the full diagnostic — the half the refusal withholds — just before + * the refusal is thrown, so the host puts it in its server log. + */ + reportWithheld: (diagnostic: string) => void; +} + +/** + * [#20148] The names a per-aggregation reference may take: every declared + * field, plus the columns every row carries whether or not the map lists them + * — the set the REST door's unknown-field gate reads (`resolveQueryFields`). + */ +function declaredReferenceNames(fields: Record): ReadonlySet { + return new Set([...Object.keys(fields), 'id', 'created_at', 'updated_at']); +} + +/** + * [#20148] A `{ $field }` in a per-aggregation filter that `where`'s + * cross-field rules refuse — a referent the object does not declare, or an + * `addDays` pair the offset has no meaning on. + * + * The WORDS follow `where`'s. `driver-sql` refuses the same comparison in a + * `where` with the fields, the operator and the specific reason withheld from + * the message and written to the server log — its disclosure posture for every + * cross-field refusal — so this refusal withholds exactly those and hands the + * diagnostic to the host's log instead. What stays is the refusal's identity + * (`INVALID_FILTER` / 400), WHICH aggregation carries the reference (a position + * in the query, not in the predicate), and the capability boundary — none of + * them derived from what the filter names. + */ +function withheldAggregationReferenceError(root: string): Error { + // Under the REST envelope's 500-character bound (`truncateClientMessage`), so + // the sentence saying where the withheld half went is never the part cut off. + return invalidFilterError( + `A { "$field" } reference in \`${root}\` cannot be evaluated. It must name a field the object ` + + `declares, and an addDays offset applies only between two date or two datetime fields, read from ` + + `an integer or a numeric field; evaluated anyway, its count would be silently wrong. The fields, the ` + + `operator and the reason are withheld from the message, as for the same comparison in a ` + + `\`where\`; the full diagnostic is in the server log.`, + ); +} + +/** + * [#20148] Judge every scalar-comparison `{ $field }` reference in one + * per-aggregation filter against the object's declaration — the same + * traversal {@link assertNodeIsEvaluable} takes, which has already run: `$and` + * / `$or` / `$not` are descended, every other `$` key has been refused there, + * and a reference is judged only as the whole comparand of the six scalar + * comparisons, the one position that walk lets it stand. + * + * In `driver-sql`'s order: the referent is declared, an offset column is + * declared, then the `addDays` pair rule. A filter KEY the object does not + * declare (or a formula) has no class here, so the same-class half of the pair + * rule does not judge it. + */ +function assertAggregationFilterReferencesAreDeclared( + filter: unknown, + root: string, + declared: AggregationFilterDeclaration, +): void { + const { fields } = declared; + if (!fields || typeof fields !== 'object' || Array.isArray(fields)) return; + const map = fields as Record; + if (Object.keys(map).length === 0) return; + const names = declaredReferenceNames(map); + const classes = new Map( + [...names].map((name) => [name, declaredFieldClass(map, name)]), + ); + const refuse = (field: string, op: string, path: string, ref: string, reason: string): never => { + declared.reportWithheld( + `Operator "${op}" on field "${field}" at ${path} compares against another field ` + + `({ "$field": "${ref}" }), which cannot be evaluated here: ${reason}`, + ); + throw withheldAggregationReferenceError(root); + }; + const walk = (cond: unknown, path: string): void => { + if (!cond || typeof cond !== 'object') return; + for (const [key, value] of Object.entries(cond)) { + const here = `${path}.${key}`; + if (key === '$and' || key === '$or') { + const branches = Array.isArray(value) ? value : [value]; + branches.forEach((c, i) => walk(c, `${here}[${i}]`)); + continue; + } + if (key === '$not') { + walk(value, here); + continue; + } + if (key.startsWith('$')) continue; + if (typeof value !== 'object' || value === null || value instanceof Date || Array.isArray(value)) continue; + for (const [op, target] of Object.entries(value)) { + if (!REFERENCE_COMPARISON_OPERATORS.has(op) || !isFieldReferenceShape(target)) continue; + const at = `${here}.${op}`; + const ref = String(target.$field); + if (!names.has(ref)) { + refuse(key, op, at, ref, + `"${ref}" is not a declared field of "${declared.object}" — only declared fields can be referenced.`); + } + const offset = target.addDays; + if (isFieldReferenceShape(offset) && !names.has(String(offset.$field))) { + refuse(key, op, at, ref, + `the addDays offset "${String(offset.$field)}" is not a declared field of "${declared.object}" — ` + + `only declared fields can be referenced.`); + } + if (offset !== undefined) { + const reason = offsetPairViolation(key, target, classes); + if (reason !== undefined) refuse(key, op, at, ref, reason); + } + } + } + }; + walk(filter, root); } /** @@ -973,6 +1149,55 @@ export function matchesAggregationFilter( return matchesHaving(row, filter, clause.root, clause); } +/** + * [#20148] The two operands as UTC instants — when one of them is a `Date` and + * both denote an instant — else `null`, and the comparison runs exactly as it + * always did. + * + * A `Date` bound met the stored text of a datetime column (canonical UTC ISO on + * every driver, ADR-0053 D-B) and JS compared the two by coercion: `<` and + * friends read the `Date` as its epoch and the ISO string as `NaN`, and `==` + * read the `Date` as its `toString()`. So + * `{ opened_at: { $gt: new Date('2026-02-01') } }` counted NO row in a + * per-aggregation `filter` (and kept no group in `having`), `$ne` / `$nin` + * counted every row, and a `$between` of two `Date`s kept every row — while the + * same bound in a `where` answered 4 of 6 on driver-memory and driver-sql + * alike, each driver reading it by the column's storage rule. This walker is + * TYPE-BLIND (it holds a flat row, no declaration), so it reads the pair the + * way the spec defines for a type-blind evaluator — {@link utcInstantMs}, the + * lift `@objectstack/formula`'s evaluator applies to the same pairing: a + * `Date`, epoch milliseconds, a bare `YYYY-MM-DD` (its UTC midnight) or an + * ISO / zone-naive timestamp. Anything else — a wall-clock `time` value, junk, + * an Invalid Date — is not an instant, and the pair is left to the comparison + * below, unchanged. + */ +function instantsOf(value: unknown, target: unknown): readonly [number, number] | null { + if (!(value instanceof Date) && !(target instanceof Date)) return null; + const a = utcInstantMs(value); + const b = utcInstantMs(target); + return a === null || b === null ? null : [a, b]; +} + +/** [#20148] Equality: the instants when a `Date` is in the pair, else the walker's loose `==`. */ +function comparandEquals(value: unknown, target: unknown): boolean { + const instants = instantsOf(value, target); + return instants ? instants[0] === instants[1] : value == target; +} + +/** [#20148] An ordering: the instants when a `Date` is in the pair, else the operands as they are. */ +function ordered(value: unknown, target: unknown, compare: (a: any, b: any) => boolean): boolean { + const instants = instantsOf(value, target); + return instants ? compare(instants[0], instants[1]) : compare(value, target); +} + +/** [#20148] List membership: `includes` as before, or a `Date` member naming the same instant. */ +function listHolds(list: readonly unknown[], value: unknown): boolean { + return list.includes(value) || list.some((member) => { + const instants = instantsOf(value, member); + return instants !== null && instants[0] === instants[1]; + }); +} + /** One column's condition — implicit equality or an operator object. */ function checkCondition( value: any, @@ -983,14 +1208,15 @@ function checkCondition( row: Record = {}, ): boolean { // Implicit equality (primitives, null, Date, array exact-match) — loose `==` - // to mirror the Filter Protocol's memory evaluation. + // to mirror the Filter Protocol's memory evaluation. [#20148] A `Date` bound + // is compared as an instant ({@link instantsOf}). if ( typeof condition !== 'object' || condition === null || condition instanceof Date || Array.isArray(condition) ) { - return value == condition; + return comparandEquals(value, condition); } const keys = Object.keys(condition); @@ -1027,18 +1253,22 @@ function checkCondition( if (!compareWithReference(row, value, op, target)) return false; continue; } + // [#20148] The comparison and list arms read a `Date` bound — or a `Date` + // value — as an instant ({@link instantsOf}); every other pair compares + // exactly as before. switch (op) { - case '$eq': if (value != target) return false; break; - case '$ne': if (value == target) return false; break; - case '$gt': if (!(value > target)) return false; break; - case '$gte': if (!(value >= target)) return false; break; - case '$lt': if (!(value < target)) return false; break; - case '$lte': if (!(value <= target)) return false; break; + case '$eq': if (!comparandEquals(value, target)) return false; break; + case '$ne': if (comparandEquals(value, target)) return false; break; + case '$gt': if (!ordered(value, target, (a, b) => a > b)) return false; break; + case '$gte': if (!ordered(value, target, (a, b) => a >= b)) return false; break; + case '$lt': if (!ordered(value, target, (a, b) => a < b)) return false; break; + case '$lte': if (!ordered(value, target, (a, b) => a <= b)) return false; break; case '$between': - if (Array.isArray(target) && (value < target[0] || value > target[1])) return false; + if (Array.isArray(target) + && (ordered(value, target[0], (a, b) => a < b) || ordered(value, target[1], (a, b) => a > b))) return false; break; - case '$in': if (!Array.isArray(target) || !target.includes(value)) return false; break; - case '$nin': if (Array.isArray(target) && target.includes(value)) return false; break; + case '$in': if (!Array.isArray(target) || !listHolds(target, value)) return false; break; + case '$nin': if (Array.isArray(target) && listHolds(target, value)) return false; break; case '$exists': { const exists = value !== undefined && value !== null; if (exists !== !!target) return false; diff --git a/packages/rest/src/aggregation-filter-where-doors.test.ts b/packages/rest/src/aggregation-filter-where-doors.test.ts new file mode 100644 index 00000000000..e98e39a7582 --- /dev/null +++ b/packages/rest/src/aggregation-filter-where-doors.test.ts @@ -0,0 +1,190 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20148] A per-aggregation `filter` meets the doors `where` meets, through + * the door a caller actually uses: `POST /api/v1/data/:object/query` → + * `RestServer` → `ObjectStackProtocolImplementation.findData` → + * `ObjectQL.aggregate` → a real sqlite `SqlDriver` (the harness + * `list-view-grouping-query-door.test.ts` boots). + * + * Measured on the base through this door, on driver-sql and driver-memory + * alike: each shape below answered `200` with the filtered count `0` on a + * populated table (every row under a negation, a `$not`, or a `$or` branch + * that held), while the same condition as the call's `where` — the TWIN every + * row runs beside it — was refused `400`. Now both refuse, on an empty table + * as on a populated one, and the per-aggregation refusal is raised before the + * driver is asked for a row. + * + * | row | per-aggregation `filter` | the `where` twin | + * |:--|:--|:--| + * | 1 | a date no temporal field can read | `INVALID_FILTER` (the engine's temporal door) | + * | 2 | `addDays` between two numeric fields | `INVALID_FILTER` (driver-sql's cross-field compiler) | + * | 3 | a `{ $field }` naming no declared field | `INVALID_FILTER` (driver-sql's cross-field compiler) | + * | 4 | a key naming no field | `INVALID_FIELD` (the read door's field gate) | + * + * Row 5 — a `Date` bound — has no reach through this door: JSON carries it as + * its ISO text, which answered correctly before. The last block pins that + * spelling against its twin, as the control the engine-level `Date` pins + * (`packages/objectql`, `engine-aggregate-filter.test.ts`) are measured to. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from './rest-server'; + +const OBJECT = 'ledger_order'; + +const LEDGER_ORDER = { + name: OBJECT, + label: 'Ledger Order', + fields: { + customer_id: { name: 'customer_id', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + cap: { name: 'cap', type: 'number' as const }, + placed_on: { name: 'placed_on', type: 'date' as const }, + due_on: { name: 'due_on', type: 'date' as const }, + opened_at: { name: 'opened_at', type: 'datetime' as const }, + }, +}; + +const ROWS = [ + { id: 'o1', customer_id: 'c1', amount: 100, cap: 50, placed_on: '2026-01-10', due_on: '2026-01-05', opened_at: '2026-01-01T10:00:00.000Z' }, + { id: 'o2', customer_id: 'c1', amount: 400, cap: 10, placed_on: '2026-01-02', due_on: '2026-01-20', opened_at: '2026-01-02T10:00:00.000Z' }, + { id: 'o3', customer_id: 'c2', amount: 900, cap: 5000, placed_on: '2026-03-01', due_on: '2026-01-01', opened_at: '2026-02-01T10:00:00.000Z' }, + { id: 'o4', customer_id: 'c2', amount: 300, cap: 1, placed_on: '2026-02-01', due_on: '2026-02-01', opened_at: '2026-02-05T10:00:00.000Z' }, + { id: 'o5', customer_id: 'c2', amount: 50, cap: 2, placed_on: '2026-01-15', due_on: '2026-03-01', opened_at: '2026-02-06T10:00:00.000Z' }, + { id: 'o6', customer_id: 'c3', amount: 20, cap: 20, placed_on: '2026-02-01', due_on: '2026-01-31', opened_at: '2026-03-01T10:00:00.000Z' }, +]; + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +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; +} + +/** A booted door over a real sqlite table — populated or empty — with its driver reads counted. */ +async function boot(populated: boolean) { + const driver: any = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(LEDGER_ORDER as any); + await engine.syncSchemas(); + if (populated) await engine.insert(OBJECT, ROWS as any); + + const calls = { aggregate: 0, find: 0 }; + const find = driver.find.bind(driver); + const aggregate = driver.aggregate.bind(driver); + driver.find = (o: string, ...rest: unknown[]) => { if (o === OBJECT) calls.find += 1; return find(o, ...rest); }; + driver.aggregate = (o: string, ...rest: unknown[]) => { if (o === OBJECT) calls.aggregate += 1; return aggregate(o, ...rest); }; + + 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(); + const post = async (body: Record) => { + const res = makeRes(); + // What the wire carries: JSON, so a `Date` would arrive as its ISO text. + await route!.handler({ params: { object: OBJECT }, body: JSON.parse(JSON.stringify(body)) } as any, res); + return res; + }; + return { post, calls }; +} + +const perAggregation = (filter: unknown) => ({ + groupBy: ['customer_id'], + aggregations: [{ function: 'count', alias: 'n' }, { function: 'count', alias: 'm', filter }], +}); +const whereTwin = (filter: unknown) => ({ where: filter, aggregations: [{ function: 'count', alias: 'n' }] }); + +describe('[#20148] POST /data/:object/query — a per-aggregation filter meets where\'s doors', () => { + const ROWS_REFUSED: ReadonlyArray, 'INVALID_FILTER' | 'INVALID_FIELD']> = [ + ['row 1 — a date no date field can read', { placed_on: { $gt: 'not-a-date' } }, 'INVALID_FILTER'], + ['row 1 — behind a $or branch that holds', { $or: [{ amount: { $gt: 0 } }, { placed_on: { $gt: 'not-a-date' } }] }, 'INVALID_FILTER'], + ['row 2 — addDays between two numeric fields', { amount: { $gt: { $field: 'cap', addDays: 1 } } }, 'INVALID_FILTER'], + ['row 2 — addDays between a date and a datetime', { placed_on: { $lte: { $field: 'opened_at', addDays: 1 } } }, 'INVALID_FILTER'], + ['row 3 — a { $field } naming no declared field', { amount: { $gt: { $field: 'nope' } } }, 'INVALID_FILTER'], + ['row 3 — the same, under $not', { $not: { amount: { $gt: { $field: 'nope' } } } }, 'INVALID_FILTER'], + ['row 4 — a key naming no field', { nope: 1 }, 'INVALID_FIELD'], + ['row 4 — the same, under $ne', { nope: { $ne: 1 } }, 'INVALID_FIELD'], + ]; + + for (const [name, filter, code] of ROWS_REFUSED) { + it(`${name}: 400 ${code} on an empty and a populated table, before any read — as its where twin`, async () => { + let message: string | undefined; + for (const populated of [false, true]) { + const { post, calls } = await boot(populated); + const res = await post(perAggregation(filter)); + expect(res._status, `populated=${populated}`).toBe(400); + expect(res._json.code, `populated=${populated}`).toBe(code); + expect(calls, `populated=${populated}`).toEqual({ aggregate: 0, find: 0 }); + message ??= res._json.error; + expect(res._json.error, 'one filter, one answer, whatever the rows').toBe(message); + + const twin = await post(whereTwin(filter)); + expect(twin._status, `twin, populated=${populated}`).toBe(400); + expect(twin._json.code, `twin, populated=${populated}`).toBe(code); + } + }); + } + + it('rows 2 and 3 withhold the names the where twin withholds — the fields and the operator stay off the wire', async () => { + const { post } = await boot(true); + for (const filter of [{ amount: { $gt: { $field: 'cap', addDays: 1 } } }, { amount: { $gt: { $field: 'nope' } } }]) { + const res = await post(perAggregation(filter)); + expect(res._json.code).toBe('INVALID_FILTER'); + expect(res._json.error).toContain('`aggregations[1].filter`'); + expect(res._json.error).toContain('the full diagnostic is in the server log'); + for (const name of ['"amount"', '"cap"', '"nope"', '$gt']) expect(res._json.error).not.toContain(name); + } + }); + + it('row 4 names the key and the position, as where\'s gate names its own parameter', async () => { + const { post } = await boot(true); + const res = await post(perAggregation({ nope: 1 })); + expect(res._json).toMatchObject({ code: 'INVALID_FIELD', field: 'nope', object: OBJECT }); + expect(res._json.error).toContain("'aggregations[1].filter'"); + }); + + const COUNTED: ReadonlyArray, Record]> = [ + ['a declared-field filter', { amount: { $gt: 100 } }, { c1: 1, c2: 2, c3: 0 }], + ['a { $field } between two declared numeric fields', { amount: { $gt: { $field: 'cap' } } }, { c1: 2, c2: 2, c3: 0 }], + ['addDays between two date fields', { placed_on: { $lte: { $field: 'due_on', addDays: 7 } } }, { c1: 2, c2: 2, c3: 1 }], + ['a { $field } to a column every row carries', { opened_at: { $lte: { $field: 'created_at' } } }, { c1: 2, c2: 3, c3: 1 }], + ['row 5\'s spelling on the wire — a datetime bound as ISO text', { opened_at: { $gt: '2026-02-01T00:00:00.000Z' } }, { c1: 0, c2: 3, c3: 1 }], + ]; + + for (const [name, filter, perGroup] of COUNTED) { + it(`${name}: counted, and the counts are the where twin's`, async () => { + const { post } = await boot(true); + const res = await post(perAggregation(filter)); + expect(res._status ?? 200).toBe(200); + const counts = Object.fromEntries((res._json.records as any[]).map((r) => [r.customer_id, r.m])); + expect(counts).toEqual(perGroup); + const twin = await post(whereTwin(filter)); + expect(twin._status ?? 200).toBe(200); + expect(twin._json.records).toEqual([{ n: Object.values(perGroup).reduce((a, b) => a + b, 0) }]); + }); + } +});