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,