diff --git a/.changeset/analytics-filter-not-null-safe-and-boolean-const.md b/.changeset/analytics-filter-not-null-safe-and-boolean-const.md new file mode 100644 index 0000000000..10ffc575a0 --- /dev/null +++ b/.changeset/analytics-filter-not-null-safe-and-boolean-const.md @@ -0,0 +1,56 @@ +--- +"@objectstack/service-analytics": minor +--- + +fix(service-analytics)!: 分析查询的 `where` —— `$not` 变 NULL-safe、`{$not:{}}` 变零行、`$or` 的 `{}` 析取项不再被丢 (#5325) + +`filter-normalizer.ts` 的 `buildNode` 是这个包里**第二份**同缺陷拷贝:第一份 +(`read-scope-sql.ts` 的 `compileNode`,RLS 读作用域)已由 #5297 修好,而这一份编译的是 +**作者自己写的 `where`** —— dashboard widget / dataset 的筛选器。两者是各自独立的函数, +所以那一单合入后这三条仍然在。以 `driver-sql` 同一份 fixture 实测(4 行,行 3、4 的 +`stage` 为 NULL,行 3 的 `amount` 为 NULL,行 4 的 `owner` 为 NULL): + +| widget 的 `where` | 改前取到的行 | 改后(= driver-memory / formula / #5296 后的 driver-sql) | +|---|---|---| +| `{ $not: { stage: 'won' } }` | `2` | `2,3,4` | +| `{ $not: { stage: { $in: ['won'] } } }` | `2` | `2,3,4` | +| `{ $not: {} }` | **全表** | **零行** | +| `{ $or: [{ stage: 'won' }, {}] }` | `1` | 全表 | +| `{ $not: { $or: [{stage:'won'},{owner:'u1'}] } }` | `2` | `2,4` | + +**这是可观察的行为变更,不是内部重构 —— 已有的图表数值会变:** + +- **`{$not: {}}` 的 widget 此前画的是整个数据集,现在是零行。** `buildNode({})` 返回 + `null`(= 无约束 = TRUE),`$not` 分支的 `if (inner)` 因此为假,整条 `$not` 消失, + WHERE 一个字都不发 —— 一条意思是「什么都不显示」的筛选器显示了全部。`NOT TRUE ≡ FALSE`, + 现在它编译成 `1 = 0`。 +- **`$not` 下 NULL 行的去留变了,所以图上的数字会变。** SQL 是三值逻辑而 `WHERE` 只保留 + TRUE,裸 `NOT (stage = ?)` 把 `stage` 为 NULL 的行全部丢掉;`driver-memory`、`formula` + 和(#5296 之后的)`driver-sql` 都把它们算进来。同一条 widget filter,在分析查询和普通 + `find()` 上给出不同的行集,取决于哪个后端接住它。#5146 已拍板 JS 家族的答案为准,本次 + 按同一口径把守卫**下推到叶子**(`{col: {$null: false}}` / `{$or: [{col:{$null:true}}, …]}`, + 极性逐算子决定)。**受影响的图表数值会上升**(负向筛选现在包含空值行)。 +- **`$or` 里的 `{}` 析取项不再被丢。** TRUE 是 AND 的单位元但**吸收** OR,所以 + `{$or: [{stage:'won'}, {}]}` 整条为 TRUE;此前它被 `.filter(n => n !== null)` 丢掉, + 查询被静默**收紧**成剩余分支。 +- **空集合是布尔常量,不再是「没有谓词」。** `{stage: {$in: []}}` 此前编译成空子句 + → 无约束 → 画全表,现在是零行(`1 = 0`);`{$nin: []}` 不排除任何行。 +- **两处新的响亮拒收(此前静默放宽):** `$not` / `$or` / `$and` 的**非对象**操作数 + (`{$not: null}` 曾整条消失 → 等于不筛),以及**零个操作符的字段约束** `{a: {}}` + —— 后者按 #5240 的拍板拒收,与 driver-sql / driver-memory / formula 一致;不这么做的话, + 「TRUE 吸收 OR」会把 `{$or: [{a: {}}, {b: 2}]}` 从 `b = 2` 放宽成全表。 + +实现落在 normalizer 而不是某个 strategy:守卫在这一层是**结构**(多一个 `$null` 合取项), +经 `filterNodeToCondition` 交给 ObjectQL 引擎后在**任何驱动上都成立**,包括本身不 NULL-safe +的那些;只加在 raw-SQL 那条路径,等于说「分析查询的 `$not` 是什么意思取决于哪个驱动接住它」。 +代价是引擎路径会**双重加守卫**,已实测幂等(`NOT (c IS NOT NULL AND (c IS NOT NULL AND c = v))` +与单层等价),只是 SQL 多一层冗余谓词。 + +`NormalizedFilterNode` 因此新增布尔常量 kind —— 该联合此前只有 `leaf | and | or | not`, +没有 FALSE 的表示法,这正是 `{$not:{}}` 只能编译成「什么都不发」的根本原因。三个编译器 +(`native-sql-strategy.compileFilterNode`、`objectql-strategy.filterNodeToCondition`、 +回显给浏览器的 `renderFilterNodeSql`)各自实现它;引擎路径用的是 `{$not: {}}`,即 +driver-sql / formula / driver-memory 参考匹配器早已钉住的零行写法(#5134),没有另造第二种。 + +`$and: []` / `$or: []` 的空组合子**不在本次范围**,仍然 fail-closed 抛错(独立裁定见 #5322), +并已加用例钉在抛错这一侧。 diff --git a/packages/services/service-analytics/src/__tests__/filter-normalizer-not-null-safe.test.ts b/packages/services/service-analytics/src/__tests__/filter-normalizer-not-null-safe.test.ts new file mode 100644 index 0000000000..0194b4403d --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/filter-normalizer-not-null-safe.test.ts @@ -0,0 +1,588 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5325] The analytics filter NORMALIZER answers `$not` and the boolean + * identities the way the rest of the repo answers them. + * + * # Why a second file in this package + * + * `read-scope-not-null-safe.test.ts` (#5297) pins the same three squares for + * `read-scope-sql.ts` — the RLS lowering. This file pins them for the OTHER + * SQL-producing path in the package: the author's own `where`, normalized by + * `filter-normalizer.ts` and compiled by BOTH strategies. The two are separate + * functions; fixing one left the other carrying every defect, because + * `buildNode` was written from `compileNode` and inherited them (that is why + * `filter-normalizer.ts`'s own header says the two must not drift). + * + * `native-sql-filter-logic-conformance.test.ts` already runs the shared + * `FILTER_LOGIC_CASES` table against a real SQLite engine and was green through + * all three defects: that table deliberately carries no NULL rows and no + * boolean-identity case (#5239 / the spec half of #5146 land those). These cases + * are the pin until the shared table absorbs them. + * + * # The three defects, as measured on the fixture below + * + * | `where` | was | now (= JS family) | + * |-----------------------------------------------|----------|-------------------| + * | `{$not: {stage: 'won'}}` | `2` | `2,3,4` | + * | `{$not: {stage: {$in: ['won']}}}` | `2` | `2,3,4` | + * | `{$not: {}}` | ALL | zero rows | + * | `{$or: [{stage: 'won'}, {}]}` | `1` | ALL | + * | `{$not: {$or: [{stage:'won'},{owner:'u1'}]}}` | `2` | `2,4` | + * + * - `$not` emitted a bare `NOT (…)`. SQL is three-valued and a `WHERE` keeps + * only TRUE, so every row whose column is NULL fell into UNKNOWN and + * vanished — while `driver-memory`, `formula` and (since #5296) `driver-sql` + * return those rows. One widget filter, two row sets, chosen by which + * backend answered. + * - `buildNode({})` returns `null` (= no constraint = TRUE) and the `$not` + * branch tested `if (inner)`, so `{$not: {}}` — the ZERO-row filter — + * produced no node, no `WHERE`, and a chart drawn over the entire dataset. + * - a `{}` disjunct was dropped by `.filter(n => n !== null)`, collapsing + * `$or` to its surviving branches. TRUE ABSORBS a disjunction; dropping it + * silently NARROWED the query instead. + * + * # Where the expected ids come from + * + * Measured, not reasoned: the fixture is row-for-row `driver-sql`'s + * `sql-driver-not-null-safe.test.ts`, and every id set below is the answer that + * file, `formula/src/matches-filter-not-null-safe.test.ts`, + * `driver-memory/src/memory-matcher-not-null-safe.test.ts` and this package's + * own `read-scope-not-null-safe.test.ts` assert for the same filter. Moving an + * expectation here re-opens the divergence #5146 closed. + * + * `sql.js` (pure WASM) is the engine, for the reason spelled out at the top of + * `native-sql-filter-logic-conformance.test.ts`: a 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. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; + +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; + +/** + * Rows 3 and 4 are the point: `stage` is NULL in both, row 3 additionally + * carries a NULL `amount` and row 4 a NULL `owner`, so a guard applied to the + * wrong column shows up as a wrong id rather than passing by luck. + */ +const FIXTURE = [ + { id: '1', stage: 'won', owner: 'u1', amount: 10 }, + { id: '2', stage: 'lost', owner: 'u2', amount: 20 }, + { id: '3', stage: null, owner: 'u1', amount: null }, + { id: '4', stage: null, owner: null, amount: 40 }, +]; + +const ALL = ['1', '2', '3', '4']; + +const CUBE: Cube = { + name: 'deals', + title: 'Deals', + sql: 'deal', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: Object.fromEntries( + ['id', 'stage', 'owner', 'amount'].map((n) => [ + n, + { name: n, label: n, type: n === 'amount' ? 'number' : '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('[#5325] analytics `where` — NULL-safe `$not` and the boolean identities', () => { + let db: any; + let nativeCtx: StrategyContext; + /** The engine filter the ObjectQL path last handed to `executeAggregate`. */ + let lastEngineFilter: Record | undefined; + let objectqlCtx: StrategyContext; + + /** Run a statement in the strategy's `$n` dialect and return the id column. */ + const run = (sql: string, params: unknown[]): string[] => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const out: string[] = []; + while (stmt.step()) out.push(String(stmt.getAsObject().id)); + stmt.free(); + return out.sort((x, y) => x.localeCompare(y)); + }; + + 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 "deal" ("id" TEXT PRIMARY KEY, "stage" TEXT, "owner" TEXT, "amount" REAL);`); + const insert = db.prepare(`INSERT INTO "deal" ("id","stage","owner","amount") VALUES (?,?,?,?)`); + for (const r of FIXTURE) insert.run([r.id, r.stage, r.owner, r.amount]); + insert.free(); + + nativeCtx = { + getCube: (name: string) => (name === 'deals' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + 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; + + // The ObjectQL path hands a `FilterCondition` to the engine rather than SQL. + // `compileScopedFilterToSql` — this package's OTHER FilterCondition consumer, + // already NULL-safe since #5297 — stands in for the engine, so the condition + // is EXECUTED rather than merely inspected, and the double guard the module + // header predicts is exercised for real. + objectqlCtx = { + getCube: (name: string) => (name === 'deals' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async ( + _object: string, + options: { groupBy?: string[]; filter?: Record }, + ) => { + lastEngineFilter = options.filter; + const { sql, params } = compileScopedFilterToSql( + (options.filter ?? {}) as FilterCondition, + 'deal', + ); + const stmt = db.prepare( + `SELECT "id" FROM "deal" AS "deal" WHERE ${sql.length > 0 ? sql : '1 = 1'}`, + ); + stmt.bind(params as any[]); + const out: Record[] = []; + while (stmt.step()) out.push({ ...stmt.getAsObject(), total: 1 }); + stmt.free(); + return out; + }, + } as StrategyContext; + }); + + afterAll(() => { + db?.close(); + }); + + const query = (where: unknown): AnalyticsQuery => ({ + cube: 'deals', + measures: ['total'], + dimensions: ['id'], + timezone: 'UTC', + where, + } as AnalyticsQuery); + + /** + * The rows this `where` admits on the raw-SQL path, executed end to end + * through `NativeSQLStrategy` — the seam, not the compiler in isolation. A + * filter that compiles to nothing turns into a query with no `WHERE` HERE, + * one call above the compiler, which is exactly how `{$not: {}}` used to + * escape (#5297's lesson). + */ + const ids = async (where: unknown): Promise => { + const result = await new NativeSQLStrategy().execute(query(where), nativeCtx); + return result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + }; + + const sqlFor = async (where: unknown): Promise<{ sql: string; params: unknown[] }> => + new NativeSQLStrategy().generateSql(query(where), nativeCtx); + + /** The same `where` through the ObjectQL path: normalizer → engine filter. */ + const engineIds = async (where: unknown): Promise => { + const result = await new ObjectQLStrategy().execute(query(where), objectqlCtx); + return result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + }; + + // ── The issue's measured table, one case per row ─────────────────────────── + + describe('the five measured cases', () => { + it('`{$not: {stage: "won"}}` returns the NULL-stage rows — was `2`', async () => { + expect(await ids({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + }); + + it('`{$not: {stage: {$in: ["won"]}}}` returns them too — was `2`', async () => { + expect(await ids({ $not: { stage: { $in: ['won'] } } })).toEqual(['2', '3', '4']); + }); + + it('`{$not: {}}` matches NO row — was the whole dataset', async () => { + expect(await ids({ $not: {} })).toEqual([]); + }); + + it('`{$or: [{stage: "won"}, {}]}` matches EVERY row — was `1`', async () => { + expect(await ids({ $or: [{ stage: 'won' }, {}] })).toEqual(ALL); + }); + + it('`{$not: {$or: […]}}` excludes a NULL row whose OTHER branch matches — was `2`', async () => { + // Row 3 has a NULL stage but owner = 'u1', so the inner `$or` IS satisfied + // and the negation must reject it. A guard hoisted above the `$not` instead + // of pushed to the leaves would hand row 3 back — the reason the rewrite is + // per leaf. + expect(await ids({ $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '4']); + }); + }); + + // ── Defect: `{$not: {}}` emitted no WHERE at all ────────────────────────── + + describe('`{$not: {}}` is FALSE, and it reaches the generated SQL', () => { + it('compiles to the constant-false clause inside a real WHERE', async () => { + // Was: no `$not` node at all → `whereClauses` empty → `SELECT … FROM "deal" + // GROUP BY …` with no `WHERE`, i.e. every row under a filter meaning "none". + const { sql, params } = await sqlFor({ $not: {} }); + expect(sql).toContain('WHERE'); + expect(sql).toContain('1 = 0'); + expect(params).toEqual([]); + }); + + it('stays FALSE next to a real predicate, in either key order', async () => { + expect(await ids({ $and: [{ owner: 'u1' }, { $not: {} }] })).toEqual([]); + expect(await ids({ owner: 'u1', $not: {} })).toEqual([]); + expect(await ids({ $not: {}, owner: 'u1' })).toEqual([]); + }); + + it('a `$not` of a FALSE filter is TRUE again', async () => { + expect(await ids({ $not: { $not: {} } })).toEqual(ALL); + // Folded to the constant rather than left as a `NOT (1 = 0)` that only + // happens to evaluate right. + const { sql } = await sqlFor({ $not: { $not: {} } }); + expect(sql).toContain('1 = 1'); + expect(sql).not.toContain('NOT ('); + }); + + it('a FALSE disjunct does not absorb an `$or` — it is the OR identity', async () => { + expect(await ids({ $or: [{ stage: 'won' }, { $not: {} }] })).toEqual(['1']); + }); + }); + + // ── Defect: the dropped `{}` disjunct ───────────────────────────────────── + + describe('a `{}` disjunct makes the whole `$or` TRUE', () => { + it('binds NOTHING — the discarded branch takes its comparand with it', async () => { + // Worse than a wide query: a value left in `params` with no `$n` to consume + // it shifts every later placeholder onto the wrong value, so a filter binds + // a comparand the author never wrote (#5297). + const { sql, params } = await sqlFor({ $or: [{ stage: 'won' }, {}] }); + expect(params).toEqual([]); + expect(sql).not.toContain('WHERE'); + }); + + it('keeps the rest of the filter aligned when a later predicate follows', async () => { + // The alignment case in full: an absorbed `$or` in front of a surviving + // predicate. `owner = 'u1'` must bind `$1`, not `$2`. + const { sql, params } = await sqlFor({ $and: [{ $or: [{ stage: 'won' }, {}] }, { owner: 'u1' }] }); + expect(params).toEqual(['u1']); + expect(sql).toContain('$1'); + expect(sql).not.toContain('$2'); + expect(await ids({ $and: [{ $or: [{ stage: 'won' }, {}] }, { owner: 'u1' }] })).toEqual(['1', '3']); + }); + + it('holds at depth and in either order', async () => { + expect(await ids({ $or: [{}, { stage: 'won' }] })).toEqual(ALL); + expect(await ids({ $or: [{}, {}] })).toEqual(ALL); + expect(await ids({ $or: [{ $or: [{ stage: 'won' }, {}] }, { owner: 'u2' }] })).toEqual(ALL); + }); + + it('a `{}` member of a `$and` is still just the identity', async () => { + expect(await ids({ $and: [{}, { stage: 'won' }] })).toEqual(['1']); + const { params } = await sqlFor({ $and: [{}, { stage: 'won' }] }); + expect(params).toEqual(['won']); + }); + }); + + // ── Defect: NULL-safe negation, per operator ────────────────────────────── + + describe('a NULL column does not satisfy the negated condition', () => { + it('the guard rides the LEAF, so the emitted SQL negates a TOTAL predicate', async () => { + const { sql } = await sqlFor({ $not: { stage: 'won' } }); + expect(sql).toBe( + 'SELECT id AS "id", COUNT(*) AS "total" FROM "deal" ' + + 'WHERE NOT ((stage IS NOT NULL AND stage = $1)) GROUP BY id', + ); + }); + + it('`$not` over MULTIPLE columns admits a row that is NULL in EITHER', async () => { + expect(await ids({ $not: { stage: 'won', owner: 'u1' } })).toEqual(['2', '3', '4']); + }); + + it('`$not` of a `$and` admits every row failing either conjunct', async () => { + expect(await ids({ $not: { $and: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '3', '4']); + }); + + it('a double negation is the positive filter again, NULL rows excluded', async () => { + expect(await ids({ $not: { $not: { stage: 'won' } } })).toEqual(['1']); + expect(await ids({ $not: { $not: { stage: 'won' } } })).toEqual(await ids({ stage: 'won' })); + }); + + it('`$not` still ANDs with its sibling keys', async () => { + expect(await ids({ $not: { stage: 'won' }, owner: 'u1' })).toEqual(['3']); + }); + + it('a `$not` nested inside a `$or` branch stays NULL-safe', async () => { + expect(await ids({ $or: [{ $not: { stage: 'won' } }, { owner: 'u2' }] })).toEqual(['2', '3', '4']); + }); + + it('`$not` of `$ne` / `$nin` is NOT widened — polarity is per operator', async () => { + // A blanket `OR stage IS NULL` would hand back rows 3 and 4, i.e. rows the + // filter excludes. `{$not: {$ne: 'won'}}` means "stage IS won". + expect(await ids({ $not: { stage: { $ne: 'won' } } })).toEqual(['1']); + expect(await ids({ $not: { stage: { $nin: ['won'] } } })).toEqual(['1']); + const { sql } = await sqlFor({ $not: { stage: { $ne: 'won' } } }); + expect(sql).toContain('NOT ((stage IS NULL OR stage != $1))'); + }); + + it('`$not` of an ordering comparison returns the NULL rows', async () => { + expect(await ids({ $not: { amount: { $gt: 15 } } })).toEqual(['1', '3']); + }); + + it('`$not` of `$between` returns the NULL rows', async () => { + // `$between` lowers to `gte` + `lte` here; both are positive comparisons, + // so the pair takes the same guard a single comparison does. + expect(await ids({ $not: { amount: { $between: [15, 30] } } })).toEqual(['1', '3', '4']); + }); + + it('`$not` of `$contains` / `$startsWith` / `$endsWith` returns the NULL rows', async () => { + expect(await ids({ $not: { stage: { $contains: 'w' } } })).toEqual(['2', '3', '4']); + expect(await ids({ $not: { stage: { $startsWith: 'w' } } })).toEqual(['2', '3', '4']); + expect(await ids({ $not: { stage: { $endsWith: 'n' } } })).toEqual(['2', '3', '4']); + }); + + it('`$not` of `$notContains` does NOT return them — the mirror case', async () => { + // The one operator where the two JS backends disagree for a null-valued + // field; `formula` is followed, as `driver-sql` and `read-scope-sql` follow + // it, so this module casts no vote on a disagreement filed elsewhere. + expect(await ids({ $not: { stage: { $notContains: 'w' } } })).toEqual(['1']); + }); + + it('`$not` of a null predicate is untouched — it was already two-valued', async () => { + expect(await ids({ $not: { stage: { $null: true } } })).toEqual(['1', '2']); + expect(await ids({ $not: { stage: { $null: false } } })).toEqual(['3', '4']); + expect(await ids({ $not: { stage: { $exists: true } } })).toEqual(['3', '4']); + expect(await ids({ $not: { stage: { $exists: false } } })).toEqual(['1', '2']); + // No guard is wrapped around a predicate that can never be UNKNOWN. + const { sql } = await sqlFor({ $not: { stage: { $null: true } } }); + expect(sql).toContain('WHERE NOT (stage IS NULL)'); + }); + + it('`{field: null}` under a `$not` is untouched too', async () => { + expect(await ids({ $not: { stage: null } })).toEqual(['1', '2']); + }); + + it('an empty `$in` / `$nin` under a `$not` keeps its constant value', async () => { + // Both are boolean CONSTANTS, so they are total and take no guard — and a + // constant must survive the negation rather than be dropped from it. + expect(await ids({ $not: { stage: { $in: [] } } })).toEqual(ALL); + expect(await ids({ $not: { stage: { $nin: [] } } })).toEqual([]); + }); + + it('a guarded relation traversal guards the DOTTED member, not its alias', async () => { + // `{account: {region: 'NA'}}` flattens to the member `account.region` (the + // normalizer's own dotted-key flattening), so the guard has to flatten the + // same way: guarding `account` would test the relation, not the column the + // leaf reads. Asserted on the generated SQL only — `region` is not a column + // of this fixture, which is the point: both halves resolve to ONE member. + const { sql } = await sqlFor({ $not: { account: { region: 'NA' } } }); + expect(sql).toContain('NOT (("account"."region" IS NOT NULL AND "account"."region" = $1))'); + expect(sql).not.toContain('"deal"."account" IS NOT NULL'); + }); + }); + + // ── An empty set is a constant, not an absent predicate ─────────────────── + + describe('an empty `$in` / bare `[]` is the FALSE constant, not a dropped clause', () => { + it('`{stage: {$in: []}}` matches no row — was the whole dataset', async () => { + expect(await ids({ stage: { $in: [] } })).toEqual([]); + const { sql } = await sqlFor({ stage: { $in: [] } }); + expect(sql).toContain('1 = 0'); + }); + + it('`{stage: {$nin: []}}` excludes nothing', async () => { + expect(await ids({ stage: { $nin: [] } })).toEqual(ALL); + }); + + it('a bare `[]` is the same constant its explicit spelling is', async () => { + expect(await ids({ stage: [] })).toEqual([]); + }); + + it('it composes: FALSE absorbs an `$and`, is the identity of an `$or`', async () => { + expect(await ids({ $and: [{ stage: { $in: [] } }, { owner: 'u1' }] })).toEqual([]); + expect(await ids({ $or: [{ stage: { $in: [] } }, { owner: 'u1' }] })).toEqual(['1', '3']); + }); + }); + + // ── The ObjectQL path: the guard survives as STRUCTURE ──────────────────── + + describe('the ObjectQL path gets the same rows, through a FilterCondition', () => { + it('hands the engine a guard that is STRUCTURE, not SQL', async () => { + expect(await engineIds({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + // The guard travels as one more conjunct inside the `$not`, so any driver + // behind the engine — NULL-safe or not — admits the same rows. Rendering it + // only in the SQL strategy would have made the answer depend on the driver. + expect(JSON.stringify(lastEngineFilter)).toContain('$ne'); + expect(JSON.stringify(lastEngineFilter)).toContain('$not'); + }); + + it('DOUBLE-guarding is idempotent — the stand-in engine guards again', async () => { + // `compileScopedFilterToSql` runs its OWN `nullSafeNegationOperand` over + // the condition this path already guarded, so the executed SQL carries the + // guard twice. `NOT (c IS NOT NULL AND (c IS NOT NULL AND c = v))` is the + // same predicate as the single-guarded form: redundant, not wrong. That is + // the trade the module header names — one extra conjunct for portability. + const { sql } = compileScopedFilterToSql( + lastEngineFilter as FilterCondition, + 'deal', + ); + expect(sql.match(/IS NOT NULL/g)?.length).toBeGreaterThanOrEqual(2); + expect(run(`SELECT "id" FROM "deal" AS "deal" WHERE ${sql}`, ['won'])).toEqual(['2', '3', '4']); + }); + + it('`{$not: {}}` reaches the engine as the zero-row filter', async () => { + expect(await engineIds({ $not: {} })).toEqual([]); + // `{$not: {}}` is the spelling `driver-sql`, `formula` and driver-memory's + // matcher already pin as FALSE (#5134); this strategy invents no second one. + expect(lastEngineFilter).toEqual({ $and: [{ $not: {} }] }); + }); + + it('a `{}` disjunct absorbs the `$or` on this path too', async () => { + expect(await engineIds({ $or: [{ stage: 'won' }, {}] })).toEqual(ALL); + // `withReadScope` maps an EMPTY filter to `undefined` — no constraint at + // all, which is what an absorbed `$or` means. Was `{stage: 'won'}`. + expect(lastEngineFilter).toBeUndefined(); + }); + + it('the remaining measured cases agree row for row with the SQL path', async () => { + for (const where of [ + { $not: { stage: { $in: ['won'] } } }, + { $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } }, + { $not: { stage: { $ne: 'won' } } }, + { $not: { amount: { $gt: 15 } } }, + { $not: { stage: { $null: true } } }, + { stage: { $in: [] } }, + { $or: [{ stage: { $in: [] } }, { owner: 'u1' }] }, + ]) { + expect(await engineIds(where), JSON.stringify(where)).toEqual(await ids(where)); + } + }); + }); + + // ── The THIRD compiler: the display SQL `/analytics/sql` echoes ─────────── + + describe('the echoed display SQL renders the identities too', () => { + const echo = async (where: unknown): Promise<{ sql: string; params: unknown[] }> => + new ObjectQLStrategy().generateSql(query(where), objectqlCtx); + + it('`{$not: {}}` echoes a constant-false WHERE, not an unfiltered SELECT', async () => { + // The echo exists to REPRODUCE execution. A statement with no `WHERE` + // handed to whoever is debugging "why is this chart empty" returns the + // whole table and cannot reproduce the result — the lie this rendering + // block's own comment warns about, in the other direction. + const { sql, params } = await echo({ $not: {} }); + expect(sql).toContain('WHERE'); + expect(sql).toContain('1 = 0'); + expect(params).toEqual([]); + }); + + it('an absorbed `$or` echoes no WHERE and binds nothing', async () => { + const { sql, params } = await echo({ $or: [{ stage: 'won' }, {}] }); + expect(sql).not.toContain('WHERE'); + expect(params).toEqual([]); + }); + + it('an absorbed `$or` does not shift a later placeholder', async () => { + const { sql, params } = await echo({ $and: [{ $or: [{ stage: 'won' }, {}] }, { owner: 'u1' }] }); + expect(params).toEqual(['u1']); + expect(sql).toContain('$1'); + expect(sql).not.toContain('$2'); + }); + + it('a NULL-safe `$not` echoes the guard it executes', async () => { + const { sql } = await echo({ $not: { stage: 'won' } }); + expect(sql).toContain('NOT ('); + expect(sql).toContain('stage IS NOT NULL'); + }); + + it('an empty `$in` echoes the constant', async () => { + const { sql } = await echo({ stage: { $in: [] } }); + expect(sql).toContain('1 = 0'); + }); + }); + + // ── Nothing that failed closed stopped failing closed ───────────────────── + + describe('the fail-closed guarantees survive the rewrite', () => { + it('an empty `$and` / `$or` still THROWS — #5322 is its own ruling', async () => { + // The empty-combinator square is decided separately (#5322). This change + // must not quietly turn either of them into a boolean identity on the way + // past, so both stay pinned on the THROWING side, inside a `$not` as well + // as outside. + await expect(ids({ $and: [] })).rejects.toThrowError(/non-empty array/); + await expect(ids({ $or: [] })).rejects.toThrowError(/non-empty array/); + await expect(ids({ $not: { $and: [] } })).rejects.toThrowError(/non-empty array/); + await expect(ids({ $not: { $or: [] } })).rejects.toThrowError(/non-empty array/); + await expect(ids({ $or: [{ stage: 'won' }, { $and: [] }] })).rejects.toThrowError(/non-empty array/); + }); + + it('an unknown operator inside a `$not` still THROWS rather than being guarded', async () => { + await expect(ids({ $not: { stage: { $regex: '.*' } } })).rejects.toThrowError(/Unsupported filter operator/); + await expect(ids({ $not: { $nor: [{ stage: 'won' }] } })).rejects.toThrowError(/Unsupported top-level filter operator/); + }); + + it('a non-object `$not` operand is REFUSED, not silently dropped', async () => { + // Was: `typeof null === 'object'` → `buildNode` on a non-node → no node → + // the negation vanished and the query widened to every row. A `$or` branch + // of garbage is refused for the mirror reason: skipping it narrows, and + // reading it as TRUE widens. + await expect(ids({ $not: null })).rejects.toThrowError(/"\$not" requires a filter object/); + await expect(ids({ $not: 'x' })).rejects.toThrowError(/"\$not" requires a filter object/); + await expect(ids({ $not: [] })).rejects.toThrowError(/"\$not" requires a filter object/); + await expect(ids({ $or: [{ stage: 'won' }, 'x'] })).rejects.toThrowError(/branches must be filter objects/); + await expect(ids({ $and: [null] })).rejects.toThrowError(/branches must be filter objects/); + }); + + it('a zero-operator field constraint is REFUSED (#5240), not read as TRUE', async () => { + // Forced by this change rather than chosen: `{a: {}}` produced no leaf, + // "no leaf" IS the constant TRUE, and TRUE now absorbs a `$or` — so left + // alone, `{$or: [{a: {}}, {b: 2}]}` would have gone from `b = 2` to every + // row. #5240 already ruled the shape refused on every backend (driver-sql, + // driver-memory, formula); this is the same refusal at the analytics door. + await expect(ids({ stage: {} })).rejects.toThrowError(/zero operators/); + await expect(ids({ $or: [{ stage: {} }, { owner: 'u1' }] })).rejects.toThrowError(/zero operators/); + await expect(ids({ $not: { stage: {} } })).rejects.toThrowError(/zero operators/); + // A nested relation is NOT this shape and still flattens. + const { sql } = await sqlFor({ account: { region: 'NA' } }); + expect(sql).toContain('"account"."region" = $1'); + }); + + it('an undefined value is still skipped, as it always was', async () => { + expect(await ids({ stage: undefined, owner: 'u1' })).toEqual(['1', '3']); + expect(await ids({ $not: undefined, owner: 'u1' })).toEqual(['1', '3']); + }); + + it('an ordinary filter compiles to exactly the SQL it always did', async () => { + const { sql, params } = await sqlFor({ stage: 'won' }); + expect(sql).toContain('WHERE stage = $1'); + expect(params).toEqual(['won']); + expect(await ids({ stage: 'won' })).toEqual(['1']); + // Still three-valued OUTSIDE a negation: `!= 'won'` drops the NULL rows. + // That divergence from the JS backends is real and out of #5146's scope. + expect(await ids({ stage: { $ne: 'won' } })).toEqual(['2']); + expect(await ids({})).toEqual(ALL); + }); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index c2c84f3a47..b3bb6cd8ac 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -46,11 +46,54 @@ * now a {@link NormalizedFilterNode} tree, and each strategy compiles it the * way its own backend expresses a disjunction. * + * # `null` is TRUE, and TRUE is a VALUE — not "nothing happened" (#5325) + * + * {@link buildNode} returns `null` for a condition that constrains nothing + * (`{}`, an all-`{}` `$and`). That `null` is the boolean constant TRUE, and the + * two places a compiler forgets it are exactly where this one used to be wrong — + * the same two squares `read-scope-sql.ts` was wrong on (#5297), because this + * module was written from it: + * + * - TRUE is the AND identity, so dropping it from a `$and` is right — but it + * ABSORBS a `$or`: one TRUE disjunct makes the whole disjunction TRUE. + * Filtering it out (`{$or: [{}, {a: 1}]}` → `a = 1`) silently NARROWED a + * widget's filter to its surviving branches. + * - `NOT TRUE ≡ FALSE`, so `{$not: {}}` is the zero-row predicate. Producing + * `null` for it meant no `WHERE` was emitted at all and the widget charted + * the ENTIRE dataset — the #3650 / #4128 silent-widening class again. + * + * FALSE therefore has a spelling of its own ({@link NormalizedFilterNode}'s + * `const` kind) instead of being representable only as silence. Every compiler + * of this tree implements it: `native-sql-strategy.compileFilterNode`, + * `objectql-strategy.filterNodeToCondition` and its display-SQL twin + * `renderFilterNodeSql`. + * + * # `$not` is NULL-safe (#5146) + * + * SQL is three-valued and a `WHERE` keeps only TRUE, so a bare `NOT (col = ?)` + * drops every row whose `col` is NULL — while `driver-memory`, `formula` and + * (since #5296) `driver-sql` return those rows. One widget filter, two row sets, + * chosen by whichever backend answered. #5146 ruled the JS answer canonical, and + * {@link nullSafeNegationOperand} applies the same leaf-wise totalisation + * `sql-driver.ts` and `read-scope-sql.ts` apply. + * + * The rewrite lives HERE rather than in `native-sql-strategy` on purpose: at + * this layer the guard is STRUCTURE (one more `{col: {$null: false}}` conjunct), + * not a SQL trick, so it survives `filterNodeToCondition` handing the tree to + * the ObjectQL engine and holds on any driver behind it — including one that is + * not NULL-safe by itself. Guarding only in the SQL strategy would make "what + * does this widget's `$not` mean" depend on which backend caught it, which is + * what #5146 spent a round eliminating. The cost is that the engine path can + * guard twice (this rewrite, then `driver-sql`'s own); that is idempotent — + * `NOT (c IS NOT NULL AND (c IS NOT NULL AND c = v))` is the same predicate — + * so it buys portability for one redundant conjunct. + * * 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. + * vocabulary, `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, and + * `filter-normalizer-not-null-safe.test.ts` for the two squares that table + * deliberately does not carry (NULL handling, boolean identities). */ export interface NormalizedAnalyticsFilter { @@ -112,13 +155,58 @@ function stringifyForCube(v: unknown): string { * 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. + * + * The `const` kind is the boolean constant (#5325). The union carried only + * `leaf | and | or | not`, so there was no way to SAY "matches nothing": a + * `{$not: {}}` — whose meaning is exactly that — could only be expressed by + * emitting nothing, which every compiler reads as "no constraint", i.e. the + * opposite. TRUE keeps its existing spelling (`null` = no constraint, the AND + * identity); FALSE needs a node because it must survive into the WHERE clause. */ export type NormalizedFilterNode = | { kind: 'leaf'; member: string; operator: string; values: string[] } + | { kind: 'const'; value: boolean } | { kind: 'and'; children: NormalizedFilterNode[] } | { kind: 'or'; children: NormalizedFilterNode[] } | { kind: 'not'; child: NormalizedFilterNode }; +/** + * The SQL boolean constants the compilers of this tree emit for a `const` node. + * + * `1 = 0` / `1 = 1` are the spellings already used on both sides of the repo — + * `read-scope-sql.ts` compiles an empty `$in` to `1 = 0`, `driver-sql`'s + * `applyFalseConstant` emits the same (#5134), and Knex renders an empty + * `whereIn` that way. They need no bindings, are valid on every dialect these + * strategies target (unlike a bare `FALSE`), and keep the statement a normal + * SELECT so `GROUP BY` / `LIMIT` still behave. + */ +export const SQL_CONST_FALSE = '1 = 0'; +export const SQL_CONST_TRUE = '1 = 1'; + +/** The tree's FALSE. A fresh object per call — nodes are never shared. */ +function falseNode(): NormalizedFilterNode { + return { kind: 'const', value: false }; +} + +/** + * `NOT` of a node, with `null` read as the constant TRUE it is. + * + * `NOT TRUE ≡ FALSE` is the whole point: `{$not: {}}` used to fall off the tree + * here, taking the WHERE clause with it (#5325). A `NOT` of a constant folds to + * the opposite constant, so `{$not: {$not: {}}}` is TRUE again rather than a + * `NOT (1 = 0)` that only happens to evaluate right. + */ +function notOf(inner: NormalizedFilterNode | null): NormalizedFilterNode { + if (!inner) return falseNode(); + if (inner.kind === 'const') return { kind: 'const', value: !inner.value }; + return { kind: 'not', child: inner }; +} + +/** A node the normalizer can walk: a plain object, not `null` and not an array. */ +function isFilterObject(v: unknown): v is Record { + return v !== null && typeof v === 'object' && !Array.isArray(v) && !(v instanceof Date); +} + /** `null` means "no constraint" — an empty object contributes no predicate. */ function andOf(children: NormalizedFilterNode[]): NormalizedFilterNode | null { if (children.length === 0) return null; @@ -146,6 +234,21 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { if (typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { const wrapper = raw as Record; + // A field constrained by ZERO operators, ruled on in #5240: REFUSE it, the + // way `driver-sql`, `driver-memory` and `formula` now do. This module used + // to produce no leaf for it — and "no leaf" is the constant TRUE, which is + // load-bearing since #5325 made TRUE absorb a `$or`: left alone, + // `{$or: [{a: {}}, {b: 2}]}` would have gone from `b = 2` to EVERY row. + // Neither silent reading is the author's intent (the shape is an authoring + // accident — a filter builder that recorded a field and never its operator), + // and a loud refusal is the answer the rest of the repo already gives. + if (Object.keys(wrapper).length === 0) { + throw new Error( + `[analytics] "${key}" carries a field constraint with zero operators ({}). ` + + `Refusing rather than reading it as "every row" or "no row" — #5240 ruled this ` + + `shape refused on every backend.`, + ); + } const opKeys = Object.keys(wrapper).filter((k) => k.startsWith('$')); if (opKeys.length > 0) { for (const opKey of opKeys) { @@ -194,6 +297,19 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { continue; } + // An EMPTY set is a boolean constant, not an absent predicate (#5134). + // `buildFilterClause` returns `null` for a value-less `in`/`notIn`, and + // a `null` clause is read as "no constraint" by every compiler of this + // tree — so `{stage: {$in: []}}` charted every row instead of none. It + // also has to be a CONSTANT rather than a dropped clause for the + // NULL-safe `$not` rewrite below to stay correct: a dropped conjunct + // inside a negation flips the whole negation's answer, while `1 = 0` + // negates to `1 = 1` the way `read-scope-sql.ts` already has it. + if ((opKey === '$in' || opKey === '$nin') && Array.isArray(wrapper[opKey]) && (wrapper[opKey] as unknown[]).length === 0) { + out.push({ kind: 'const', value: opKey === '$nin' }); + continue; + } + const cubeOp = MONGO_TO_CUBE_OP[opKey]; if (!cubeOp) { // NEVER drop: a missing predicate does not narrow the query, it @@ -223,21 +339,24 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { return out; } - // Implicit equality / array → in - if (Array.isArray(raw)) leaf('in', raw.map(stringifyForCube)); - else leaf('equals', [stringifyForCube(raw)]); + // Implicit equality / array → in. An empty array is the same constant its + // explicit `{$in: []}` spelling is — see the note at that branch. + if (Array.isArray(raw)) { + if (raw.length === 0) out.push({ kind: 'const', value: false }); + else leaf('in', raw.map(stringifyForCube)); + } else leaf('equals', [stringifyForCube(raw)]); return out; } /** - * Compile a `FilterCondition` object into a node. `null` = no constraint. + * Compile a `FilterCondition` object into a node. `null` = no constraint (TRUE). * * 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. + * `compileNode`, including its fail-closed empty-array rejection AND (since + * #5325) its treatment of the two boolean identities, 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[] = []; @@ -253,20 +372,53 @@ function buildNode(cond: Record): NormalizedFilterNode | null { `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; + const branches = raw.map((sub) => { + // A non-object element is refused rather than skipped: skipping it + // NARROWS a `$or` to its remaining branches and, under the TRUE-absorbs + // rule below, would otherwise have to be read as TRUE and widen the + // query to every row. Neither is a defensible reading of garbage input — + // `read-scope-sql.ts` refuses the same shape. + if (!isFilterObject(sub)) { + throw new Error( + `[analytics] "${key}" branches must be filter objects, got ${JSON.stringify(sub)}. ` + + `Skipping it would silently change which rows the filter admits.`, + ); + } + return buildNode(sub); + }); + // A `null` branch is the constant TRUE. It is the AND identity, so it + // drops out of a `$and` — but it ABSORBS a `$or`: one TRUE disjunct makes + // the whole disjunction TRUE, so the group contributes NO constraint + // rather than collapsing to its surviving branches. Collapsing is what + // narrowed `{$or: [{}, {stage: 'won'}]}` to `stage = 'won'` (#5325). + if (key === '$or' && branches.some((n) => n === null)) continue; + const kept = branches.filter((n): n is NormalizedFilterNode => n !== null); + if (kept.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 }); + if (key === '$and') children.push(...kept); + else children.push(kept.length === 1 ? kept[0] : { kind: 'or', children: kept }); continue; } if (key === '$not') { - const inner = raw && typeof raw === 'object' ? buildNode(raw as Record) : null; - if (inner) children.push({ kind: 'not', child: inner }); + if (!isFilterObject(raw)) { + // Same call as the branch elements above: a `$not` of garbage used to + // vanish, which turns "exclude these rows" into "exclude nothing". + throw new Error( + `[analytics] "$not" requires a filter object, got ${JSON.stringify(raw)}. ` + + `Dropping it would silently widen the query to rows the filter excludes.`, + ); + } + // NULL-safe negation (#5146): totalise the operand's leaves FIRST, so the + // negation can never be UNKNOWN and this path admits the same rows + // `driver-memory` / `formula` / `driver-sql` admit. The guard is added as + // STRUCTURE here, which is what makes it survive into the ObjectQL engine + // path too (see the module header). + const inner = buildNode(nullSafeNegationOperand(raw)); + // `notOf` turns a TRUE operand into FALSE instead of nothing: `{$not: {}}` + // is the zero-row filter, and emitting nothing for it charted every row. + children.push(notOf(inner)); continue; } @@ -283,6 +435,210 @@ function buildNode(cond: Record): NormalizedFilterNode | null { return andOf(children); } +// ── [#5146 / #5325] NULL-safe `$not` ───────────────────────────────────────── + +/** + * What one field constraint needs so the leaves it produces are TOTAL — TRUE or + * FALSE for every row, never UNKNOWN. + * + * - `'none'` — already total (`set` / `notSet`, a boolean constant), or + * a shape this normalizer refuses, which must keep refusing. + * - `'requireValue'` — a NULL column does NOT satisfy it: `col IS NOT NULL AND (…)`. + * - `'allowNull'` — a NULL column DOES satisfy it: `col IS NULL OR (…)`. + */ +type NullGuard = 'none' | 'requireValue' | 'allowNull'; + +/** + * Does a NULL column satisfy this one operator, under the semantics the JS + * backends (`driver-memory`'s `match`, `formula`'s `matchesFilterCondition`) + * give it? They evaluate a missing value in ordinary two-valued JS — `undefined + * !== 'won'` is simply `true` — and #5146 ruled that answer canonical. + * + * This is `sql-driver.ts`'s and `read-scope-sql.ts`'s table, with the + * differences that come from THIS module's emitter rather than from a different + * reading of #5146 — each guard matches its own emitter, which is the invariant, + * not the literal table: + * + * - `$null` / `$exists` are read by IDENTITY (`=== true` / `=== false`) + * because {@link fieldLeaves} reads them that way, where `read-scope-sql` + * uses truthiness because its emitter does. Immaterial in practice: both + * compile to a null predicate, so they are total either way and never + * reach the polarity question. + * - `$between` exists in this vocabulary; it lowers to `gte` + `lte`, two + * positive comparisons, so it takes the same default they do. + * - `$eq` / `$ne` do NOT get `read-scope-sql`'s `value === null` arms. That + * compiler turns a `null` comparand into `IS NULL` / `IS NOT NULL`; this one + * stringifies it (`stringifyForCube(null)` → `''`) and compares against the + * empty string, so `{$eq: null}` here is an ordinary value comparison. The + * guard follows the emitter; the `''` comparand itself is a separate defect, + * filed on its own and deliberately not decided here. + * + * The default is the large positive-comparison family (`$gt` / `$in` / + * `$contains` / …), every member of which answers `false` for a value that is + * not there. An operator this module does not support also lands here; it is + * guarded and then still THROWS from {@link fieldLeaves}, so fail-closed is + * preserved. + */ +function nullValueSatisfiesOperator(op: string, value: unknown): boolean { + switch (op) { + case '$ne': return true; + case '$null': return value === true; + case '$exists': return value === false; + // Negative-polarity set / substring tests hold vacuously for an absent value. + case '$nin': return true; + // `$notContains` is the one operator where the two JS backends disagree for + // a null-valued field (`driver-memory` answers false, `formula` true). + // `formula` is followed because `driver-sql` and `read-scope-sql` follow it, + // so this module casts no vote on a disagreement that is filed elsewhere. + case '$notContains': return true; + default: return false; + } +} + +/** Is this operator's compiled leaf already total for a NULL column? */ +function operatorIsNullTotal(op: string, value: unknown): boolean { + switch (op) { + // Compile to `set` / `notSet` — `IS NULL` / `IS NOT NULL`, two-valued by + // construction, on every strategy that compiles this tree. + case '$null': + case '$exists': + return true; + // An EMPTY set compiles to a boolean CONSTANT (see `fieldLeaves`), and a + // constant is total. Wrapping a guard around it would only add a redundant + // conjunct to a predicate whose value is already decided. + case '$in': + case '$nin': + return Array.isArray(value) && value.length === 0; + default: + return false; + } +} + +/** + * The guard one field constraint needs. A constraint is the AND of its + * operators, so it is total when every operator is, and a NULL column satisfies + * it only when it satisfies all of them. + */ +function nullGuardForFieldSpec(spec: unknown): NullGuard { + // `{field: null}` compiles to `notSet` (`IS NULL`) — already total. + if (spec === null) return 'none'; + // A bare array is an implicit `$in`; an EMPTY one is the FALSE constant. + if (Array.isArray(spec)) return spec.length === 0 ? 'none' : 'requireValue'; + // A scalar / Date is an implicit `=`; a NULL column fails it. + if (typeof spec !== 'object' || spec instanceof Date) return 'requireValue'; + const entries = Object.entries(spec as Record); + // `{field: {}}` is REFUSED by `fieldLeaves` (#5240). Passing it through + // unrewritten is what keeps that refusal reachable — a guard wrapped around it + // would only change which message the caller sees. + if (entries.length === 0) return 'none'; + let total = true; + let nullSatisfies = true; + for (const [op, value] of entries) { + if (!operatorIsNullTotal(op, value)) total = false; + if (!nullValueSatisfiesOperator(op, value)) nullSatisfies = false; + } + if (total) return 'none'; + return nullSatisfies ? 'allowNull' : 'requireValue'; +} + +/** + * Guard one `field: spec` entry, writing either the untouched entry into `out` + * or its guarded form into `guarded`. + * + * A nested relation spec (`{account: {region: 'NA'}}`) is flattened with the + * dotted key {@link fieldLeaves} would have produced, so the guard lands on the + * SAME member as the leaf it protects — guarding `account` when the leaf reads + * `account.region` would test a column that does not exist. + */ +function guardFieldEntry( + key: string, + spec: unknown, + out: Record, + guarded: unknown[], +): void { + if ( + isFilterObject(spec) && + Object.keys(spec).length > 0 && + !Object.keys(spec).some((k) => k.startsWith('$')) + ) { + for (const [nested, value] of Object.entries(spec)) { + guardFieldEntry(`${key}.${nested}`, value, out, guarded); + } + return; + } + + const guard = nullGuardForFieldSpec(spec); + if (guard === 'none') { + out[key] = spec; + } else if (guard === 'requireValue') { + // `col IS NOT NULL AND (…)` — both conjuncts of the enclosing node. + guarded.push({ [key]: { $null: false } }, { [key]: spec }); + } else { + // `col IS NULL OR (…)` — one conjunct, so the OR binds tighter than the AND + // this node's keys form. + guarded.push({ $or: [{ [key]: { $null: true } }, { [key]: spec }] }); + } +} + +/** + * [#5146] Rewrite the operand of a `$not` so every leaf compiles to a TOTAL + * predicate — which is what makes `NOT (…)` mean here what it means in + * `driver-memory`, `formula` and (since #5296) `driver-sql`. + * + * # Why the guard rides the LEAF, not the `NOT` + * + * For a flat operand `NOT (a IS NOT NULL AND a = ?)` and `NOT (a = ?) OR a IS + * NULL` are the same predicate. They stop being the same as soon as the operand + * nests: hoisting the guard above a `$not` whose operand is a `$or` re-admits + * rows the JS backends exclude — a NULL `a` would satisfy the whole negation + * even when the `$or`'s OTHER branch is satisfied. Totalising each leaf makes + * the rewrite compositional instead: De Morgan is sound over two-valued leaves, + * so `$and`, `$or` and a nested `$not` all stay correct with no special cases. + * + * # Why polarity is per operator + * + * A blanket `OR col IS NULL` would WIDEN the negative-polarity operators: + * `{$not: {a: {$ne: 5}}}` means "a is 5", and both JS backends exclude a NULL + * row from it. Adding an unconditional null escape there would hand back exactly + * the rows the filter excludes. So each leaf is guarded in the direction its own + * operator answers, per {@link nullValueSatisfiesOperator}. + * + * # Why it is a REWRITE of the condition, not of the tree + * + * The output is still a `FilterCondition`, so `buildNode` compiles it with no + * new cases and — the point of doing it here rather than in the SQL strategy — + * the guard reaches the ObjectQL engine as structure too. Running only inside a + * `$not` keeps every other comparison's shape untouched, and a NESTED `$not` is + * left alone on purpose: its own branch totalises its operand, and + * `NOT ` is itself total, so recursing would stack a redundant guard on + * the same column. + */ +function nullSafeNegationOperand(node: Record): Record { + const out: Record = {}; + const guarded: unknown[] = []; + for (const [key, value] of Object.entries(node)) { + if ((key === '$and' || key === '$or') && Array.isArray(value)) { + // A non-object element is passed through so `buildNode` still refuses it + // with its own message. + out[key] = value.map((element) => (isFilterObject(element) ? nullSafeNegationOperand(element) : element)); + continue; + } + if (key.startsWith('$')) { + // `$not` (handled by its own branch) and anything else `$`-prefixed keep + // whatever this module does with them today — the rewrite rules on NULL, + // not on the operator vocabulary, and an unknown one must still throw. + out[key] = value; + continue; + } + guardFieldEntry(key, value, out, guarded); + } + if (guarded.length > 0) { + const existing = Array.isArray(out.$and) ? out.$and : []; + out.$and = [...existing, ...guarded]; + } + return out; +} + /** * Normalize an analytics query's `where` (FilterCondition) into the tree the * strategies compile. `null` when the query carries no `where`. @@ -309,6 +665,9 @@ export function collectFilterLeaves( ): NormalizedAnalyticsFilter[] { if (!node) return []; if (node.kind === 'leaf') return [{ member: node.member, operator: node.operator, values: node.values }]; + // A boolean constant names no member — it constrains rows, not columns — so + // it contributes nothing to the cross-object envelope check. + if (node.kind === 'const') return []; if (node.kind === 'not') return collectFilterLeaves(node.child); return node.children.flatMap(collectFilterLeaves); } 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 688af29e75..9d274981cf 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -6,6 +6,8 @@ import type { AnalyticsStrategy, StrategyContext } from './types.js'; import { normalizeAnalyticsFilterTree, coerceFilterValueForSql, + SQL_CONST_FALSE, + SQL_CONST_TRUE, type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; @@ -574,6 +576,25 @@ export class NativeSQLStrategy implements AnalyticsStrategy { * 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. + * + * # `null` is the constant TRUE, and TRUE absorbs a disjunction (#5325) + * + * A `null` return means "constrains nothing", which is the boolean TRUE — the + * AND identity, so it drops out of an `and`, but the OR ABSORBER, so one TRUE + * disjunct makes the whole `or` TRUE. Filtering it out of an `or` narrowed the + * query to the surviving branches. `NOT TRUE ≡ FALSE`, so a negation whose + * operand constrains nothing compiles to the FALSE constant rather than + * disappearing (which added no `WHERE` and charted every row). + * + * # The invariant that keeps `params` aligned + * + * **A call that returns `null` leaves `params` exactly as it found it.** It + * has to: a value bound with no `$n` to consume it shifts every later + * placeholder onto the wrong value, and a filter that binds the WRONG comparand + * is worse than one that is merely too wide (#5297). Leaves decide emptiness + * before they bind, and the absorbing `or` — the one place a clause that HAS + * bound is discarded — truncates back to the length it started at, so the + * invariant holds inductively for every node kind. */ private compileFilterNode( node: NormalizedFilterNode | null, @@ -585,6 +606,10 @@ export class NativeSQLStrategy implements AnalyticsStrategy { ): string | null { if (!node) return null; + if (node.kind === 'const') { + return node.value ? SQL_CONST_TRUE : SQL_CONST_FALSE; + } + if (node.kind === 'leaf') { const colExpr = this.resolveFieldSql(cube, node.member, parentTable, joins); // Resolve the (object, column) this member binds against so the value @@ -595,12 +620,30 @@ export class NativeSQLStrategy implements AnalyticsStrategy { if (node.kind === 'not') { const inner = this.compileFilterNode(node.child, cube, parentTable, joins, params, ctx); - return inner ? `NOT (${inner})` : null; + // `NOT TRUE ≡ FALSE`. Returning `null` here is what made `{$not: {}}` emit + // no `WHERE` at all — a filter meaning "no rows" that showed all of them. + // The normalizer already folds that case into a `const` node; this arm is + // the same identity applied to anything else that constrains nothing. + return inner ? `NOT (${inner})` : SQL_CONST_FALSE; } - const parts = node.children - .map((child) => this.compileFilterNode(child, cube, parentTable, joins, params, ctx)) - .filter((s): s is string => !!s); + // Everything committed before this group, so an absorbed `or` can put both + // back exactly as they were. + const paramBase = params.length; + const joinBase = new Map(joins); + const parts: string[] = []; + for (const child of node.children) { + const clause = this.compileFilterNode(child, cube, parentTable, joins, params, ctx); + if (clause === null) { + // TRUE: the AND identity, the OR absorber. + if (node.kind !== 'or') continue; + params.length = paramBase; + joins.clear(); + for (const [alias, clauseSql] of joinBase) joins.set(alias, clauseSql); + return null; + } + parts.push(clause); + } if (parts.length === 0) return null; if (parts.length === 1) return parts[0]; return `(${parts.join(node.kind === 'or' ? ' OR ' : ' AND ')})`; diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 7eefa3fbfe..7a5a7bc9b0 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -7,6 +7,8 @@ import { normalizeAnalyticsFilterTree, collectFilterLeaves, coerceFilterValueForObjectQL, + SQL_CONST_FALSE, + SQL_CONST_TRUE, type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; @@ -768,23 +770,39 @@ export class ObjectQLStrategy implements AnalyticsStrategy { if (rendered) conjuncts.push(rendered); } - /** A node as a standalone `FilterCondition` the engine can consume. */ + /** + * A node as a standalone `FilterCondition` the engine can consume. + * + * `null` = no constraint, which is the boolean TRUE — the AND identity but the + * OR ABSORBER, so a `null` branch makes the whole disjunction unconstrained + * instead of collapsing it to its surviving branches (#5325). FALSE is handed + * to the engine as `{$not: {}}`, the spelling `driver-sql`, `formula` and + * `driver-memory`'s matcher all already pin as the zero-row filter (#5134) — + * this strategy invents no second one. + */ private filterNodeToCondition( node: NormalizedFilterNode | null, cube: Cube, ): Record | null { if (!node) return null; + if (node.kind === 'const') { + return node.value ? null : { $not: {} }; + } + if (node.kind === 'not') { const inner = this.filterNodeToCondition(node.child, cube); - return inner ? { $not: inner } : null; + // `NOT TRUE ≡ FALSE` — a negation of nothing is the zero-row filter, not + // the absence of a filter (which is what let `{$not: {}}` chart every row). + return inner ? { $not: inner } : { $not: {} }; } 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; + const branches = node.children.map((child) => this.filterNodeToCondition(child, cube)); + // One TRUE disjunct absorbs the disjunction. + if (branches.some((c) => c === null)) return null; + const kept = branches.filter((c): c is Record => !!c); + return kept.length > 0 ? { $or: kept } : null; } // `leaf` and `and` share the merge path so one field carrying several @@ -802,6 +820,14 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * 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. + * + * The boolean identities render too (#5325). This string exists to REPRODUCE + * execution: a `{$not: {}}` filter that runs as zero rows but echoes SQL with + * no `WHERE` hands whoever is debugging "why is this chart empty" a statement + * that returns the whole table. Same reason the absorbed `$or` branch and the + * `params` truncation below match {@link NativeSQLStrategy.compileFilterNode} + * exactly — including the invariant that a `null` return leaves `params` + * untouched, so no comparand is left with no placeholder to consume it. */ private renderFilterNodeSql( node: NormalizedFilterNode | null, @@ -810,6 +836,10 @@ export class ObjectQLStrategy implements AnalyticsStrategy { ): string | null { if (!node) return null; + if (node.kind === 'const') { + return node.value ? SQL_CONST_TRUE : SQL_CONST_FALSE; + } + if (node.kind === 'leaf') { return this.buildFilterClauseSql( this.resolveFieldName(cube, node.member, 'any'), @@ -821,12 +851,20 @@ export class ObjectQLStrategy implements AnalyticsStrategy { if (node.kind === 'not') { const inner = this.renderFilterNodeSql(node.child, cube, params); - return inner ? `NOT (${inner})` : null; + return inner ? `NOT (${inner})` : SQL_CONST_FALSE; } - const parts = node.children - .map((child) => this.renderFilterNodeSql(child, cube, params)) - .filter((s): s is string => !!s); + const paramBase = params.length; + const parts: string[] = []; + for (const child of node.children) { + const clause = this.renderFilterNodeSql(child, cube, params); + if (clause === null) { + if (node.kind !== 'or') continue; + params.length = paramBase; + return null; + } + parts.push(clause); + } if (parts.length === 0) return null; if (parts.length === 1) return parts[0]; return `(${parts.join(node.kind === 'or' ? ' OR ' : ' AND ')})`;