From 20ac91e27e66469021581def6562b9c1970ac0a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 13:55:35 +0000 Subject: [PATCH] fix(service-analytics): $or / $not filters reach the query (#4128 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last of the silently-dropped filter family, and the one dropped for a structural reason rather than an oversight: `normalizeAnalyticsFilters` produced a flat ARRAY, which cannot carry a disjunction. Both strategies therefore skipped `$or` and `$not`, so a widget or dataset filtering with either compiled a WHERE clause that simply did not contain it and drew every row — #3650's symptom, and unlike a rejected query it looks like a working chart. The normalizer now produces a TREE (`normalizeAnalyticsFilterTree`), which is the single owner of the vocabulary, and each strategy compiles it the way its own backend expresses a disjunction: - NativeSQLStrategy builds the WHERE recursively, routing every leaf through its existing clause emitter — so the storage-form coercion and the calendar-day upper-bound rule (#3777) apply at every depth, including inside an $or, where a second combinator-aware emitter would have been free to drift from the first. Parentheses are explicit rather than resting on SQL precedence: `a AND b OR c` happens to be right, and being right by construction is what stops a later edit making it wrong. - ObjectQLStrategy hands $or/$not to the engine, which speaks them natively. AND-ed leaves still merge per field exactly as before, so a query without combinators produces byte-identical engine input. - /analytics/sql renders the same tree. That block's own comment warns against echoing something other than what executes; a dropped disjunction was that lie in the other direction. - The cross-object envelope check now sees members nested in an $or. It rejects cross-object filters, so a member it could not see was a filter it could not reject. Empty $and/$or arrays throw rather than being ignored, and the tree walker's combinator handling deliberately mirrors `read-scope-sql.ts` — the compiler in this same package that has always handled the full tree. That the package held one correct implementation and one lossy one for the same filter shape, with nothing holding them to each other, is the actual defect behind this one. `native-sql-filter-logic-conformance.test.ts` runs the SHARED combinator table (FILTER_LOGIC_CASES, #3774) against a real SQLite engine and asserts row ids, so the analytics raw-SQL path now stands beside driver-sql, driver-memory, formula and read-scope-sql under one standard. 14 of its 17 cases fail without this change. service-analytics 446 green; full workspace build clean. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TqqZmPS5a4gJGBoCTwipFr --- .changeset/analytics-or-not-combinators.md | 40 +++ .../filter-operator-coverage.test.ts | 6 +- ...ative-sql-filter-logic-conformance.test.ts | 141 ++++++++ .../src/strategies/filter-normalizer.ts | 319 +++++++++++------- .../src/strategies/native-sql-strategy.ts | 76 ++++- .../src/strategies/objectql-strategy.ts | 137 +++++++- 6 files changed, 573 insertions(+), 146 deletions(-) create mode 100644 .changeset/analytics-or-not-combinators.md create mode 100644 packages/services/service-analytics/src/__tests__/native-sql-filter-logic-conformance.test.ts diff --git a/.changeset/analytics-or-not-combinators.md b/.changeset/analytics-or-not-combinators.md new file mode 100644 index 0000000000..d1a47f2605 --- /dev/null +++ b/.changeset/analytics-or-not-combinators.md @@ -0,0 +1,40 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): a `$or` / `$not` filter no longer vanishes from an analytics query (#4128 follow-up) + +The last of the silently-dropped filter family. `normalizeAnalyticsFilters` +produced a flat **array**, which cannot carry a disjunction, so both strategies +skipped `$or` and `$not` outright — a widget or dataset whose filter used +either compiled a WHERE clause that simply did not contain it, and drew every +row. That is #3650's symptom, and unlike a rejected query it looks like a +working chart. + +The normalizer now produces a **tree** (`normalizeAnalyticsFilterTree`), and +each strategy compiles it the way its own backend expresses a disjunction: + +- **`NativeSQLStrategy`** builds the WHERE recursively, routing every leaf + through its existing clause emitter — so the storage-form coercion and the + calendar-day upper-bound rule (#3777) apply at every depth, including inside + an `$or`. Parentheses are explicit rather than relying on SQL precedence. +- **`ObjectQLStrategy`** hands `$or` / `$not` to the engine, which speaks them + natively. AND-ed leaves still merge per field exactly as before, so a query + without combinators produces byte-identical engine input. +- **`/analytics/sql`** renders the same tree, so the echoed statement keeps + reproducing what executes rather than showing a conjunction where the engine + runs a disjunction. +- The **cross-object envelope check** now sees members nested inside an `$or`. + It rejects cross-object filters, so a member it could not see was a filter it + could not reject. + +Empty `$and` / `$or` arrays now throw instead of being ignored, matching the +fail-closed stance of `read-scope-sql.ts` — the compiler in this same package +that has always handled the full tree, and whose semantics the tree walker now +mirrors deliberately. + +Cover is `native-sql-filter-logic-conformance.test.ts`, which runs the shared +combinator table (`FILTER_LOGIC_CASES`, #3774) against a real SQLite engine and +asserts row ids. The analytics raw-SQL path now stands beside `driver-sql`, +`driver-memory`, `formula` and `read-scope-sql` under that one standard; 14 of +its 17 cases fail without this change. diff --git a/packages/services/service-analytics/src/__tests__/filter-operator-coverage.test.ts b/packages/services/service-analytics/src/__tests__/filter-operator-coverage.test.ts index 94098c7ee9..11cd0c06a8 100644 --- a/packages/services/service-analytics/src/__tests__/filter-operator-coverage.test.ts +++ b/packages/services/service-analytics/src/__tests__/filter-operator-coverage.test.ts @@ -28,7 +28,7 @@ import type { AnalyticsQuery, FilterCondition } from '@objectstack/spec/data'; import type { StrategyContext } from '@objectstack/spec/contracts'; import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; -import { normalizeAnalyticsFilters } from '../strategies/filter-normalizer.js'; +import { normalizeAnalyticsFilterTree } from '../strategies/filter-normalizer.js'; interface Row { id: string; @@ -181,12 +181,12 @@ describe('analytics filters — every authorable operator reaches the query (#41 // rows the filter excludes. A typo'd or non-spec operator is a caller // error, and a loud one — the same call driver-memory made in #3948. expect(() => - normalizeAnalyticsFilters({ where: { name: { $sortOf: 'alpha' } } }), + normalizeAnalyticsFilterTree({ where: { name: { $sortOf: 'alpha' } } }), ).toThrow(/Unsupported filter operator "\$sortOf"/); }); it('a malformed $between throws rather than binding a half-open guess', () => { - expect(() => normalizeAnalyticsFilters({ where: { score: { $between: [10] } } })).toThrow( + expect(() => normalizeAnalyticsFilterTree({ where: { score: { $between: [10] } } })).toThrow( /two-element/, ); }); diff --git a/packages/services/service-analytics/src/__tests__/native-sql-filter-logic-conformance.test.ts b/packages/services/service-analytics/src/__tests__/native-sql-filter-logic-conformance.test.ts new file mode 100644 index 0000000000..a79ca36a4c --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/native-sql-filter-logic-conformance.test.ts @@ -0,0 +1,141 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Filter logical-combinator conformance for the analytics raw-SQL strategy, + * executed against a real SQLite engine (`sql.js`, pure WASM). + * + * The cases come from `@objectstack/spec/data`, so this backend now stands + * beside `driver-sql`, `driver-memory`, `formula`'s `matchesFilterCondition` + * and `read-scope-sql` under one standard — see `filter-logic-conformance.ts` + * for why that standard exists (#3774). + * + * ## Why this consumer arrives late, and what it proves + * + * The analytics strategies could not have passed this table before: their + * normalizer produced a flat ARRAY, which cannot carry a disjunction, so an + * author's `{$or: […]}` was skipped outright and the compiled WHERE simply + * did not contain it. That is not "unsupported" — a missing predicate WIDENS + * the query, returning rows the author excluded, and it is invisible to a + * test that asserts the emitted SQL string (the SQL stays valid, just + * broader). Every case below whose filter carries a combinator fails against + * the pre-tree normalizer, most of them by returning the entire fixture. + * + * The read-scope compiler in this same package (`read-scope-sql.ts`) has + * always compiled the full tree, and is already a consumer of this table — + * so the package contained one correct implementation and one lossy one, for + * the same filter shape, with nothing holding them to each other. It does + * now. + * + * ## Why `sql.js` and not `better-sqlite3` + * + * Same reason as `read-scope-sql-conformance.test.ts`: the native binding is + * loadable only by the exact Node ABI it was built for and aborts the vitest + * worker on CI's Node, taking the file's cases silently with it. `sql.js` is + * the pure-WASM engine `driver-sql` itself falls back to. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { FILTER_LOGIC_CASES, FILTER_LOGIC_ROWS } from '@objectstack/spec/data'; +import type { Cube } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; + +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; + +/** + * Dimension ids match the fixture's column names so the shared cases apply + * unchanged. `id` is selected and grouped by, which makes the result rows the + * matched row ids. + */ +const CUBE: Cube = { + name: 'logic', + title: 'Logic', + sql: 't', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: Object.fromEntries( + ['id', 'a', 'b', 'c', 'owner', 'status', 'parent_object', 'parent_id'].map((n) => [ + n, + { name: n, label: n, type: 'string', sql: n }, + ]), + ), + public: false, +} as unknown as Cube; + +/** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ +async function locateWasm(): Promise<((file: string) => string) | undefined> { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + const dir = dirname(pkgJsonPath); + return (file: string) => join(dir, 'dist', file); + } catch { + return undefined; + } +} + +describe('NativeSQLStrategy — filter logic conformance', () => { + let db: any; + let ctx: StrategyContext; + + beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + + db = new SQL.Database(); + db.run(` + CREATE TABLE "t" ( + "id" TEXT PRIMARY KEY, + "a" TEXT, "b" TEXT, "c" TEXT, + "owner" TEXT, "status" TEXT, + "parent_object" TEXT, "parent_id" TEXT + ); + `); + const insert = db.prepare( + `INSERT INTO "t" ("id","a","b","c","owner","status","parent_object","parent_id") + VALUES (?,?,?,?,?,?,?,?)`, + ); + for (const r of FILTER_LOGIC_ROWS) { + insert.run([r.id, r.a, r.b, r.c, r.owner, r.status, r.parent_object, r.parent_id]); + } + insert.free(); + + ctx = { + getCube: (name: string) => (name === 'logic' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + // The strategy binds `$1`-style placeholders in ascending order, each + // pushed immediately before it is referenced, so a positional rewrite to + // SQLite's `?` preserves the pairing. + executeRawSql: async (_object: string, sql: string, params: unknown[]) => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const out: Record[] = []; + while (stmt.step()) out.push(stmt.getAsObject()); + stmt.free(); + return out; + }, + } as StrategyContext; + }); + + afterAll(() => { + db?.close(); + }); + + for (const c of FILTER_LOGIC_CASES) { + it(c.name, async () => { + const result = await new NativeSQLStrategy().execute( + { + cube: 'logic', + measures: ['total'], + dimensions: ['id'], + where: c.filter, + } as AnalyticsQuery, + ctx, + ); + const got = result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + expect(got, c.note).toEqual(c.expected); + }); + } +}); diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index 12872dd32e..c2c84f3a47 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -31,17 +31,26 @@ * `$contains` `$notContains` `$startsWith` `$endsWith`; * - value-DEPENDENT, so resolved explicitly rather than through the map — * `$null` and `$exists`, whose meaning flips with their boolean; - * - lowered — `$and` (flattened in place), and `$between`, which becomes its - * two bounds so each strategy's existing upper-bound handling applies the - * calendar-day whole-day rule (see the note at the lowering); + * - lowered — `$between`, which becomes its two bounds so each strategy's + * existing upper-bound handling applies the calendar-day whole-day rule + * (see the note at the lowering); + * - structural — `$and` / `$or` / `$not`, carried as tree nodes; * - anything else THROWS. An operator outside the vocabulary is a caller * error, and a loud one beats a silently widened read — the call * driver-memory made for the same shape in #3948. * - * The one remaining gap is declared, not silent: the `$or` / `$not` - * combinators are still skipped, because expressing them needs a recursive - * WHERE builder rather than this flat array. Row-result cover for everything - * above lives in `filter-operator-coverage.test.ts`. + * `$or` / `$not` were the last of that family, and they were dropped for a + * structural reason rather than an oversight: this module produced a flat + * ARRAY, which cannot carry a disjunction. So an author's `{$or: […]}` + * vanished from the WHERE clause and the widget drew every row. The output is + * now a {@link NormalizedFilterNode} tree, and each strategy compiles it the + * way its own backend expresses a disjunction. + * + * Row-result cover: `filter-operator-coverage.test.ts` for the operator + * vocabulary, and `native-sql-filter-logic-conformance.test.ts`, which runs + * the SHARED combinator table (`FILTER_LOGIC_CASES`, #3774) that the SQL + * compiler, the in-memory matcher, `formula` and `read-scope-sql` are already + * held to. */ export interface NormalizedAnalyticsFilter { @@ -92,134 +101,216 @@ function stringifyForCube(v: unknown): string { return String(v); } -function flattenCondition(cond: Record, out: NormalizedAnalyticsFilter[]): void { - for (const [key, raw] of Object.entries(cond)) { - if (raw === undefined) continue; - - if (key === '$and' && Array.isArray(raw)) { - for (const sub of raw) { - if (sub && typeof sub === 'object') { - flattenCondition(sub as Record, out); - } - } - continue; - } - // Logical $or / $not require recursive WHERE building which the - // current strategies don't yet support; ignore so partial queries - // still run. - if (key === '$or' || key === '$not') continue; +/** + * One node of the normalized filter TREE. + * + * A tree rather than the flat array this module used to produce, because a flat + * array cannot express `$or` — and what it did with one was DROP it, which does + * not narrow a query, it widens it to rows the author excluded (#3650's + * symptom). The structure is the minimum that survives that: leaves carry the + * pipeline's `{member, operator, values}` triple unchanged, and the combinators + * are explicit so each strategy can compile them the way its own backend + * expresses them — recursive SQL for the raw-SQL path, a passed-through + * `$or`/`$not` for the engine path. + */ +export type NormalizedFilterNode = + | { kind: 'leaf'; member: string; operator: string; values: string[] } + | { kind: 'and'; children: NormalizedFilterNode[] } + | { kind: 'or'; children: NormalizedFilterNode[] } + | { kind: 'not'; child: NormalizedFilterNode }; - if (raw === null) { - out.push({ member: key, operator: 'notSet', values: [] }); - continue; - } +/** `null` means "no constraint" — an empty object contributes no predicate. */ +function andOf(children: NormalizedFilterNode[]): NormalizedFilterNode | null { + if (children.length === 0) return null; + if (children.length === 1) return children[0]; + return { kind: 'and', children }; +} - if (typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { - const wrapper = raw as Record; - const opKeys = Object.keys(wrapper).filter(k => k.startsWith('$')); - if (opKeys.length > 0) { - for (const opKey of opKeys) { - // `$between [min, max]` LOWERS to its two bounds rather than getting a - // `between` operator of its own. Both strategies already carry the - // calendar-day whole-day rule on their upper bound — NativeSQLStrategy - // compiles a bare-day `lte` half-open (#3777), ObjectQLStrategy hands - // `$lte` to the driver, which does the same — so a range's max - // inherits that rule by construction instead of needing a second - // implementation to keep in step. (The preview evaluator's `$between` - // gap was closed the same way, sharing its `$lte` helper.) - // - // Before this, `$between` was simply absent from the operator map and - // fell to the `continue` below: the predicate VANISHED from the WHERE - // clause, so a dashboard widget carrying a range filter charted the - // entire dataset — #3650's symptom, on the surface #3650 was about. - // The temporal conformance matrix caught it as row results - // (`native-sql-temporal-conformance.test.ts`). - if (opKey === '$between') { - const v = wrapper[opKey]; - if (!Array.isArray(v) || v.length !== 2) { - // Never drop it: an unbounded read is the failure mode this whole - // branch exists to prevent, and it is indistinguishable from a - // legitimately wide query. Same stance driver-memory took for the - // same shape (#3948). - throw new Error( - `[analytics] "$between" on "${key}" needs a two-element [min, max] array, got ` + - `${JSON.stringify(v)}. Dropping the predicate would silently widen the query to every row.`, - ); - } - out.push({ member: key, operator: 'gte', values: [stringifyForCube(v[0])] }); - out.push({ member: key, operator: 'lte', values: [stringifyForCube(v[1])] }); - continue; - } +/** + * Compile one `field: value | { $op: … }` entry into its leaves. + * + * Multiple operators on one field AND together — the rule + * `FILTER_LOGIC_CASES` pins for every other backend, and the one a range + * `{ $gte, $lte }` depends on. + */ +function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { + const out: NormalizedFilterNode[] = []; + const leaf = (operator: string, values: string[]): void => { + out.push({ kind: 'leaf', member: key, operator, values }); + }; - // The two null predicates read their BOOLEAN, not just their key — - // which is why neither can live in MONGO_TO_CUBE_OP. `$null: true` - // asks for IS NULL (`notSet`), `$null: false` for IS NOT NULL - // (`set`); `$exists` is the mirror image. `$null` is the shape the - // console emits for an "is empty" / "is not empty" filter - // (`is_null`/`is_not_null` normalise to it in `filter.zod.ts`), so - // dropping it silently meant such a widget showed every row. - if (opKey === '$null' || opKey === '$exists') { - const isNull = opKey === '$null' ? wrapper[opKey] === true : wrapper[opKey] === false; - out.push({ member: key, operator: isNull ? 'notSet' : 'set', values: [] }); - continue; - } + if (raw === null) { + leaf('notSet', []); + return out; + } - const cubeOp = MONGO_TO_CUBE_OP[opKey]; - if (!cubeOp) { - // NEVER drop: a missing predicate does not narrow the query, it - // WIDENS it — the compiled SQL stays valid and simply returns rows - // the author excluded, which is indistinguishable from a - // legitimately broad query and invisible to any test that asserts - // the emitted SQL. That failure mode is #3650's, and skipping - // unmapped operators is how `$between` reproduced it (#4128). - // driver-memory made the same call for the same reason in #3948. + if (typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { + const wrapper = raw as Record; + const opKeys = Object.keys(wrapper).filter((k) => k.startsWith('$')); + if (opKeys.length > 0) { + for (const opKey of opKeys) { + // `$between [min, max]` LOWERS to its two bounds rather than getting a + // `between` operator of its own. Both strategies already carry the + // calendar-day whole-day rule on their upper bound — NativeSQLStrategy + // compiles a bare-day `lte` half-open (#3777), ObjectQLStrategy hands + // `$lte` to the driver, which does the same — so a range's max + // inherits that rule by construction instead of needing a second + // implementation to keep in step. (The preview evaluator's `$between` + // gap was closed the same way, sharing its `$lte` helper.) + // + // Before this, `$between` was simply absent from the operator map and + // fell to the `continue` below: the predicate VANISHED from the WHERE + // clause, so a dashboard widget carrying a range filter charted the + // entire dataset — #3650's symptom, on the surface #3650 was about. + // The temporal conformance matrix caught it as row results + // (`native-sql-temporal-conformance.test.ts`). + if (opKey === '$between') { + const v = wrapper[opKey]; + if (!Array.isArray(v) || v.length !== 2) { + // Never drop it: an unbounded read is the failure mode this whole + // branch exists to prevent, and it is indistinguishable from a + // legitimately wide query. Same stance driver-memory took for the + // same shape (#3948). throw new Error( - `[analytics] Unsupported filter operator "${opKey}" on "${key}". ` + - `Supported: ${Object.keys(MONGO_TO_CUBE_OP).join(', ')}, $between, $null, $exists ` + - `(and $and; $or/$not are not yet compiled by the analytics strategies). ` + - `Dropping it would silently widen the query to rows the filter excludes.`, + `[analytics] "$between" on "${key}" needs a two-element [min, max] array, got ` + + `${JSON.stringify(v)}. Dropping the predicate would silently widen the query to every row.`, ); } - const v = wrapper[opKey]; - const values = Array.isArray(v) - ? v.map(stringifyForCube) - : [stringifyForCube(v)]; - out.push({ member: key, operator: cubeOp, values }); + leaf('gte', [stringifyForCube(v[0])]); + leaf('lte', [stringifyForCube(v[1])]); + continue; + } + + // The two null predicates read their BOOLEAN, not just their key — + // which is why neither can live in MONGO_TO_CUBE_OP. `$null: true` + // asks for IS NULL (`notSet`), `$null: false` for IS NOT NULL + // (`set`); `$exists` is the mirror image. `$null` is the shape the + // console emits for an "is empty" / "is not empty" filter + // (`is_null`/`is_not_null` normalise to it in `filter.zod.ts`), so + // dropping it silently meant such a widget showed every row. + if (opKey === '$null' || opKey === '$exists') { + const isNull = opKey === '$null' ? wrapper[opKey] === true : wrapper[opKey] === false; + leaf(isNull ? 'notSet' : 'set', []); + continue; } - continue; + + const cubeOp = MONGO_TO_CUBE_OP[opKey]; + if (!cubeOp) { + // NEVER drop: a missing predicate does not narrow the query, it + // WIDENS it — the compiled SQL stays valid and simply returns rows + // the author excluded, which is indistinguishable from a + // legitimately broad query and invisible to any test that asserts + // the emitted SQL. That failure mode is #3650's, and skipping + // unmapped operators is how `$between` reproduced it (#4128). + // driver-memory made the same call for the same reason in #3948. + throw new Error( + `[analytics] Unsupported filter operator "${opKey}" on "${key}". ` + + `Supported: ${Object.keys(MONGO_TO_CUBE_OP).join(', ')}, $between, $null, $exists, ` + + `and the $and/$or/$not combinators. ` + + `Dropping it would silently widen the query to rows the filter excludes.`, + ); + } + const v = wrapper[opKey]; + leaf(cubeOp, Array.isArray(v) ? v.map(stringifyForCube) : [stringifyForCube(v)]); } - // Nested relation (e.g. {profile: {verified: true}}). Flatten with - // dot-prefixed keys so cube field path resolution still works. - for (const [nestedKey, nestedVal] of Object.entries(wrapper)) { - flattenCondition({ [`${key}.${nestedKey}`]: nestedVal }, out); + return out; + } + // Nested relation (e.g. {profile: {verified: true}}). Flatten with + // dot-prefixed keys so cube field path resolution still works. + for (const [nestedKey, nestedVal] of Object.entries(wrapper)) { + out.push(...fieldLeaves(`${key}.${nestedKey}`, nestedVal)); + } + return out; + } + + // Implicit equality / array → in + if (Array.isArray(raw)) leaf('in', raw.map(stringifyForCube)); + else leaf('equals', [stringifyForCube(raw)]); + return out; +} + +/** + * Compile a `FilterCondition` object into a node. `null` = no constraint. + * + * Every entry of one object ANDs with its siblings, at every depth — the rule + * `filter-logic-conformance.ts` exists to hold each backend to (#3774). The + * combinator handling deliberately mirrors `read-scope-sql.ts`'s + * `compileNode`, including its fail-closed empty-array rejection, so the two + * SQL-producing paths in this package cannot drift apart about what a filter + * MEANS. + */ +function buildNode(cond: Record): NormalizedFilterNode | null { + const children: NormalizedFilterNode[] = []; + + for (const [key, raw] of Object.entries(cond)) { + if (raw === undefined) continue; + + if (key === '$and' || key === '$or') { + if (!Array.isArray(raw) || raw.length === 0) { + throw new Error( + `[analytics] "${key}" requires a non-empty array. An empty combinator has no ` + + `defensible reading — dropping it widens the query, and treating it as "match ` + + `nothing" silently empties a chart.`, + ); } + const branches = raw + .map((sub) => (sub && typeof sub === 'object' ? buildNode(sub as Record) : null)) + .filter((n): n is NormalizedFilterNode => n !== null); + if (branches.length === 0) continue; + // `$and` folds into this object's own AND; `$or` becomes a node, since + // OR is exactly the structure a flat list could not carry. + if (key === '$and') children.push(...branches); + else children.push(branches.length === 1 ? branches[0] : { kind: 'or', children: branches }); continue; } - // Implicit equality / array → in - if (Array.isArray(raw)) { - out.push({ member: key, operator: 'in', values: raw.map(stringifyForCube) }); - } else { - out.push({ member: key, operator: 'equals', values: [stringifyForCube(raw)] }); + if (key === '$not') { + const inner = raw && typeof raw === 'object' ? buildNode(raw as Record) : null; + if (inner) children.push({ kind: 'not', child: inner }); + continue; + } + + if (key.startsWith('$')) { + throw new Error( + `[analytics] Unsupported top-level filter operator "${key}". ` + + `Dropping it would silently widen the query to rows the filter excludes.`, + ); } + + children.push(...fieldLeaves(key, raw)); } + + return andOf(children); } /** - * Normalize an analytics query's `where` (FilterCondition) into the - * internal array form used by all strategies. + * Normalize an analytics query's `where` (FilterCondition) into the tree the + * strategies compile. `null` when the query carries no `where`. */ -export function normalizeAnalyticsFilters(query: { where?: unknown } | unknown): NormalizedAnalyticsFilter[] { - if (!query || typeof query !== 'object') return []; - - const out: NormalizedAnalyticsFilter[] = []; +export function normalizeAnalyticsFilterTree( + query: { where?: unknown } | unknown, +): NormalizedFilterNode | null { + if (!query || typeof query !== 'object') return null; const where = (query as { where?: unknown }).where; + if (!where || typeof where !== 'object' || Array.isArray(where)) return null; + return buildNode(where as Record); +} - if (where && typeof where === 'object' && !Array.isArray(where)) { - flattenCondition(where as Record, out); - } - - return out; +/** + * Every leaf in the tree, structure discarded. + * + * For asking WHICH MEMBERS a filter touches — the cross-object envelope check + * is the caller. Never for building a predicate: the leaves of an `$or` read + * as a conjunction here, so compiling from this list would turn `a OR b` into + * `a AND b`. Use {@link normalizeAnalyticsFilterTree} for that. + */ +export function collectFilterLeaves( + node: NormalizedFilterNode | null, +): NormalizedAnalyticsFilter[] { + if (!node) return []; + if (node.kind === 'leaf') return [{ member: node.member, operator: node.operator, values: node.values }]; + if (node.kind === 'not') return collectFilterLeaves(node.child); + return node.children.flatMap(collectFilterLeaves); } /** Recover a finite number from a purely-numeric token, else undefined. */ diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 08656d0216..055bf7ec52 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -3,7 +3,11 @@ import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; import type { Cube } from '@objectstack/spec/data'; import type { AnalyticsStrategy, StrategyContext } from './types.js'; -import { normalizeAnalyticsFilters, coerceFilterValueForSql } from './filter-normalizer.js'; +import { + normalizeAnalyticsFilterTree, + coerceFilterValueForSql, + type NormalizedFilterNode, +} from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; import { nextUtcCalendarDay } from '@objectstack/core'; @@ -134,19 +138,19 @@ export class NativeSQLStrategy implements AnalyticsStrategy { } } - // Build WHERE clause + // Build WHERE clause. The filter is a TREE, so it compiles recursively — + // a flat loop can only ever AND, which is precisely why an author's `$or` + // used to be dropped instead of compiled. const whereClauses: string[] = []; - const normalizedFilters = normalizeAnalyticsFilters(query); - if (normalizedFilters.length > 0) { - for (const filter of normalizedFilters) { - const colExpr = this.resolveFieldSql(cube, filter.member, tableName, joins); - // Resolve the (object, column) this member binds against so the value - // can be coerced to the column's storage form (see buildFilterClause). - const target = this.resolveStorageTarget(cube, filter.member, tableName); - const clause = this.buildFilterClause(colExpr, filter.operator, filter.values, params, ctx, target); - if (clause) whereClauses.push(clause); - } - } + const filterSql = this.compileFilterNode( + normalizeAnalyticsFilterTree(query), + cube, + tableName, + joins, + params, + ctx, + ); + if (filterSql) whereClauses.push(filterSql); // Build time dimension filters if (query.timeDimensions && query.timeDimensions.length > 0) { @@ -503,6 +507,52 @@ export class NativeSQLStrategy implements AnalyticsStrategy { return ctx.coerceTemporalFilterColumn(target.object, target.field, col) || col; } + /** + * Compile a normalized filter node into a boolean SQL expression, recursing + * through the combinators. `null` = no constraint. + * + * Leaves go through {@link buildFilterClause} exactly as they did when this + * was a flat loop, so the storage-form coercion and the calendar-day + * upper-bound rule (#3777) apply at every depth — including inside an `$or`, + * where a second, combinator-aware implementation would have been free to + * drift from the first. + * + * Parenthesisation is explicit rather than left to SQL's precedence: `AND` + * does bind tighter than `OR`, so `a AND b OR c` happens to be right, but + * being right by construction is what keeps a future edit from making it + * wrong. + */ + private compileFilterNode( + node: NormalizedFilterNode | null, + cube: Cube, + parentTable: string, + joins: Map, + params: unknown[], + ctx: StrategyContext, + ): string | null { + if (!node) return null; + + if (node.kind === 'leaf') { + const colExpr = this.resolveFieldSql(cube, node.member, parentTable, joins); + // Resolve the (object, column) this member binds against so the value + // can be coerced to the column's storage form (see buildFilterClause). + const target = this.resolveStorageTarget(cube, node.member, parentTable); + return this.buildFilterClause(colExpr, node.operator, node.values, params, ctx, target); + } + + if (node.kind === 'not') { + const inner = this.compileFilterNode(node.child, cube, parentTable, joins, params, ctx); + return inner ? `NOT (${inner})` : null; + } + + const parts = node.children + .map((child) => this.compileFilterNode(child, cube, parentTable, joins, params, ctx)) + .filter((s): s is string => !!s); + if (parts.length === 0) return null; + if (parts.length === 1) return parts[0]; + return `(${parts.join(node.kind === 'or' ? ' OR ' : ' AND ')})`; + } + private buildFilterClause( rawCol: string, operator: string, diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 5c21747488..7eefa3fbfe 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -3,7 +3,12 @@ import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; import type { Cube } from '@objectstack/spec/data'; import type { AnalyticsStrategy, StrategyContext } from './types.js'; -import { normalizeAnalyticsFilters, coerceFilterValueForObjectQL } from './filter-normalizer.js'; +import { + normalizeAnalyticsFilterTree, + collectFilterLeaves, + coerceFilterValueForObjectQL, + type NormalizedFilterNode, +} from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; import { nextUtcCalendarDay } from '@objectstack/core'; import { @@ -99,11 +104,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // Operands that cannot merge into their field's entry without one silently // replacing the other; ANDed in below so the engine intersects them. const conjuncts: Record[] = []; - for (const f of normalizeAnalyticsFilters(query)) { - const fieldName = this.resolveFieldName(cube, f.member, 'any'); - const extra = this.mergeFilterOperand(filter, fieldName, this.convertFilter(f.operator, f.values)); - if (extra) conjuncts.push(extra); - } + this.applyFilterNode(normalizeAnalyticsFilterTree(query), cube, filter, conjuncts); // #3650 — and the time-dimension WINDOWS, through the SAME merge, so a // `dateRange` and a caller `where` bound on one field compose instead of // clobbering each other. @@ -222,8 +223,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // `/analytics/sql` and `execute()` accept/reject the SAME set). An in-envelope // cross-object dim renders as a LEFT JOIN — its logical shape; `execute()` // serves it via FK-expand. + // EVERY member the filter touches, including ones nested in an `$or` — + // the envelope check rejects cross-object filters, so a member it cannot + // see is a filter it cannot reject. const plan = this.planCrossObject(cube, query, Object.fromEntries( - normalizeAnalyticsFilters(query).map((f) => [this.resolveFieldName(cube, f.member, 'any'), true]), + collectFilterLeaves(normalizeAnalyticsFilterTree(query)) + .map((f) => [this.resolveFieldName(cube, f.member, 'any'), true]), )); const crossByDim = new Map((plan?.crossDims ?? []).map((cd) => [cd.outputName, cd])); const joinClauses: string[] = []; @@ -288,15 +293,16 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // so `/analytics/sql` rejects the same out-of-envelope set `execute()` does.) const whereParts: string[] = []; - for (const f of normalizeAnalyticsFilters(query)) { - const clause = this.buildFilterClauseSql( - this.resolveFieldName(cube, f.member, 'any'), - f.operator, - f.values, - params, - ); - if (clause) whereParts.push(clause); - } + // Recursive, so the echoed statement carries the same disjunctions the + // engine filter does — the echo exists to REPRODUCE execution, and an + // `$or` rendered as a conjunction (or dropped) is exactly the lie this + // block's comment above warns about, in the other direction. + const filterClause = this.renderFilterNodeSql( + normalizeAnalyticsFilterTree(query), + cube, + params, + ); + if (filterClause) whereParts.push(filterClause); // Bounds bind as `$n` placeholders like every other comparand: this string // travels to the browser, and a window can carry tenant-derived dates. // A bare-day upper bound renders half-open (`< day+1`) because that is @@ -727,6 +733,105 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * are handed back for the caller to AND in separately, so the engine * intersects them instead of the strategy picking a winner. */ + /** + * Fold a normalized filter node into the engine filter being built. + * + * AND-ed LEAVES merge per field through {@link mergeFilterOperand}, exactly + * as the flat loop this replaced did — so a query without combinators still + * produces byte-identical engine input. Anything structural (`$or`, `$not`, + * a nested `$and` that cannot merge) becomes its own conjunct, which the + * caller ANDs in. The engine speaks these combinators natively + * (`FilterCondition` declares them and every driver compiles them), so this + * path hands them over rather than lowering them. + */ + private applyFilterNode( + node: NormalizedFilterNode | null, + cube: Cube, + filter: Record, + conjuncts: Record[], + ): void { + if (!node) return; + + if (node.kind === 'leaf') { + const fieldName = this.resolveFieldName(cube, node.member, 'any'); + const extra = this.mergeFilterOperand(filter, fieldName, this.convertFilter(node.operator, node.values)); + if (extra) conjuncts.push(extra); + return; + } + + if (node.kind === 'and') { + for (const child of node.children) this.applyFilterNode(child, cube, filter, conjuncts); + return; + } + + const rendered = this.filterNodeToCondition(node, cube); + if (rendered) conjuncts.push(rendered); + } + + /** A node as a standalone `FilterCondition` the engine can consume. */ + private filterNodeToCondition( + node: NormalizedFilterNode | null, + cube: Cube, + ): Record | null { + if (!node) return null; + + if (node.kind === 'not') { + const inner = this.filterNodeToCondition(node.child, cube); + return inner ? { $not: inner } : null; + } + + if (node.kind === 'or') { + const branches = node.children + .map((child) => this.filterNodeToCondition(child, cube)) + .filter((c): c is Record => !!c); + return branches.length > 0 ? { $or: branches } : null; + } + + // `leaf` and `and` share the merge path so one field carrying several + // operators composes here the same way it does at the top level. + const filter: Record = {}; + const conjuncts: Record[] = []; + this.applyFilterNode(node, cube, filter, conjuncts); + if (conjuncts.length > 0) { + filter.$and = [...(Array.isArray(filter.$and) ? filter.$and : []), ...conjuncts]; + } + return Object.keys(filter).length > 0 ? filter : null; + } + + /** + * Render a normalized filter node as the display SQL `/analytics/sql` + * echoes. Values still bind as `$n` placeholders — the echo travels to the + * browser, so a comparand is never inlined. + */ + private renderFilterNodeSql( + node: NormalizedFilterNode | null, + cube: Cube, + params: unknown[], + ): string | null { + if (!node) return null; + + if (node.kind === 'leaf') { + return this.buildFilterClauseSql( + this.resolveFieldName(cube, node.member, 'any'), + node.operator, + node.values, + params, + ); + } + + if (node.kind === 'not') { + const inner = this.renderFilterNodeSql(node.child, cube, params); + return inner ? `NOT (${inner})` : null; + } + + const parts = node.children + .map((child) => this.renderFilterNodeSql(child, cube, params)) + .filter((s): s is string => !!s); + if (parts.length === 0) return null; + if (parts.length === 1) return parts[0]; + return `(${parts.join(node.kind === 'or' ? ' OR ' : ' AND ')})`; + } + private mergeFilterOperand( filter: Record, field: string,