diff --git a/.changeset/21007-aggregation-filter-json-column-refusal.md b/.changeset/21007-aggregation-filter-json-column-refusal.md new file mode 100644 index 00000000000..3833781bcdd --- /dev/null +++ b/.changeset/21007-aggregation-filter-json-column-refusal.md @@ -0,0 +1,27 @@ +--- +"@objectstack/objectql": minor +"@objectstack/core": minor +"@objectstack/driver-sql": patch +--- + +fix(objectql)!: a per-aggregation `filter` refuses `$in` / `$nin` / `$eq` / `$ne` / an ordering / `$between` / implicit equality on a declared JSON-stored field with `INVALID_FILTER` / 400, in the words `where` refuses them in, instead of counting rows the stored arrays cannot support + +Clause-②: yes (widening) + + + +**BREAKING** (`@objectstack/objectql`): this narrows what `aggregate` accepts in one position, `aggregations[i].filter`, on every driver and for every caller that reaches the engine: the REST query door (`POST /api/v1/data/:object/query`), a flow or hook, and the analytics strategy that lowers a dataset measure's filter onto `engine.aggregate`. The published `applyInMemoryAggregation(rows, ast, timezone, fields)` narrows the same way when it is handed a field map. It ships as `minor` under the launch-window convention for accept-set narrowings. + +**What is refused.** On a field the object declares JSON-stored (a structured-JSON type such as `json` or `address`, an inherently multi-value option type such as `tags`, `multiselect` or `checkboxes`, or a `select`, `radio`, `lookup`, `user`, `file` or `image` field declared `multiple: true`), a per-aggregation `filter` that compares the field with `$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$between`, `$in`, `$nin` or implicit equality (`{ "owners": "u1" }`) is refused with `INVALID_FILTER` / 400, whatever the comparand (`null` and an empty list included), at any depth under `$and` / `$or` / `$not`, and before any driver is asked for a row, so an empty table refuses it too. That is the set `driver-sql`'s `where` refuses on such a column, for the same reason. + +**What an author sees now.** The same 400 body the same filter gets as a `where`: the filter WAS NOT APPLIED, the comparison can never equal one member of a stored list, and the spelling to use, `{ "FIELD": { "$contains": "a" } }` for membership, or an `$or` of `$contains` for any-of. The field and the operator are withheld from the message, as they are for `where`, and the full diagnostic, naming both and the aggregation position, goes to the server log. + +**Why a refusal.** The engine evaluates a per-aggregation filter itself, and it compared the whole stored array against a scalar. Measured through `POST /api/v1/data/:object/query` on SQLite and PostgreSQL 16 over six rows of a `multiple: true` lookup, two of them holding `u1`: `{ owners: { $in: ['u1', 'u9'] } }` counted 0, `{ owners: { $nin: ['u1', 'u9'] } }` counted all 6, the two rows it was asked to exclude among them, `$gt` / `$lte` / `$between` counted 4 / 1 / 5, and `{ tags: { $eq: 'red' } }` counted the row holding `['red']` by JS loose equality. The same filters in `where` were 400 on both dialects. + +**Who is affected.** A dashboard, report, dataset measure or caller whose per-aggregation filter compares a JSON-stored field with one of those operators and read the count as a real answer. Also a host calling `applyInMemoryAggregation` directly with a `fields` map: it now judges each `aggregations[i].filter` against that map before any row (an empty `rows` array included) and throws the same `INVALID_FILTER` / 400. It takes an optional fifth argument, `reportWithheld(diagnostic)`, which receives the withheld field, operator and position; without it the diagnostic is dropped. A call without `fields` judges nothing, as before. Write `$contains` for "holds this member", an `$or` of `$contains` for "holds any of these", and `$not` around either for the exclusion. + +**Unchanged.** `$contains` and `$notContains` (membership on such a field), `$exists`, `$null` and `$empty`; every operator on a field that is not JSON-stored; `having`; `where`; and a host whose engine has no declaration for the object, where nothing is judged. + +**`@objectstack/core`** (three new root exports): `JSON_COLUMN_INCOMPATIBLE_OPERATORS`, `jsonColumnOperatorRefusalText(field, op, bare)` and its return type `JsonColumnOperatorRefusalText` (`{ message, diagnostic }`). They are the operator set and the two texts (the withheld message and the full diagnostic) of the JSON-column refusal, so `driver-sql`'s `where` and the engine's per-aggregation filter refuse with one set and one sentence. + +**`@objectstack/driver-sql`**: no behaviour change. Its JSON-column gate reads the set and the text from `@objectstack/core`; every refusal it prints is byte for byte what it printed before. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index caa3f25436a..717fa46c332 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -125,6 +125,13 @@ export * from './utils/temporal-comparand.js'; // do not depend on each other, and each driver used to carry its own copy. export * from './utils/temporal-storage-form.js'; +// [#21007] …and the refusal a scalar comparison gets on a field stored as a +// JSON column: the operator set and the words. `driver-sql` refuses it on +// `where`, and `@objectstack/objectql` on the per-aggregation `filter` it +// evaluates itself — one set and one sentence, here for the reason the entry +// above gives. +export * from './utils/json-column-operator-refusal.js'; + // [#12350 / ADR-0126 §4] THE activation-ledger row contract, parameterized by // `metadata_type`. Same reason as the two entries above: its consumers — // `@objectstack/objectql` (packaged actions) and diff --git a/packages/core/src/utils/json-column-operator-refusal.test.ts b/packages/core/src/utils/json-column-operator-refusal.test.ts new file mode 100644 index 00000000000..e45e9ba83fc --- /dev/null +++ b/packages/core/src/utils/json-column-operator-refusal.test.ts @@ -0,0 +1,69 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#21007] The JSON-column refusal's operator set and words, moved here from + * `driver-sql` so the engine's per-aggregation `filter` refuses with them too. + * + * Two pins, both against what `driver-sql` answered BEFORE the move: + * + * - **The set** — the 22 spellings `driver-sql`'s module-private + * `JSON_COLUMN_INCOMPATIBLE_OPERATORS` held, member for member. + * - **The words** — the SHA-256 of each text, captured from `driver-sql`'s + * built `jsonColumnOperatorError` at the commit before the move (`8f784959c`) + * through a real `SqlDriver` over SQLite: the withheld message (one text for + * every operator), and the diagnostic for an operator, for `$between`, and + * for the bare equality spelling, on a column named `members`. A hash rather + * than a second literal copy, so this file is not a third place the sentence + * lives; the length beside each hash says how far a failure moved it. + * + * A deliberate change of wording updates the hashes in the PR that makes it — + * and then reaches `where` and the per-aggregation `filter` alike, which is the + * point of the move. + */ + +import { describe, it, expect } from 'vitest'; +import { createHash } from 'node:crypto'; +import { JSON_COLUMN_INCOMPATIBLE_OPERATORS, jsonColumnOperatorRefusalText } from './json-column-operator-refusal.js'; + +const sha256 = (text: string): string => createHash('sha256').update(text, 'utf8').digest('hex'); + +describe('[#21007] JSON_COLUMN_INCOMPATIBLE_OPERATORS', () => { + it('holds exactly the spellings driver-sql refused before the move', () => { + expect([...JSON_COLUMN_INCOMPATIBLE_OPERATORS].sort()).toEqual([ + '!=', '$between', '$eq', '$gt', '$gte', '$in', '$lt', '$lte', '$ne', '$nin', + '<', '<=', '<>', '=', '==', '>', '>=', + 'between', 'in', 'nin', 'not_in', 'notin', + ]); + }); + + it('leaves out the membership spelling, the rest of the text family and the null predicates', () => { + for (const op of ['$contains', '$notContains', '$startsWith', '$endsWith', '$icontains', '$null', '$exists', '$empty']) { + expect(JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op), op).toBe(false); + } + }); +}); + +describe('[#21007] jsonColumnOperatorRefusalText — byte for byte what driver-sql printed before the move', () => { + const MESSAGE = { sha: 'c6103dd665625ab3a779822fecbe650bd7f67fc6ea38d17d5fd57cf6ae9193ff', length: 748 }; + + it.each([ + ['an operator', '$in', false, { sha: '358d5aae39170ab3da198beb39368a3f3476dd057dbd5da5f4295cf244eab67e', length: 648 }], + ['$between', '$between', false, { sha: '505c094ac5212ac121cdeb4803787ba036f46177450d542fbccf60a9386924fe', length: 658 }], + ['the bare equality spelling', '=', true, { sha: '92bb1728ca012c4cde6deb05be3cb0320e113c8c5ac250ecad5b091f3bb87094', length: 660 }], + ] as const)('%s', (_name, op, bare, diagnostic) => { + const text = jsonColumnOperatorRefusalText('members', op, bare); + expect({ sha: sha256(text.message), length: text.message.length }).toEqual(MESSAGE); + expect({ sha: sha256(text.diagnostic), length: text.diagnostic.length }).toEqual(diagnostic); + }); + + it('the message names neither the field nor the operator, and prescribes $contains and an $or of it', () => { + const { message, diagnostic } = jsonColumnOperatorRefusalText('secret_col', '$nin', false); + expect(message).not.toContain('secret_col'); + expect(message).not.toContain('"$nin"'); + expect(message).toContain('WAS NOT APPLIED'); + expect(message).toContain('{ "FIELD": { "$contains": "a" } }'); + expect(message).toContain('{ "$or": [{ "FIELD": { "$contains": "a" } }'); + expect(diagnostic).toContain('Operator "$nin" on field "secret_col"'); + expect(diagnostic).toContain('{ "secret_col": { "$contains": "a" } }'); + }); +}); diff --git a/packages/core/src/utils/json-column-operator-refusal.ts b/packages/core/src/utils/json-column-operator-refusal.ts new file mode 100644 index 00000000000..7bf8d41ca42 --- /dev/null +++ b/packages/core/src/utils/json-column-operator-refusal.ts @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#21007] The refusal a SCALAR comparison operator gets when it is aimed at a + * field stored as a JSON column — a `multiple: true` field, an inherently + * multi-value option type (`tags`, `multiselect`, `checkboxes`) or a + * structured-JSON type (`json`, `address`, …): the operator set and the words. + * + * ## Two faces, one rule + * + * `driver-sql` refuses these operators on its `where` (#7398): such a column + * holds the serialization `["a","b"]`, so `$in` / `$eq` compare that whole text + * against one value and match nothing, while `$nin` / `$ne` return the very rows + * they were asked to exclude, and the orderings return a lexicographic verdict + * over the serialization. `@objectstack/objectql` evaluates a per-aggregation + * `filter` itself, row by row, and gave the same three wrong answers in JS — + * `{ owners: { $nin: ['u1'] } }` counted the rows holding `u1`. It now refuses + * the same operators on the same declared fields, before any driver is asked. + * + * The two faces cannot import each other (the engine does not depend on a + * driver), and a copy each is how one refusal comes to answer one mistake in + * two ways. So the set and the sentence live here, on the floor both already + * stand on — beside `temporalStorageForm`, which the same two faces share for + * the same reason. Each face keeps its own error CONSTRUCTOR (the driver's + * carries the #8220 provenance seam, the engine's the ADR-0112 envelope); what + * they share is what the caller reads. + * + * ## The other half of the JSON column's contract + * + * `$contains` is the membership spelling on such a column (`FILTER_OPERATORS`' + * `$contains` docblock, `@objectstack/spec`), and it is what the refusal + * prescribes — `$contains` for one member, an `$or` of `$contains` for any-of. + * That is why it is ABSENT from the set below, with the rest of the text family + * and the null predicates. + */ + +/** + * [#7398] Operators whose SQL lowering compares a column's STORED SCALAR to a + * value — every spelling either of `driver-sql`'s two comparison emitters + * answers (`applyFilterCondition`'s plain-column switch and + * `applyNormalizedComparison`'s normalised arms). + * + * The bare infix forms are here for the same reason they are in `driver-sql`'s + * `SCALAR_COMPARAND_OPERATORS`: `applyNormalizedComparison` really does + * answer `in` / `nin` / `not_in` / `notin` / `=` / `<>` / `>` …, so a filter + * spelled that way against a normalised column compiles, and a gate that only + * knew the `$`-forms would leave the failure alive at a different spelling — + * the lesson #5234 already paid for there. + * + * `$between` is included although the card's minimum set stopped at the four + * ordering comparisons: it IS `>= AND <=` (`driver-sql` even decomposes a + * calendar-day `$between` into `$gte`/`$lt` ahead of its emitter), so refusing + * the halves and compiling the compound would be the same wrong answer at one + * more spelling. + * + * Deliberately ABSENT, and this is the load-bearing half of the set: the `LIKE` + * family (`$contains`, `$notContains`, `$startsWith`, `$endsWith`, + * `$icontains`) and the null predicates (`$null`, `$exists`). `$contains` is + * the ONLY working membership spelling on a JSON-array column and downstream + * code depends on it (#7398's own tables), while `IS NULL` asks about the + * column's presence, which is a well-formed question whatever the column holds. + * + * [#21007] Moved here from `driver-sql`, unchanged, so the per-aggregation + * `filter` refuses exactly the operators `where` refuses. + */ +export const JSON_COLUMN_INCOMPATIBLE_OPERATORS: ReadonlySet = new Set([ + '$eq', '=', '==', + '$ne', '!=', '<>', + '$gt', '>', '$gte', '>=', '$lt', '<', '$lte', '<=', + '$in', 'in', + '$nin', 'nin', 'not_in', 'notin', + '$between', 'between', +]); + +/** The two texts of one JSON-column refusal — see {@link jsonColumnOperatorRefusalText}. */ +export interface JsonColumnOperatorRefusalText { + /** + * What the caller is told. It names neither the field nor the operator: on a + * read scope the predicate is an administrator's, so both are withheld + * (#7929 / #8197), and the sentence says where they went. + */ + readonly message: string; + /** The full diagnostic — the field and the operator named — for the server log. */ + readonly diagnostic: string; +} + +/** + * [#7398] The words of the refusal: a scalar-comparison operator met a field + * stored as JSON TEXT, so the comparison can never mean what the caller wrote. + * + * The mechanism is one line of SQL. A `multiple: true` field is stored as the + * serialization `["U1","U2"]`, so `members in ('U1')` is FALSE — the text + * genuinely is not equal to that id — and `members not in ('U1')` is TRUE: + * + * - `$in` / `$eq` / bare equality → **0 rows**, fail-CLOSED. Silent, and a + * `200` with an empty array is byte-identical to a query that legitimately + * matched nothing, so no caller has anything to key on. + * - `$nin` / `$ne` → **the row it was asked to exclude**, fail-OPEN. That is + * the dangerous half and the reason this is a refusal rather than a + * documented footgun: an exclusion that silently stops excluding WIDENS a + * result set, the direction #3948 / #4209 / #5347 all ruled outranks a + * narrowing one. + * - The ordering comparisons are not even uniformly empty: `$lte` matched, + * because `["usr_…"` sorts below `usr_…` on the leading `[`. A lexicographic + * compare over a serialization is a wrong answer, not a narrow one. + * + * The message states the filter WAS NOT APPLIED, because "no rows" is a + * legitimate answer to a legitimate query, so a caller must be told that this + * one was never asked. The prescription is `$contains` (and an `$or` of + * `$contains` for any-of). It survives redaction with PLACEHOLDER names — the + * SHAPE is the repair, and the shape names nothing. + * + * `bare` is the implicit-equality spelling `{ field: value }`, whose operator + * the diagnostic names as `=`. + * + * [#21007] Moved here from `driver-sql`'s `jsonColumnOperatorError`, byte for + * byte, so `where` and the per-aggregation `filter` print one sentence. The + * caller builds the error: this returns only the text. + */ +export function jsonColumnOperatorRefusalText( + field: string, + op: string, + bare: boolean, +): JsonColumnOperatorRefusalText { + const spelling = bare + ? `The bare equality spelling { "${field}": value }` + : `Operator "${op}"`; + const on = bare ? '' : ` on field "${field}"`; + return { + message: + `A constraint in this filter WAS NOT APPLIED: it aims a scalar comparison operator at a ` + + `field this driver stores as a JSON TEXT column (e.g. ["a","b"]), and such an operator ` + + `compares that whole serialized text against a single value — it can never equal one ` + + `member. Use "$contains" for membership ({ "FIELD": { "$contains": "a" } }), or an $or of ` + + `"$contains" for any-of ({ "$or": [{ "FIELD": { "$contains": "a" } }, ` + + `{ "FIELD": { "$contains": "b" } }] }). Refused rather than compiled because the answer ` + + `was silently wrong in BOTH directions: $in/$eq matched nothing, while $nin/$ne returned ` + + `the very rows they were asked to exclude. The field and the operator this filter ` + + `used are withheld from the message; the full diagnostic is in the server log.`, + diagnostic: + `${spelling}${on} WAS NOT APPLIED: "${field}" is a multi-value (or otherwise JSON-valued) ` + + `field, stored by this driver as a JSON TEXT column (e.g. ["a","b"]), and "${op}" compares ` + + `that whole serialized text against a single value — it can never equal one member. ` + + `Use "$contains" for membership ({ "${field}": { "$contains": "a" } }), or an $or of ` + + `"$contains" for any-of ({ "$or": [{ "${field}": { "$contains": "a" } }, ` + + `{ "${field}": { "$contains": "b" } }] }). Refused rather than compiled because the answer ` + + `was silently wrong in BOTH directions: $in/$eq matched nothing, while $nin/$ne returned ` + + `the very rows they were asked to exclude.`, + }; +} diff --git a/packages/drivers/driver-sql/src/sql-driver-json-column-refusal-shared-text.test.ts b/packages/drivers/driver-sql/src/sql-driver-json-column-refusal-shared-text.test.ts new file mode 100644 index 00000000000..202ffb553c2 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-json-column-refusal-shared-text.test.ts @@ -0,0 +1,88 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#21007] The JSON-column gate's set and words are `@objectstack/core`'s now — + * this driver's `where` and `@objectstack/objectql`'s per-aggregation `filter` + * refuse with ONE set and ONE sentence. This file pins the driver half of the + * move: what it throws IS the shared text, and what it refuses IS the shared + * set, on a real `SqlDriver` over SQLite. + * + * That the shared text is byte for byte what this driver printed before the move + * is pinned beside the text itself (`@objectstack/core`'s + * `json-column-operator-refusal.test.ts`, hashes captured from this driver at the + * commit before). Together: this driver's refusal did not change by one byte. + * + * The per-operator content of the refusal (code, status, the prescription, every + * lowering face) stays pinned in `sql-driver-json-column-operator-refusal.test.ts`. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { JSON_COLUMN_INCOMPATIBLE_OPERATORS, jsonColumnOperatorRefusalText } from '@objectstack/core'; +import { FILTER_OPERATORS, type FilterCondition } from '@objectstack/spec/data'; +import { SqlDriver, withheldFilterDiagnosticOf } from './index.js'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +const LIST_OPERATORS = new Set(['$in', '$nin']); + +function comparandFor(op: string): unknown { + if (LIST_OPERATORS.has(op)) return ['usr_1']; + if (op === '$between') return ['a', 'b']; + if (op === '$exists' || op === '$null' || op === '$empty') return true; + return 'usr_1'; +} + +describe('[#21007] driver-sql — the JSON-column refusal is the shared set and the shared text', () => { + let driver: SqlDriver; + + beforeAll(async () => { + driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await driver.syncSchema('team', { + name: 'team', + fields: { name: { type: 'text' }, members: { type: 'lookup', reference: 'user', multiple: true } }, + } as any); + }); + afterAll(async () => { + await (driver as any)?.disconnect?.(); + }); + + async function refusalOf(where: Record): Promise { + try { + await driver.find('team', { where: where as FilterCondition }); + return null; + } catch (e) { + return e as WireBearingError; + } + } + + // Every declared operator, judged by whether the shared set names it — so a + // member added to or dropped from the set moves this driver's answer with it. + for (const op of FILTER_OPERATORS) { + const refused = JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op); + it(`${op}: ${refused ? 'refused with the shared text' : 'not this refusal'}`, async () => { + const err = await refusalOf({ members: { [op]: comparandFor(op) } }); + const shared = jsonColumnOperatorRefusalText('members', op, false); + if (!refused) { + expect(err?.message ?? '').not.toBe(shared.message); + return; + } + expect(err, 'refused').not.toBeNull(); + expect(err!.code).toBe('INVALID_FILTER'); + expect(err!.status).toBe(400); + expect(err!.message).toBe(shared.message); + expect(withheldFilterDiagnosticOf(err)).toBe(shared.diagnostic); + }); + } + + it('the bare equality spelling: refused with the shared text, operator "="', async () => { + const err = await refusalOf({ members: 'usr_1' }); + const shared = jsonColumnOperatorRefusalText('members', '=', true); + expect(err?.code).toBe('INVALID_FILTER'); + expect(err?.status).toBe(400); + expect(err?.message).toBe(shared.message); + expect(withheldFilterDiagnosticOf(err)).toBe(shared.diagnostic); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 60b9ceaf529..f01da213a08 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -150,6 +150,10 @@ import { AsyncLocalStorage } from 'node:async_hooks'; import { createHash } from 'node:crypto'; import { existsSync } from 'node:fs'; import { currentPerfTiming, perfNow, type PerfTiming } from '@objectstack/observability'; +// [#21007] The JSON-column gate's operator set and refusal text — shared with +// `@objectstack/objectql`'s per-aggregation `filter`, which refuses the same +// operators on the same declared fields. See {@link jsonColumnOperatorError}. +import { JSON_COLUMN_INCOMPATIBLE_OPERATORS, jsonColumnOperatorRefusalText } from '@objectstack/core'; /** * [#20768] The async scope of a driver's own PRE-DDL question: the ADR-0104 @@ -3294,108 +3298,43 @@ function unrenderableTextComparandError( } /** - * [#7398] Operators whose SQL lowering compares a column's STORED SCALAR to a - * value — every spelling either of this driver's two comparison emitters - * answers ({@link SqlDriver.applyFilterCondition}'s plain-column switch and - * {@link SqlDriver.applyNormalizedComparison}'s normalised arms). - * - * The bare infix forms are here for the same reason they are in - * {@link SCALAR_COMPARAND_OPERATORS}: `applyNormalizedComparison` really does - * answer `in` / `nin` / `not_in` / `notin` / `=` / `<>` / `>` …, so a filter - * spelled that way against a normalised column compiles, and a gate that only - * knew the `$`-forms would leave the failure alive at a different spelling — - * the lesson #5234 already paid for in this file. - * - * `$between` is included although the card's minimum set stopped at the four - * ordering comparisons: it IS `>= AND <=` (this driver even decomposes a - * calendar-day `$between` into `$gte`/`$lt` a few lines above the emitter), so - * refusing the halves and compiling the compound would be the same wrong answer - * at one more spelling. - * - * Deliberately ABSENT, and this is the load-bearing half of the set: the `LIKE` - * family (`$contains`, `$notContains`, `$startsWith`, `$endsWith`, - * `$icontains`) and the null predicates (`$null`, `$exists`). `$contains` is - * the ONLY working membership spelling on a JSON-array column and downstream - * code depends on it (#7398's own tables), while `IS NULL` asks about the - * column's presence, which is a well-formed question whatever the column holds. + * [#7398] The column-type gate's operator set — `JSON_COLUMN_INCOMPATIBLE_OPERATORS` + * — and the words of its refusal live in `@objectstack/core` + * (`json-column-operator-refusal.ts`) since #21007, imported above under the + * same name, unchanged. They moved there because `@objectstack/objectql` + * evaluates a per-aggregation `filter` itself and now refuses the same operators + * on the same declared fields: one set and one sentence for both faces, rather + * than a copy each. The reasoning behind every member and every sentence is in + * that module's docblocks. */ -const JSON_COLUMN_INCOMPATIBLE_OPERATORS: ReadonlySet = new Set([ - '$eq', '=', '==', - '$ne', '!=', '<>', - '$gt', '>', '$gte', '>=', '$lt', '<', '$lte', '<=', - '$in', 'in', - '$nin', 'nin', 'not_in', 'notin', - '$between', 'between', -]); /** * [#7398] A scalar-comparison operator met a column this driver stores as JSON - * TEXT, so the comparison can never mean what the caller wrote. - * - * The mechanism is one line of SQL. A `multiple: true` field is stored as the - * serialization `["U1","U2"]`, so `members in ('U1')` is FALSE — the text - * genuinely is not equal to that id — and `members not in ('U1')` is TRUE. The - * measured consequences on the issue's fixture, one row whose `members` - * contains `U1`: - * - * - `$in` / `$eq` / bare equality → **0 rows**, fail-CLOSED. Silent, and a - * `200` with an empty array is byte-identical to a query that legitimately - * matched nothing, so no caller has anything to key on. - * - `$nin` / `$ne` → **the row it was asked to exclude**, fail-OPEN. That is - * the dangerous half and the reason this is a refusal rather than a - * documented footgun: an exclusion that silently stops excluding WIDENS a - * result set, the direction #3948 / #4209 / #5347 all ruled outranks a - * narrowing one. The issue's downstream delete-guard - * (`{ assignees: { $in: memberIds } }`) never fired once since it shipped. - * - The ordering comparisons are not even uniformly empty: `$lte` matched, - * because `["usr_…"` sorts below `usr_…` on the leading `[`. A lexicographic - * compare over a serialization is a wrong answer, not a narrow one. + * TEXT, so the comparison can never mean what the caller wrote — the measured + * consequences are on {@link jsonColumnOperatorRefusalText}. * * ADR-0112 class 1 — a caller mistake, so `INVALID_FILTER` / 400, the same * envelope {@link unsupportedFilterError} gives the unknown-operator refusal - * one arm over. The message states the filter WAS NOT APPLIED for the reason - * that wording exists on the comparand-shape refusals: "no rows" is a - * legitimate answer to a legitimate query, so a caller must be told that this - * one was never asked. + * one arm over. * * The prescription is `$contains` (and an `$or` of `$contains` for any-of) - * because that is the only spelling that works today — it lowers to - * `LIKE '%v%'` over the serialization. That it works at all is incidental - * rather than designed, which is the issue's ask 2 (`$overlaps` / - * `$containsAny`); ask 2 is a closed-spec-set question and is deliberately not - * answered here. + * because that is the membership spelling on such a column (the spec's + * `$contains` docblock); a closed-spec-set `$overlaps` / `$containsAny` is the + * issue's ask 2 and is deliberately not answered here. * * [#8197] The most reachable member of {@link refusalSubtree}'s family, and the * one the card led with: a CEL permission rule over a multi-select field lowers * to exactly this membership test, so the column this gate names is the * administrator's. The prescription survives redaction with PLACEHOLDER names — * the SHAPE is the repair, and the shape names nothing. + * + * [#21007] The TEXT is `@objectstack/core`'s, byte for byte what this builder + * spelled before the move; the CONSTRUCTOR stays here, because the #8220 + * provenance seam it goes through is this driver's. */ function jsonColumnOperatorError(field: string, op: string, bare: boolean, subtree?: unknown): Error { - const spelling = bare - ? `The bare equality spelling { "${field}": value }` - : `Operator "${op}"`; - const on = bare ? '' : ` on field "${field}"`; - return withheldFilterError( - `A constraint in this filter WAS NOT APPLIED: it aims a scalar comparison operator at a ` + - `field this driver stores as a JSON TEXT column (e.g. ["a","b"]), and such an operator ` + - `compares that whole serialized text against a single value — it can never equal one ` + - `member. Use "$contains" for membership ({ "FIELD": { "$contains": "a" } }), or an $or of ` + - `"$contains" for any-of ({ "$or": [{ "FIELD": { "$contains": "a" } }, ` + - `{ "FIELD": { "$contains": "b" } }] }). Refused rather than compiled because the answer ` + - `was silently wrong in BOTH directions: $in/$eq matched nothing, while $nin/$ne returned ` + - `the very rows they were asked to exclude. The field and the operator this filter ` + - `used are withheld from the message; the full diagnostic is in the server log.`, - `${spelling}${on} WAS NOT APPLIED: "${field}" is a multi-value (or otherwise JSON-valued) ` + - `field, stored by this driver as a JSON TEXT column (e.g. ["a","b"]), and "${op}" compares ` + - `that whole serialized text against a single value — it can never equal one member. ` + - `Use "$contains" for membership ({ "${field}": { "$contains": "a" } }), or an $or of ` + - `"$contains" for any-of ({ "$or": [{ "${field}": { "$contains": "a" } }, ` + - `{ "${field}": { "$contains": "b" } }] }). Refused rather than compiled because the answer ` + - `was silently wrong in BOTH directions: $in/$eq matched nothing, while $nin/$ne returned ` + - `the very rows they were asked to exclude.`, - subtree, - ); + const { message, diagnostic } = jsonColumnOperatorRefusalText(field, op, bare); + return withheldFilterError(message, diagnostic, subtree); } /** A short, non-throwing rendering of an offending comparand for the message. */ diff --git a/packages/objectql/src/engine-aggregate-filter-json-column-refusal.test.ts b/packages/objectql/src/engine-aggregate-filter-json-column-refusal.test.ts new file mode 100644 index 00000000000..10cdf6c51a5 --- /dev/null +++ b/packages/objectql/src/engine-aggregate-filter-json-column-refusal.test.ts @@ -0,0 +1,336 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#21007] A per-aggregation `filter` REFUSES a scalar comparison on a field the +// object declares JSON-stored — the equality and ordering family, `$between`, +// `$in` / `$nin` and implicit equality — with `where`'s code and words, before +// any driver is asked. `$contains` / `$notContains` (membership), `$exists`, +// `$null` and `$empty` keep answering, and a scalar column is untouched. +// +// Measured before this change (`origin/main` `d1f8ce865`) +// through `POST /data/:object/query` on SQLite and a live PostgreSQL 16.14, on +// the six rows below: `where` refused every one of these 400 `INVALID_FILTER` on +// both dialects, while the per-aggregation `filter` answered 200 with a count +// the stored arrays cannot support — +// +// | per-aggregation `filter` | base `m` (SQLite = PostgreSQL = this engine) | +// |:--|:--| +// | `owners $in ['u1','u9']` (the card) | 0 | +// | `owners $nin ['u1','u9']` (the card) | 6 — `d1` and `d3`, the rows it was asked to exclude | +// | `owners $eq 'u1'` / `{ owners: 'u1' }` | 0 | +// | `owners $ne 'u1'` | 6 | +// | `owners $gt 'u1'` / `$lte 'u1'` / `$between ['u1','u9']` | 4 / 1 / 5 | +// | `tags $eq 'red'` | 1 — `d5`'s `['red']` loosely `==` `'red'` | +// | `meta $eq 'a'` / `$in ['a']` | 0 | +// +// This file is the engine-level cell, over the read shape `find()` presents (a +// driver without `aggregate()`, so the engine reads rows and lowers in memory — +// the one face that evaluates `aggregations[i].filter`); the SQLite and +// PostgreSQL cells, each beside its live `where` twin, are `packages/rest`'s +// `aggregation-filter-json-column-refusal.test.ts`. + +import { describe, it, expect, vi } from 'vitest'; +import { lowerFilterCondition } from '@objectstack/spec/data'; +import { ObjectQL } from './engine.js'; +import { declaredJsonStoredFields, matchesAggregationFilter } from './having-filter.js'; +import { applyInMemoryAggregation } from './in-memory-aggregation.js'; + +const OBJECT = 'os21007_doc'; + +const FIELDS = { + title: { type: 'text' }, + owners: { type: 'lookup', reference: 'os21007_user', multiple: true }, + tags: { type: 'tags' }, + meta: { type: 'json' }, +}; + +const ROWS = [ + { id: 'd1', title: 'u1 memo', owners: ['u1', 'u2'], tags: ['red', 'blue'], meta: null }, + { id: 'd2', title: 'none', owners: ['u2'], tags: ['blue'], meta: null }, + { id: 'd3', title: 'about u10', owners: ['u3', 'u1'], tags: ['redwood'], meta: null }, + { id: 'd4', title: 'x', owners: [], tags: [], meta: null }, + { id: 'd5', title: 'u1', owners: ['u10'], tags: ['red'], meta: null }, + { id: 'd6', title: null, owners: null, tags: null, meta: null }, +]; + +/** A driver WITHOUT `aggregate()`: the engine reads rows and lowers in memory, where the filter is evaluated. */ +function makeRowsDriver(rows: readonly Record[]) { + const find = vi.fn(async () => rows.map((row) => ({ ...row }))); + return { + name: 'rows-mock', + version: '0.0.0', + supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + find, + 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 0; }, + async bulkCreate(_o: string, r: any[]) { return r; }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; +} + +async function makeEngine(rows: readonly Record[]) { + const driver = makeRowsDriver(rows); + const engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + (engine.registry as any).registerObject({ name: OBJECT, fields: FIELDS }); + const warn = vi.spyOn((engine as any).logger, 'warn').mockImplementation(() => undefined); + return { engine, driver, warn }; +} + +const perAggregation = (filter: unknown) => ({ + aggregations: [ + { function: 'count' as const, alias: 'n' }, + { function: 'count' as const, alias: 'm', filter: filter as never }, + ], +}); + +/** Each field's member and a non-member, so every comparand is one a real author could write. */ +const VALUES: Record = { + owners: ['u1', 'u9'], + tags: ['red', 'green'], + meta: ['a', 'b'], +}; + +/** field → [name, filter, the operator the refusal is about, bare?] */ +function family(field: string): Array, string, boolean]> { + const [a, b] = VALUES[field]; + return [ + ['implicit equality', { [field]: a }, '=', true], + ['implicit equality with null', { [field]: null }, '=', true], + ['$eq', { [field]: { $eq: a } }, '$eq', false], + ['$eq null', { [field]: { $eq: null } }, '$eq', false], + ['$ne', { [field]: { $ne: a } }, '$ne', false], + ['$ne null', { [field]: { $ne: null } }, '$ne', false], + ['$gt', { [field]: { $gt: a } }, '$gt', false], + ['$gte', { [field]: { $gte: a } }, '$gte', false], + ['$lt', { [field]: { $lt: a } }, '$lt', false], + ['$lte', { [field]: { $lte: a } }, '$lte', false], + ['$between', { [field]: { $between: [a, b] } }, '$between', false], + ['$in (the card)', { [field]: { $in: [a, b] } }, '$in', false], + ['$nin (the card)', { [field]: { $nin: [a, b] } }, '$nin', false], + ['$in []', { [field]: { $in: [] } }, '$in', false], + ['$nin []', { [field]: { $nin: [] } }, '$nin', false], + ['$in under $not', { $not: { [field]: { $in: [a] } } }, '$in', false], + ['$nin in an $or branch after one that holds', { $or: [{ title: 'x' }, { [field]: { $nin: [a] } }] }, '$nin', false], + ['$in beside $exists', { [field]: { $exists: true, $in: [a] } }, '$in', false], + ]; +} + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e as Error & { code?: string; status?: number }; + } + throw new Error('expected engine.aggregate to refuse this filter, but it answered'); +} + +/** + * The refusal, as `where` gives it: the ADR-0112 identity, the statement that + * the filter was not applied, and the prescription — with the field and the + * operator withheld from the message and named in the logged diagnostic. + */ +function expectWhereRefusal( + err: Error & { code?: string; status?: number }, + warn: ReturnType, + field: string, + op: string, + bare: boolean, +): void { + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('WAS NOT APPLIED'); + expect(err.message).toContain('{ "FIELD": { "$contains": "a" } }'); + expect(err.message).toContain('{ "$or": [{ "FIELD": { "$contains": "a" } }, { "FIELD": { "$contains": "b" } }] }'); + expect(err.message).not.toContain(`"${field}"`); + const logged = warn.mock.calls.map((call: unknown[]) => String(call[0])).join('\n'); + expect(logged).toContain('INVALID_FILTER — refusal detail withheld from the response'); + expect(logged).toContain(bare + ? `The bare equality spelling { "${field}": value } WAS NOT APPLIED` + : `Operator "${op}" on field "${field}" WAS NOT APPLIED`); + expect(logged).toContain(`{ "${field}": { "$contains": "a" } }`); +} + +describe('[#21007] engine.aggregate — a per-aggregation filter refuses a scalar comparison on a declared JSON-stored field, as where does', () => { + for (const field of ['owners', 'tags', 'meta']) { + for (const [name, filter, op, bare] of family(field)) { + it(`${field} ${name}: 400 INVALID_FILTER in where's words, and no row is read`, async () => { + const { engine, driver, warn } = await makeEngine(ROWS); + const err = await refusalOf(() => engine.aggregate(OBJECT, perAggregation(filter))); + expectWhereRefusal(err, warn, field, op, bare); + // Judged on the FILTER, before the driver is asked for a row. + expect(driver.find).not.toHaveBeenCalled(); + }); + } + } + + it('the card on an EMPTY table: refused too — the verdict is the filter\'s, not the data\'s', async () => { + for (const filter of [{ owners: { $in: ['u1', 'u9'] } }, { owners: { $nin: ['u1', 'u9'] } }]) { + const { engine, warn } = await makeEngine([]); + expectWhereRefusal(await refusalOf(() => engine.aggregate(OBJECT, perAggregation(filter))), warn, 'owners', + Object.keys(filter.owners)[0], false); + const grouped = await makeEngine([]); + const err = await refusalOf(() => grouped.engine.aggregate(OBJECT, { + groupBy: ['title'], + aggregations: [{ function: 'count', alias: 'm', filter: filter as never }], + })); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + } + }); + + it('the logged diagnostic names the aggregation position the filter sits at', async () => { + const { engine, warn } = await makeEngine(ROWS); + await refusalOf(() => engine.aggregate(OBJECT, { + aggregations: [ + { function: 'count', alias: 'n' }, + { function: 'count', alias: 'k', filter: { title: { $eq: 'x' } } as never }, + { function: 'count', alias: 'm', filter: { owners: { $nin: ['u1'] } } as never }, + ], + })); + expect(warn.mock.calls.map((call: unknown[]) => String(call[0])).join('\n')).toContain('At aggregations[2].filter.owners.$nin:'); + }); +}); + +describe('[#21007] what still answers — the membership spelling, the null predicates, and every scalar column', () => { + const ANSWERED: ReadonlyArray, number]> = [ + ['owners $contains — the prescribed spelling', { owners: { $contains: 'u1' } }, 2], + ['owners $notContains', { owners: { $notContains: 'u1' } }, 4], + ['owners an $or of $contains — the any-of spelling', { $or: [{ owners: { $contains: 'u1' } }, { owners: { $contains: 'u3' } }] }, 2], + ['owners $exists', { owners: { $exists: true } }, 5], + ['owners $null', { owners: { $null: true } }, 1], + ['owners $empty', { owners: { $empty: true } }, 2], + ['control — title $in', { title: { $in: ['u1', 'x'] } }, 2], + ['control — title $nin', { title: { $nin: ['u1', 'x'] } }, 4], + ['control — title $eq', { title: { $eq: 'x' } }, 1], + ['control — title implicit equality', { title: 'x' }, 1], + ['control — title $gt', { title: { $gt: 'u1' } }, 2], + ]; + for (const [name, filter, m] of ANSWERED) { + it(`${name}: m = ${m}`, async () => { + const { engine, warn } = await makeEngine(ROWS); + expect(await engine.aggregate(OBJECT, perAggregation(filter))).toEqual([{ n: 6, m }]); + expect(warn.mock.calls.map((call: unknown[]) => String(call[0])).join('\n')).not.toContain('INVALID_FILTER'); + }); + } +}); + +describe('[#21007] the per-row floor — a caller evaluating rows directly meets the same refusal', () => { + const jsonStored = declaredJsonStoredFields(FIELDS); + + it.each([ + ['$in', { owners: { $in: ['u1'] } }], + ['$nin', { owners: { $nin: ['u1'] } }], + ['$eq', { tags: { $eq: 'red' } }], + ['implicit equality', { tags: 'red' }], + ['$between', { meta: { $between: ['a', 'b'] } }], + ] as const)('%s', (_name, filter) => { + let thrown: (Error & { code?: string; status?: number }) | undefined; + try { + matchesAggregationFilter(ROWS[0], filter as never, 0, undefined, jsonStored); + } catch (e) { + thrown = e as Error & { code?: string; status?: number }; + } + expect(thrown?.code).toBe('INVALID_FILTER'); + expect(thrown?.status).toBe(400); + expect(thrown?.message).toContain('{ "FIELD": { "$contains": "a" } }'); + }); + + it('a row with NO value refuses too: the verdict is above the no-value exit', () => { + expect(() => matchesAggregationFilter(ROWS[5], { owners: { $in: ['u1'] } } as never, 0, undefined, jsonStored)) + .toThrow(/WAS NOT APPLIED/); + }); + + it('undeclared (no jsonStored set): the walker answers as it always did', () => { + expect(matchesAggregationFilter(ROWS[0], { owners: { $in: ['u1'] } } as never, 0)).toBe(false); + }); +}); + +describe('[#21007] applyInMemoryAggregation — a direct caller with a field map meets the same refusal, before any row is judged', () => { + // `applyInMemoryAggregation` is published (objectql's root and `./core`), so a + // host can reach the walker with a field map and no engine in front of it. + // It calls the engine's own gate once per filter; without that call the + // per-row backstop answered both cells below 200 — an empty row set has no + // row to reach it, and on the LOWERED filter (the shape every engine seam + // hands the evaluator: spec `lowerFilterCondition`, rule 3) a row with no + // value is decided by the `$null` arm before the comparison is walked. + const NEGATIONS: ReadonlyArray, string]> = [ + ['$ne', { meta: { $ne: 'a' } }, '$ne'], + ['$nin', { meta: { $nin: ['a', 'b'] } }, '$nin'], + ['$not $in', { $not: { meta: { $in: ['a'] } } }, '$in'], + ]; + const NULL_META_ROWS = ROWS.map((row) => ({ ...row, meta: null })); + const ast = (filter: unknown, groupBy?: string[]) => ({ + ...(groupBy ? { groupBy } : {}), + aggregations: [ + { function: 'count' as const, alias: 'n' }, + { function: 'count' as const, alias: 'm', filter: filter as never }, + ], + }); + + function refusalFrom(run: () => unknown): Error & { code?: string; status?: number } { + try { + run(); + } catch (e) { + return e as Error & { code?: string; status?: number }; + } + throw new Error('expected applyInMemoryAggregation to refuse this filter, but it answered'); + } + + async function engineMessageFor(filter: unknown): Promise { + const { engine } = await makeEngine([]); + return (await refusalOf(() => engine.aggregate(OBJECT, perAggregation(filter)))).message; + } + + for (const [name, filter, op] of NEGATIONS) { + const lowered = lowerFilterCondition(filter); + + it(`${name} — the lowered filter carries the $null arm the walker short-circuits on`, () => { + expect(JSON.stringify(lowered)).toContain('"$null"'); + }); + + const cells: ReadonlyArray<{ cell: string; rows: Record[]; spelled: unknown; groupBy?: string[] }> = [ + { cell: 'an EMPTY row set', rows: [], spelled: filter }, + { cell: 'an EMPTY row set, grouped', rows: [], spelled: filter, groupBy: ['title'] }, + { cell: 'meta null in every row, the lowered filter', rows: NULL_META_ROWS, spelled: lowered }, + { cell: 'meta null in every row, the filter as written', rows: NULL_META_ROWS, spelled: filter }, + ]; + for (const { cell, rows, spelled, groupBy } of cells) { + it(`${name} on ${cell}: 400 INVALID_FILTER, engine.aggregate's very message, the diagnostic to reportWithheld`, async () => { + const reported: string[] = []; + const err = refusalFrom(() => applyInMemoryAggregation( + rows.map((row) => ({ ...row })), ast(spelled, groupBy), undefined, FIELDS, + (diagnostic) => { reported.push(diagnostic); }, + )); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toBe(await engineMessageFor(filter)); + expect(err.message).toContain('{ "FIELD": { "$contains": "a" } }'); + expect(err.message).not.toContain('"meta"'); + expect(reported.join('\n')).toContain(`Operator "${op}" on field "meta" WAS NOT APPLIED`); + expect(reported.join('\n')).toContain('At aggregations[1].filter.'); + }); + } + } + + it('no reportWithheld: the same refusal, the diagnostic dropped', () => { + const err = refusalFrom(() => applyInMemoryAggregation([], ast({ owners: { $in: ['u1'] } }), undefined, FIELDS)); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + }); + + it('no field map: nothing is judged, and the walker answers as it always did', () => { + expect(applyInMemoryAggregation(ROWS.map((row) => ({ ...row })), ast({ owners: { $in: ['u1'] } }))).toEqual([{ n: 6, m: 0 }]); + }); + + it('what still answers on a declared JSON-stored field answers here too', () => { + expect(applyInMemoryAggregation(ROWS.map((row) => ({ ...row })), ast({ owners: { $contains: 'u1' } }), undefined, FIELDS)) + .toEqual([{ n: 6, m: 2 }]); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d6df35483f9..1476684ffbf 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -17151,13 +17151,18 @@ export class ObjectQL implements IObjectQLEngine { // 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. + // [#21007] …and, last, `where`'s JSON-column rule: a scalar + // comparison (`$in`, `$nin`, `$eq`, an ordering, implicit + // equality, …) on a field declared JSON-stored is refused in + // `where`'s words — counted by the fallback, `$nin` counted the + // very rows it was asked to exclude. Same withholding, same log. const typed = normalizeFilterComparandTypes(numeric, `aggregate('${object}')`, `aggregations[${i}].filter`); 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}`, + + `is for the same refusal in a where. Full diagnostic: ${diagnostic}`, ), }); if (typed !== aggFilter) { diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index b77fa6fdf5c..09c7ccc4126 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -120,6 +120,16 @@ // matched: `{ owners: { $contains: 'u1' } }` counted 0 where the same `where` // counted 2, and `$notContains` counted every row, the members included. See // {@link storedArrayHasMember} and {@link declaredJsonStoredFields}. +// +// [#21007] …and on that same declared population, the per-aggregation `filter` +// REFUSES the scalar comparisons `where` refuses there — the equality and +// ordering family, `$between`, `$in` / `$nin` and implicit equality — with +// `where`'s code and words (`INVALID_FILTER` / 400, `@objectstack/core`'s +// `jsonColumnOperatorRefusalText`), judged once on the filter before any driver +// is asked. Before, the walker compared the whole stored array against a scalar: +// `{ 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}. import type { FilterCondition } from '@objectstack/spec/data'; // [#20099] The reference's own declaration, so a malformed `addDays` is refused @@ -169,6 +179,10 @@ import { STRUCTURED_JSON_TYPES, isMultiValueField } from '@objectstack/spec/data // `filter-comparand-shape.ts` rather than re-declared here — see the note on // {@link invalidFilterError} and on {@link unknownOperator} below. import { invalidFilterError } from './filter-comparand-shape.js'; +// [#21007] The operators `where` refuses on a column stored as JSON, and the +// words it refuses them in — one set and one sentence, shared with `driver-sql` +// rather than copied from it. +import { JSON_COLUMN_INCOMPATIBLE_OPERATORS, jsonColumnOperatorRefusalText } from '@objectstack/core'; const LOGICAL_OPERATORS = ['$and', '$or', '$not'] as const; @@ -842,6 +856,13 @@ export function declaredFieldClasses(fields: unknown): Map { const stored = new Set(); @@ -968,6 +989,16 @@ export function assertHavingIsEvaluable( * fixing it does not reveal one they could have been told about already. * {@link assertAggregationFilterReferencesAreDeclared} carries the words. * + * [#21007] …and, last, the column-type rule `where` applies on every SQL + * dialect: a scalar comparison — the equality and ordering family, `$between`, + * `$in` / `$nin`, implicit equality — aimed at a field the object declares + * JSON-stored is refused, in `where`'s words, with the field and the operator + * withheld from the message and handed to the host's log, as `where` does. + * {@link assertAggregationFilterSparesJsonStoredFields} carries it. Judged here, + * on the FILTER, because the per-row walk never meets an empty table or a + * short-circuited `$or` branch: a refusal raised there would be the data's + * (#20122's rule). + * * Read-only. */ export function assertAggregationFilterIsEvaluable( @@ -977,7 +1008,101 @@ export function assertAggregationFilterIsEvaluable( ): void { const clause = aggregationFilterClause(index); assertNodeIsEvaluable(filter, clause.root, { clause }); - if (declared) assertAggregationFilterReferencesAreDeclared(filter, clause.root, declared); + if (declared) { + assertAggregationFilterReferencesAreDeclared(filter, clause.root, declared); + assertAggregationFilterSparesJsonStoredFields(filter, clause.root, declared); + } +} + +/** + * [#21007] Refuse a scalar comparison aimed at a field the object declares + * JSON-stored ({@link declaredJsonStoredFields}: a structured-JSON type, or a + * multi-valued field) — every operator `@objectstack/core`'s + * `JSON_COLUMN_INCOMPATIBLE_OPERATORS` names, and implicit equality, whatever + * the comparand (`null` and an empty list included, as `where` refuses them). + * + * The per-aggregation `filter` gave the three wrong answers `driver-sql`'s + * `where` refused (#7398): the stored array never equals a scalar, so `$in` / + * `$eq` counted nothing, `$nin` / `$ne` counted the rows they were asked to + * exclude, and the orderings compared an array by JS coercion. Measured through + * `POST /data/:object/query` on SQLite and PostgreSQL 16, beside the 400 the same + * `where` answers on both. So one filter got a 400 in `where` and a wrong count + * here; now it gets the 400 in both, in the same words. + * + * The WORDS are `where`'s — `jsonColumnOperatorRefusalText`, the text `driver-sql` + * refuses with — and so is the disclosure: the message names neither the field + * nor the operator, and the full diagnostic (both named, plus this position) goes + * to `declared.reportWithheld`, the host's server log, as + * {@link assertAggregationFilterReferencesAreDeclared} does for the cross-field + * refusal. What answers on such a field is unchanged: `$contains` / + * `$notContains` (membership), `$exists`, `$null` and `$empty`. + * + * The same traversal {@link assertAggregationFilterReferencesAreDeclared} takes, + * after it: the walk has already refused every unknown `$` key, so the arms + * here meet only declared operators. A plain object with no `$` key is not a + * condition this rule reads; the engine's no-operator-object door refuses it + * earlier. No usable field map (a registry-less host) ⇒ nothing is judged. + * + * THIS is the complete door. The per-row arm in {@link checkCondition} is a + * backstop that fires only on a row that reaches it, and many never do: an + * empty table has no row, and the spec's filter lowering (rule 3) rewrites a + * negation into `$or: [{ f: { $null: true } }, { f: { $ne: v } }]` (and gives a + * `$not` operand an `{ f: { $null: false } }` conjunct), so on a row with no + * value the `$null` arm decides and the walker never reaches the comparison. + * Measured with this call removed: a `json` field null in every row answered + * `$ne` / `$nin` / `$not $in` with every row counted. Hence [#21007]'s second + * caller, `applyInMemoryAggregation`, which a host may call with a field map + * and no engine in front of it — it calls this same function, once per + * aggregation, before any row is judged. + * + * `declared` needs only the field map and where the diagnostic goes; the + * object's name is the reference rule's, not this one's. + */ +export function assertAggregationFilterSparesJsonStoredFields( + filter: unknown, + root: string, + declared: Pick, +): void { + const jsonStored = declaredJsonStoredFields(declared.fields); + if (jsonStored.size === 0) return; + const refuse = (field: string, op: string, bare: boolean, path: string): never => { + const { message, diagnostic } = jsonColumnOperatorRefusalText(field, op, bare); + declared.reportWithheld(`At ${path}: ${diagnostic}`); + throw invalidFilterError(message); + }; + 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('$') || !jsonStored.has(key)) continue; + if (isImplicitEquality(value)) refuse(key, '=', true, here); + for (const op of Object.keys(value as Record)) { + if (JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op)) refuse(key, op, false, `${here}.${op}`); + } + } + }; + walk(filter, root); +} + +/** + * [#21007] A column condition that is IMPLICIT equality — a comparand rather + * than an operator map: a primitive, `null`, a `Date` or an array. The same + * split {@link checkCondition} makes before it compares. + */ +function isImplicitEquality(condition: unknown): boolean { + return typeof condition !== 'object' + || condition === null + || condition instanceof Date + || Array.isArray(condition); } /** @@ -1524,6 +1649,19 @@ function storedArrayHasMember(value: unknown, comparand: unknown): boolean { * `$contains` asks whether its comparand is a MEMBER of the stored array * ({@link storedArrayHasMember}) and `$notContains` its exact complement; on * any other column both keep the substring test. + * + * [#21007] …and a scalar comparison on such a column — implicit equality, or an + * operator in `JSON_COLUMN_INCOMPATIBLE_OPERATORS` — is REFUSED, in `where`'s + * withheld words. The COMPLETE door is + * {@link assertAggregationFilterSparesJsonStoredFields}, which judges the filter + * once, before any row is read, and reports the diagnostic; `engine.aggregate` + * and `applyInMemoryAggregation` both call it. This arm is only the BACKSTOP + * for a row that reaches it — a caller evaluating rows directly through + * `matchesAggregationFilter`. It sits above the no-value exit, so a row with no + * value that reaches it is refused too; but its REACH is the row's: an empty + * row set never gets here, and neither does a row on which a short-circuited + * `$or` / `$and` branch has already decided (the spec lowering's rule 3 puts a + * `$null` arm ahead of every negation). */ function checkCondition( value: any, @@ -1540,12 +1678,10 @@ function checkCondition( // to mirror the Filter Protocol's memory evaluation. [#20148] A `Date` bound // is compared as an instant ({@link instantsOf}). [#20176] On a temporal // column, both sides in the column's storage form first. - if ( - typeof condition !== 'object' - || condition === null - || condition instanceof Date - || Array.isArray(condition) - ) { + if (isImplicitEquality(condition)) { + // [#21007] Not on a declared JSON-stored column: a stored array never equals + // a scalar, and `where` refuses the spelling there. + if (jsonStored) throw invalidFilterError(jsonColumnOperatorRefusalText(field, '=', true).message); return comparandEquals(form(value), form(condition)); } @@ -1578,6 +1714,12 @@ function checkCondition( if (op === '$empty' && typeof target !== 'boolean') { throw emptyFlagComparandError(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. + if (jsonStored && JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op)) { + throw invalidFilterError(jsonColumnOperatorRefusalText(field, op, false).message); + } if (value === undefined && !NO_VALUE_ANSWERED_BY_OPERATOR.has(op)) return false; // [#20099] A `{ $field }` reference as the whole comparand of a scalar // comparison is RESOLVED against this row. The arms below would compare the diff --git a/packages/objectql/src/in-memory-aggregation.ts b/packages/objectql/src/in-memory-aggregation.ts index 17e041d6271..c65412761a9 100644 --- a/packages/objectql/src/in-memory-aggregation.ts +++ b/packages/objectql/src/in-memory-aggregation.ts @@ -86,7 +86,13 @@ import { bucketDateKey, compensatedSum } from '@objectstack/core'; import type { QueryAST, GroupByNode, AggregationNode, DateGranularityValue } from '@objectstack/spec/data'; -import { declaredFieldClasses, declaredJsonStoredFields, matchesAggregationFilter } from './having-filter.js'; +import { + aggregationFilterClause, + assertAggregationFilterSparesJsonStoredFields, + declaredFieldClasses, + declaredJsonStoredFields, + matchesAggregationFilter, +} from './having-filter.js'; /** * Group + aggregate raw rows according to the AST's `groupBy` / @@ -106,18 +112,42 @@ import { declaredFieldClasses, declaredJsonStoredFields, matchesAggregationFilte * [#20873] …and `$contains` / `$notContains` on a declared JSON-stored field ask * MEMBERSHIP (having-filter.ts `declaredJsonStoredFields`), the reading the * same condition gets in a `where`. Absent ⇒ the substring reading, as before. + * + * [#21007] …and a scalar comparison on such a field (`$in`, `$nin`, `$eq`, an + * ordering, `$between`, implicit equality) is REFUSED `INVALID_FILTER` / 400, + * in the words `where` refuses it in — judged once per `aggregations[i].filter` + * by having-filter.ts `assertAggregationFilterSparesJsonStoredFields`, the same + * function `engine.aggregate` calls, BEFORE any row is judged. So an empty + * `rows` and a row set the per-row walk would short-circuit past refuse alike; + * the per-row arm is only the backstop. `reportWithheld` is where the withheld + * half of that refusal goes — the field, the operator and the position, which + * the message does not name. This entry point holds no logger of its own, so a + * host that wants the diagnostic passes its log here; absent ⇒ it is dropped, + * and the caller still gets the refusal's code, status and prescription. + * `engine.aggregate` has judged the filter already (and logged it) before it + * calls this, so it passes none. */ export function applyInMemoryAggregation( rows: any[], ast: Pick, timezone?: string, fields?: Record, + reportWithheld?: (diagnostic: string) => void, ): any[] { const groupBy = (ast.groupBy ?? []) as GroupByNode[]; const aggregations = (ast.aggregations ?? []) as AggregationNode[]; if (groupBy.length === 0 && aggregations.length === 0) return rows; // [#20176] Read once per call, and only when some aggregation carries a filter. const anyFilter = aggregations.some((a) => a?.filter && Object.keys(a.filter).length > 0); + // [#21007] The JSON-column rule, judged on each FILTER before any row is — + // the complete door, not the per-row backstop. See the docblock above. + if (fields && anyFilter) { + const declared = { fields, reportWithheld: reportWithheld ?? (() => undefined) }; + for (const [index, agg] of aggregations.entries()) { + if (!agg?.filter || Object.keys(agg.filter).length === 0) continue; + assertAggregationFilterSparesJsonStoredFields(agg.filter, aggregationFilterClause(index).root, declared); + } + } const filterClasses = fields && anyFilter ? declaredFieldClasses(fields) : undefined; // [#20873] Read once per call too, from the same declaration. const filterJsonStored = fields && anyFilter ? declaredJsonStoredFields(fields) : undefined; diff --git a/packages/rest/src/aggregation-filter-json-column-refusal.test.ts b/packages/rest/src/aggregation-filter-json-column-refusal.test.ts new file mode 100644 index 00000000000..648ae70aad3 --- /dev/null +++ b/packages/rest/src/aggregation-filter-json-column-refusal.test.ts @@ -0,0 +1,237 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#21007] A per-aggregation `filter` REFUSES a scalar comparison on a declared + * JSON-stored field the way its `where` twin does — same status, same code, same + * WORDS — through the door a caller uses: `POST /api/v1/data/:object/query` → + * `RestServer` → `ObjectStackProtocolImplementation.findData` → + * `ObjectQL.aggregate` → a real `SqlDriver`. Each row runs beside its live + * `where` twin, and the two response bodies' `error` must be the SAME string: + * the twin's is `driver-sql`'s refusal, the per-aggregation one the engine's, + * and both read `@objectstack/core`'s `jsonColumnOperatorRefusalText`. A change + * to either face that is not a change to the shared text breaks the equality + * here. + * + * Measured before this change (`origin/main` `d1f8ce865`), on SQLite and a live + * PostgreSQL 16.14, on the six rows below: every `where` twin was 400 + * `INVALID_FILTER`, while the per-aggregation `filter` answered 200 — `owners + * $in ['u1','u9']` m = 0, `owners $nin ['u1','u9']` m = 6 (the rows holding + * `u1` counted), `owners $gt 'u1'` m = 4, `tags $eq 'red'` m = 1, `meta $eq 'a'` + * m = 0 — identically on both dialects. + * + * The engine-level cell (the in-memory lowering over the read shape `find()` + * presents) is `@objectstack/objectql`'s + * `engine-aggregate-filter-json-column-refusal.test.ts`. + * + * ## The dialect axis of THIS file + * + * The SQLite cell always runs. The PostgreSQL and MySQL cells run where + * `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL` are set and are a named skip + * otherwise. ⚠️ No CI job provisions those variables for this package, so the + * live cells are red-capable and un-run in CI; the PR that landed this file + * carries their local PostgreSQL run. Each live cell owns its table, dropped + * before and after. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } 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 = 'rest_agg_filter_json_col_21007'; + +const DOC = { + name: OBJECT, + label: 'Doc 21007', + fields: { + title: { name: 'title', type: 'text' as const }, + owners: { name: 'owners', type: 'lookup' as const, reference: 'rest_agg_filter_user_21007', multiple: true }, + tags: { name: 'tags', type: 'tags' as const }, + meta: { name: 'meta', type: 'json' as const }, + }, +}; + +const ROWS = [ + { id: 'd1', title: 'u1 memo', owners: ['u1', 'u2'], tags: ['red', 'blue'] }, + { id: 'd2', title: 'none', owners: ['u2'], tags: ['blue'] }, + { id: 'd3', title: 'about u10', owners: ['u3', 'u1'], tags: ['redwood'] }, + { id: 'd4', title: 'x', owners: [], tags: [] }, + { id: 'd5', title: 'u1', owners: ['u10'], tags: ['red'] }, + { id: 'd6', title: null, owners: null, tags: null }, +]; + +const VALUES: Record = { + owners: ['u1', 'u9'], + tags: ['red', 'green'], + meta: ['a', 'b'], +}; + +/** Every member of the family `where` refuses on a JSON column, as a filter on `field`. */ +function family(field: string): Array]> { + const [a, b] = VALUES[field]; + return [ + ['implicit equality', { [field]: a }], + ['implicit equality with null', { [field]: null }], + ['$eq', { [field]: { $eq: a } }], + ['$eq null', { [field]: { $eq: null } }], + ['$ne', { [field]: { $ne: a } }], + ['$gt', { [field]: { $gt: a } }], + ['$gte', { [field]: { $gte: a } }], + ['$lt', { [field]: { $lt: a } }], + ['$lte', { [field]: { $lte: a } }], + ['$between', { [field]: { $between: [a, b] } }], + ['$in (the card)', { [field]: { $in: [a, b] } }], + ['$nin (the card)', { [field]: { $nin: [a, b] } }], + ['$in []', { [field]: { $in: [] } }], + ['$nin []', { [field]: { $nin: [] } }], + ['$in under $not', { $not: { [field]: { $in: [a] } } }], + ]; +} + +/** Controls on the scalar `title` column: the per-aggregation count equals the where twin's. */ +const CONTROLS: ReadonlyArray]> = [ + ['title $in', { title: { $in: ['u1', 'x'] } }], + ['title $nin', { title: { $nin: ['u1', 'x'] } }], + ['title $eq', { title: { $eq: 'x' } }], + ['title implicit equality', { title: 'x' }], + ['owners $contains — the prescribed spelling', { owners: { $contains: 'u1' } }], + ['owners an $or of $contains — the any-of spelling', { $or: [{ owners: { $contains: 'u1' } }, { owners: { $contains: 'u3' } }] }], +]; + +interface Cell { + id: 'sqlite' | 'pg' | 'mysql'; + label: string; + env: string | null; + config: () => Record | null; +} + +const CELLS: readonly Cell[] = [ + { id: 'sqlite', label: 'sqlite', env: null, config: () => ({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) }, + { + id: 'pg', + label: 'live postgres', + env: 'OS_TEST_POSTGRES_URL', + config: () => (process.env.OS_TEST_POSTGRES_URL ? { client: 'pg', connection: process.env.OS_TEST_POSTGRES_URL } : null), + }, + { + id: 'mysql', + label: 'live mysql', + env: 'OS_TEST_MYSQL_URL', + config: () => (process.env.OS_TEST_MYSQL_URL ? { client: 'mysql2', connection: process.env.OS_TEST_MYSQL_URL } : 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: 'count', alias: 'm', filter }], +}); +const whereTwin = (filter: unknown) => ({ where: filter, aggregations: [{ function: 'count', alias: 'n' }] }); + +for (const cell of CELLS) { + const config = cell.config(); + describe.skipIf(!config)( + `[#21007] POST /data/:object/query — a per-aggregation filter refuses a scalar comparison on a JSON-stored field as its where twin does — ${cell.label}${config ? '' : ` (skipped: set ${cell.env} to run this cell)`}`, + () => { + let engine: ObjectQL; + let driver: any; + let warn: ReturnType; + let post: (body: Record) => Promise<{ status: number; json: any }>; + + const dropTables = async () => { + if (cell.id === 'sqlite') return; + await driver?.execute(`drop table if exists ${OBJECT}`).catch(() => {}); + }; + + const boot = async (populated: boolean) => { + if (engine) await engine.destroy().catch(() => {}); + driver = new SqlDriver(config as any); + await dropTables(); + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(DOC as any); + await engine.syncSchemas(); + if (populated) for (const row of ROWS) await engine.insert(OBJECT, { ...row } as any); + warn = 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 }; + }; + }; + + beforeAll(async () => { await boot(true); }, 60_000); + afterAll(async () => { + await dropTables(); + await engine?.destroy().catch(() => {}); + }, 60_000); + + for (const field of ['owners', 'tags', 'meta']) { + for (const [name, filter] of family(field)) { + it(`${field} ${name}: 400 INVALID_FILTER, the where twin's very body`, async () => { + const twin = await post(whereTwin(filter)); + expect(twin.status, 'where twin').toBe(400); + expect(twin.json.code).toBe('INVALID_FILTER'); + warn.mockClear(); + const agg = await post(perAggregation(filter)); + expect(agg.status, JSON.stringify(agg.json)).toBe(400); + expect(agg.json.code).toBe('INVALID_FILTER'); + // The same words — byte for byte, including the envelope's own cut. + expect(agg.json.error).toBe(twin.json.error); + expect(agg.json.error).toContain('{ "FIELD": { "$contains": "a" } }'); + expect(agg.json.error).toContain('{ "$or": [{ "FIELD": { "$contains": "a" } }'); + expect(agg.json.error).not.toContain(`"${field}"`); + // The field and the operator are in the server log, not the response. + const logged = warn.mock.calls.map((call: unknown[]) => String(call[0])).join('\n'); + expect(logged).toContain('INVALID_FILTER — refusal detail withheld from the response'); + expect(logged).toMatch(new RegExp(`(Operator "\\$[a-z]+" on field "${field}"|The bare equality spelling \\{ "${field}": value \\}) WAS NOT APPLIED`)); + }, 60_000); + } + } + + for (const [name, filter] of CONTROLS) { + it(`control — ${name}: the per-aggregation count is the where twin's`, async () => { + const twin = await post(whereTwin(filter)); + expect(twin.status, JSON.stringify(twin.json)).toBe(200); + const agg = await post(perAggregation(filter)); + expect(agg.status, JSON.stringify(agg.json)).toBe(200); + expect(agg.json.records).toEqual([{ n: 6, m: twin.json.records[0].n }]); + }, 60_000); + } + + it('an empty table refuses the card too — the verdict is the filter\'s', async () => { + await boot(false); + for (const filter of [{ owners: { $in: ['u1', 'u9'] } }, { owners: { $nin: ['u1', 'u9'] } }]) { + const twin = await post(whereTwin(filter)); + const agg = await post(perAggregation(filter)); + expect(twin.status).toBe(400); + expect(agg.status).toBe(400); + expect(agg.json.code).toBe('INVALID_FILTER'); + expect(agg.json.error).toBe(twin.json.error); + } + }, 60_000); + }, + ); +}