diff --git a/.changeset/sql-driver-not-null-safe.md b/.changeset/sql-driver-not-null-safe.md new file mode 100644 index 0000000000..70a21b3725 --- /dev/null +++ b/.changeset/sql-driver-not-null-safe.md @@ -0,0 +1,54 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): `$not` 改为 NULL-safe —— 被比较列为 NULL 的行不再被否定条件静默排除 + +**这是一处可观察的查询行为变更,且直接关系到 RLS 的可见集合。** +`{ $not: { stage: 'won' } }` 以前**不返回** `stage IS NULL` 的行,现在**返回**它们。 +如果你的规则依赖了旧行为,它依赖的是「同一条规则在不同后端给出不同可见集合」。 + +SQL 是三值逻辑:`NULL = 'won'` 是 UNKNOWN,`NOT UNKNOWN` 仍是 UNKNOWN,而 `WHERE` +只保留 TRUE。于是 `applyFilterCondition` 编译出的裸 `NOT (stage = 'won')` 会把 +「该列没有值」的行整批丢掉;同一条 filter 在 `driver-memory` 与 `formula` 的 +`matchesFilterCondition` 上是普通的两值 JS 求值(`undefined !== 'won'` → 行匹配), +两边把这些行**都返回**。一个 spec 声明的算子,答案取决于跑它的是哪个驱动。 + +这不是「数目对不上」而已:权限规则里的 CEL `!expr` 经 `cel-to-filter.ts` 正是降解成 +`{ $not: {…} }`,所以同一条 read scope 在 SQL 数据源与内存数据源上准入的行集不同。 +#5146 判定以 JS 家族的答案为准(2:1 的多数派;写 `!(stage == 'won')` 的人不会预期 +「stage 为空的行被隐藏」),本次把 SQL 侧对齐过去。 + +**编译出来的形状。** `$not` 的操作数在取反之前先被改写成**全域(total)谓词** —— +永远是 TRUE 或 FALSE,不会是 UNKNOWN: + +```sql +-- 之前 +not (`stage` = 'won') +-- 现在 +not ((`stage` is not null) and (`stage` = 'won')) +``` + +对 issue 里给出的扁平形状,这与 `NOT (…) OR col IS NULL` 完全等价。把守卫下推到 +**每个叶子**而不是挂在 `NOT` 旁边,是为了在操作数嵌套时仍然正确:`$not` 里套一个 +`$or` 时,顶层的 `OR col IS NULL` 会把 JS 家族排除的行重新放进来(某一列为 NULL、 +但另一个析取分支成立的行)。 + +**守卫方向按算子逐个判定,不是一刀切。** `{ $not: { a: { $ne: 5 } } }` 的语义是 +「a 就是 5」,两个 JS 后端都把 NULL 行排除在外;无条件加 `OR a IS NULL` 会把这些行 +交回去 —— 正是本驱动反复付过学费的静默放松(#2704 / #5134)。因此 +`$ne` / `$nin` / `$notContains` 用的是 `col IS NULL OR (…)`,`$eq` / `$in` / +`$gt` / `$contains` 一族用 `col IS NOT NULL AND (…)`,而 `$null` / `$exists` / +`$eq: null` / `$ne: null` 本来就是全域谓词,一个字节都不加。 + +**只有 `$not` 路径被改写。** 普通比较的 SQL 逐字符不变(`{ a: 1 }` 仍然是 +`a = 1`),因此没有任何非否定谓词因此失去索引;`$not` 路径上的 `IS NOT NULL` 守卫 +本身处在一个原本就不可 sargable 的 `NOT (…)` 里。 + +`#5134` / PR #5243 定下的布尔单位元(`{ $not: {} }` → 零行、`$not` of FALSE → +全部行、非 filter 节点的操作数按 ADR-0112 响亮拒收)全部保持不变;`{ field: {} }` +(#5240)也刻意不在此裁定 —— 它编译出的 SQL 与之前完全一致。 + +`driver-memory` 与 `formula` 无需改动,本次为三家各补了一组 pin 测试,把「值缺失 +行在 `$not` 下的去留」钉在一起。跨驱动 conformance case(`FILTER_LOGIC_CASES`)与 +契约 TSDoc 归 spec 车道,随 #5239 落地。 diff --git a/packages/formula/src/matches-filter-not-null-safe.test.ts b/packages/formula/src/matches-filter-not-null-safe.test.ts new file mode 100644 index 0000000000..799ccea704 --- /dev/null +++ b/packages/formula/src/matches-filter-not-null-safe.test.ts @@ -0,0 +1,154 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5146] `$not` and absent values — the answers this evaluator gives, pinned. + * + * `matchesFilterCondition` needed no change for #5146: it already negates in + * ordinary two-valued JS, so `{ $not: { stage: 'won' } }` matches a record whose + * `stage` is null or missing. `driver-sql` used to disagree (SQL's + * `NOT (stage = 'won')` is UNKNOWN for a NULL column, and a `WHERE` drops it), + * so the SAME rule admitted different rows per backend — and this evaluator is + * the RLS write-side `check`, i.e. the half that decides whether a write is + * allowed, against read scopes compiled elsewhere. #5146 ruled this answer + * canonical and rewrote the SQL compiler to match it. + * + * These cases are therefore a PIN on the reference behaviour, mirrored id-for-id + * by `driver-sql`'s `sql-driver-not-null-safe.test.ts` and `driver-memory`'s + * `memory-matcher-not-null-safe.test.ts`. Moving an expectation here silently + * re-opens the divergence. + * + * `cel-to-filter.ts` is why this matters in practice: a CEL `!expr` in a + * permission rule lowers to exactly these `$not` shapes. + * + * Home for these eventually: `FILTER_LOGIC_CASES` in `@objectstack/spec/data` + * (spec lane, with #5239). + */ + +import { describe, it, expect } from 'vitest'; +import { matchesFilterCondition } from './matches-filter.js'; +import type { FilterCondition } from '@objectstack/spec/data'; + +/** Fields present but null — how a SQL NULL round-trips into a record. */ +const NULLED: Array> = [ + { 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 }, +]; + +/** The same rows with the null fields ABSENT — a partial write's post-image. */ +const MISSING: Array> = [ + { id: '1', stage: 'won', owner: 'u1', amount: 10 }, + { id: '2', stage: 'lost', owner: 'u2', amount: 20 }, + { id: '3', owner: 'u1' }, + { id: '4', amount: 40 }, +]; + +const ALL = ['1', '2', '3', '4']; + +const ids = (rows: Array>, filter: unknown): string[] => + rows.filter((r) => matchesFilterCondition(r, filter as FilterCondition)).map((r) => String(r.id)); + +/** Both readings of "no value" must give the same answer unless noted. */ +const matched = (filter: unknown): string[] => { + const nulled = ids(NULLED, filter); + expect(ids(MISSING, filter), 'a null field and an absent field must match alike').toEqual(nulled); + return nulled; +}; + +describe('[#5146] matchesFilterCondition — $not over records with no value', () => { + describe('a record with no value does not satisfy the negated condition', () => { + it('$not on an implicit equality matches the value-less records', () => { + expect(matched({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + }); + + it('$not over multiple keys matches a record missing EITHER', () => { + expect(matched({ $not: { stage: 'won', owner: 'u1' } })).toEqual(['2', '3', '4']); + }); + + it('the RLS shape: a CEL `!(stage == "won")` check keeps stage-less records', () => { + expect(matched({ $not: { stage: 'won' } })).toHaveLength(3); + }); + }); + + describe('nesting', () => { + it('$not of a $or rejects a value-less record whose OTHER branch matches', () => { + // Record 3 has no stage but owner = 'u1', so the $or holds and the + // negation rejects it. This is the case that forced `driver-sql` to put + // its NULL guard on each leaf rather than beside the `NOT`. + expect(matched({ $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '4']); + }); + + it('$not of a $and matches every record failing either conjunct', () => { + expect(matched({ $not: { $and: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '3', '4']); + }); + + it('a double negation is the positive filter again', () => { + expect(matched({ $not: { $not: { stage: 'won' } } })).toEqual(['1']); + expect(matched({ $not: { $not: { stage: 'won' } } })).toEqual(matched({ stage: 'won' })); + }); + + it('$not ANDs with its sibling keys', () => { + expect(matched({ $not: { stage: 'won' }, owner: 'u1' })).toEqual(['3']); + }); + }); + + describe('operator polarity — a negation is not a blanket "and also the empty ones"', () => { + it('$not of $ne still means "the field IS that value"', () => { + expect(matched({ $not: { stage: { $ne: 'won' } } })).toEqual(['1']); + }); + + it('$not of $nin still means "the field IS among them"', () => { + expect(matched({ $not: { stage: { $nin: ['won'] } } })).toEqual(['1']); + }); + + it('$not of $in matches the value-less records', () => { + expect(matched({ $not: { stage: { $in: ['won'] } } })).toEqual(['2', '3', '4']); + }); + + it('$not of an ordering comparison matches the value-less records', () => { + expect(matched({ $not: { amount: { $gt: 15 } } })).toEqual(['1', '3']); + }); + + it('$not of $contains matches the value-less records', () => { + expect(matched({ $not: { stage: { $contains: 'w' } } })).toEqual(['2', '3', '4']); + }); + + it('$not of $notContains does NOT match them — the mirror case', () => { + // A value-less field satisfies `$notContains` here, so the negation + // rejects it. `driver-sql` follows this answer; `driver-memory` answers + // the opposite for a null-valued field, which is filed on its own. + expect(matched({ $not: { stage: { $notContains: 'w' } } })).toEqual(['1']); + }); + + it('$not of a null predicate', () => { + expect(matched({ $not: { stage: { $null: true } } })).toEqual(['1', '2']); + expect(matched({ $not: { stage: { $null: false } } })).toEqual(['3', '4']); + expect(matched({ $not: { stage: null } })).toEqual(['1', '2']); + expect(matched({ $not: { stage: { $eq: null } } })).toEqual(['1', '2']); + }); + }); + + describe('the boolean identities still hold here too (#5134)', () => { + it('$not: {} matches nothing — NOT TRUE ≡ FALSE', () => { + expect(matched({ $not: {} })).toEqual([]); + }); + + it('$not of an empty $or matches everything', () => { + expect(matched({ $not: { $or: [] } })).toEqual(ALL); + }); + }); + + // ── Where this evaluator and `driver-memory` disagree — pinned, not fixed ── + + describe('known disagreement with driver-memory (NOT ruled on by #5146)', () => { + it('$exists reads "the key is present", so a null value EXISTS', () => { + // `driver-memory` reads `$exists` as "has a value", so it answers the + // opposite for a present-but-null field. `driver-sql` cannot tell the two + // apart at all (a NULL column is a NULL column) and keeps its existing + // `IS NOT NULL` compilation. + expect(ids(NULLED, { $not: { stage: { $exists: true } } })).toEqual([]); + expect(ids(MISSING, { $not: { stage: { $exists: true } } })).toEqual(['3', '4']); + }); + }); +}); diff --git a/packages/plugins/driver-memory/src/memory-matcher-not-null-safe.test.ts b/packages/plugins/driver-memory/src/memory-matcher-not-null-safe.test.ts new file mode 100644 index 0000000000..3b10cf18b5 --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-matcher-not-null-safe.test.ts @@ -0,0 +1,157 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5146] `$not` and absent values — the answers this matcher gives, pinned. + * + * This backend needed no change for #5146: it already evaluates a negation in + * ordinary two-valued JS, so `{ $not: { stage: 'won' } }` matches a record whose + * `stage` is null or missing (`undefined !== 'won'`). `driver-sql` used to + * DISAGREE — SQL's `NOT (stage = 'won')` is UNKNOWN for a NULL column and a + * `WHERE` drops it — which meant one CEL `!expr` permission rule admitted a + * different set of rows depending on which driver ran it. #5146 ruled this + * backend's answer canonical (it is the 2:1 majority with `formula`) and + * `driver-sql` was rewritten to match. + * + * So these cases are a PIN, not a change: they are the reference the SQL + * compiler was aligned to, and `sql-driver-not-null-safe.test.ts` asserts the + * same ids over the same fixture. Changing an expectation here silently + * re-opens the divergence — the point of writing them down is that the next + * edit has to move both files, deliberately. + * + * Where this matcher and `formula`'s `matchesFilterCondition` disagree, the case + * says so and pins what each actually answers rather than pretending to a + * consensus; those disagreements are filed separately and are NOT what #5146 + * ruled on. + * + * Home for these eventually: `FILTER_LOGIC_CASES` in `@objectstack/spec/data`, + * so all five backends are held to one table (spec lane, with #5239). + */ + +import { describe, it, expect } from 'vitest'; +import { match } from './memory-matcher.js'; + +/** Fields present but null — how a SQL NULL round-trips into a record. */ +const NULLED: Array> = [ + { 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 }, +]; + +/** The same rows with the null fields ABSENT — the shape a partial write leaves. */ +const MISSING: Array> = [ + { id: '1', stage: 'won', owner: 'u1', amount: 10 }, + { id: '2', stage: 'lost', owner: 'u2', amount: 20 }, + { id: '3', owner: 'u1' }, + { id: '4', amount: 40 }, +]; + +const ALL = ['1', '2', '3', '4']; + +const ids = (rows: Array>, filter: unknown): string[] => + rows.filter((r) => match(r, filter)).map((r) => String(r.id)); + +/** Both readings of "no value" must give the same answer unless noted. */ +const matched = (filter: unknown): string[] => { + const nulled = ids(NULLED, filter); + expect(ids(MISSING, filter), 'a null field and an absent field must match alike').toEqual(nulled); + return nulled; +}; + +describe('[#5146] memory-matcher — $not over records with no value', () => { + describe('a record with no value does not satisfy the negated condition', () => { + it('$not on an implicit equality matches the value-less records', () => { + expect(matched({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + }); + + it('$not over multiple keys matches a record missing EITHER', () => { + expect(matched({ $not: { stage: 'won', owner: 'u1' } })).toEqual(['2', '3', '4']); + }); + + it('the RLS shape: a CEL `!(stage == "won")` scope keeps stage-less records', () => { + expect(matched({ $not: { stage: 'won' } })).toHaveLength(3); + }); + }); + + describe('nesting', () => { + it('$not of a $or rejects a value-less record whose OTHER branch matches', () => { + // Record 3 has no stage but owner = 'u1', so the $or holds and the + // negation must reject it. This is the case that forced `driver-sql` to + // compile its NULL guard onto each leaf instead of beside the `NOT`. + expect(matched({ $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '4']); + }); + + it('$not of a $and matches every record failing either conjunct', () => { + expect(matched({ $not: { $and: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '3', '4']); + }); + + it('a double negation is the positive filter again', () => { + expect(matched({ $not: { $not: { stage: 'won' } } })).toEqual(['1']); + expect(matched({ $not: { $not: { stage: 'won' } } })).toEqual(matched({ stage: 'won' })); + }); + + it('$not ANDs with its sibling keys', () => { + expect(matched({ $not: { stage: 'won' }, owner: 'u1' })).toEqual(['3']); + }); + }); + + describe('operator polarity — a negation is not a blanket "and also the empty ones"', () => { + it('$not of $ne still means "the field IS that value"', () => { + expect(matched({ $not: { stage: { $ne: 'won' } } })).toEqual(['1']); + }); + + it('$not of $in matches the value-less records', () => { + expect(matched({ $not: { stage: { $in: ['won'] } } })).toEqual(['2', '3', '4']); + }); + + it('$not of an ordering comparison matches the value-less records', () => { + expect(matched({ $not: { amount: { $gt: 15 } } })).toEqual(['1', '3']); + }); + + it('$not of $contains matches the value-less records', () => { + expect(matched({ $not: { stage: { $contains: 'w' } } })).toEqual(['2', '3', '4']); + }); + + it('$not of a null predicate', () => { + expect(matched({ $not: { stage: { $null: true } } })).toEqual(['1', '2']); + expect(matched({ $not: { stage: { $null: false } } })).toEqual(['3', '4']); + }); + }); + + describe('the boolean identities still hold here too (#5134)', () => { + it('$not: {} matches nothing — NOT TRUE ≡ FALSE', () => { + expect(matched({ $not: {} })).toEqual([]); + }); + + it('$not of an empty $or matches everything', () => { + expect(matched({ $not: { $or: [] } })).toEqual(ALL); + }); + }); + + // ── Where this matcher and `formula` disagree — pinned, not harmonised ───── + + describe('known disagreements with formula.matchesFilterCondition (NOT ruled on by #5146)', () => { + it('$nin: an ABSENT field is treated differently from a null one', () => { + // The early `value === undefined` guard in `checkCondition` exempts only + // `$exists` / `$ne` / `$null`, so an absent field fails `$nin` outright + // while a null field passes it. `formula` answers "not among" for both. + // Pinned as measured; the ruling belongs to the issue that records it. + expect(ids(NULLED, { $not: { stage: { $nin: ['won'] } } })).toEqual(['1']); + expect(ids(MISSING, { $not: { stage: { $nin: ['won'] } } })).toEqual(['1', '3', '4']); + }); + + it('$notContains: a value-less field does NOT satisfy it here', () => { + // `typeof null !== 'string'` → false, so the negation matches. `formula` + // answers true for the same record, and `driver-sql` follows `formula`. + expect(matched({ $not: { stage: { $notContains: 'w' } } })).toEqual(['1', '3', '4']); + }); + + it('$exists: a present-but-null field counts as NOT existing here', () => { + // `formula` reads `$exists` as "the key is present" (a null value exists); + // this matcher reads it as "has a value". Same answer for an absent field, + // different for a null one. + expect(ids(NULLED, { $not: { stage: { $exists: true } } })).toEqual(['3', '4']); + expect(ids(MISSING, { $not: { stage: { $exists: true } } })).toEqual(['3', '4']); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-not-null-safe.test.ts b/packages/plugins/driver-sql/src/sql-driver-not-null-safe.test.ts new file mode 100644 index 0000000000..711d802f90 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-not-null-safe.test.ts @@ -0,0 +1,276 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5146] `$not` is NULL-safe: a row whose compared column is NULL is returned + * by a negation, exactly as `driver-memory` and `formula` already return it. + * + * # What was wrong + * + * SQL is three-valued. `NULL = 'won'` is UNKNOWN, `NOT UNKNOWN` is UNKNOWN, and + * a `WHERE` keeps only TRUE — so `{ $not: { stage: 'won' } }` compiled to + * `not (stage = 'won')` and silently dropped every row whose `stage` is NULL. + * The other two backends evaluate the same filter in ordinary two-valued JS + * (`undefined !== 'won'` → the row matches) and returned those rows. One + * declared operator, two answers, chosen by which driver happened to run it. + * + * That is not a cosmetic difference. A CEL `!expr` in a permission rule lowers + * to `{ $not: {…} }` (`packages/formula/src/cel-to-filter.ts`), so the SAME read + * scope admitted a different set of rows per backend. #5146 ruled the JS answer + * canonical (it is the 2:1 majority, and nobody writing `!(stage == 'won')` + * expects rows with no stage to be hidden by it). + * + * # What the fix emits + * + * Each leaf INSIDE a `$not` is compiled to a total predicate — never UNKNOWN — + * before the negation is applied: `NOT (stage IS NOT NULL AND stage = 'won')`. + * For the flat shape that is exactly the `NOT (…) OR col IS NULL` the issue + * asks for; pushing the guard down to the leaf is what keeps it correct when the + * operand nests (see the `$or` case below, where a hoisted guard would re-admit + * row 3) and lets the direction of the guard follow each operator's own answer + * for a missing value (`$ne` / `$nin` are NOT widened). + * + * Nothing outside a `$not` changes: an ordinary comparison compiles to the same + * SQL it always did, so no plain predicate loses an index to this. + * + * These expectations are duplicated by hand in the two JS backends' + * `*-not-null-safe.test.ts`. They belong in `FILTER_LOGIC_CASES` + * (`@objectstack/spec/data`) so every backend is held to them at once — that + * table is being extended under #5239 / the spec lane of #5146, which lands + * with driver-mongodb; until then these three files are the pin. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import type { FilterCondition } from '@objectstack/spec/data'; + +/** + * Rows 3 and 4 are the point: `stage` is NULL in both, and row 3 additionally + * carries a NULL `amount` while row 4 carries a NULL `owner`, so a guard that is + * applied to the wrong column shows up as a wrong id rather than 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']; + +describe('[#5146] SqlDriver compiles $not NULL-safely', () => { + let driver: SqlDriver; + let knex: any; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + knex = (driver as any).knex; + await knex.schema.createTable('deal', (t: any) => { + t.string('id').primary(); + t.string('stage'); + t.string('owner'); + t.float('amount'); + }); + await knex('deal').insert(FIXTURE); + }); + + afterEach(async () => { + await knex.destroy(); + }); + + const ids = async (where: unknown): Promise => { + const rows = await driver.find('deal', { + object: 'deal', + fields: ['id'], + where: where as FilterCondition, + }); + return rows.map((r: any) => String(r.id)).sort(); + }; + + /** The SQL this filter compiles to — the same assertion style as #3912's. */ + const sqlFor = (where: unknown): string => { + const qb = knex('deal').select('id'); + (driver as any).applyFilterCondition(qb, where, 'and', 'deal'); + return qb.toString(); + }; + + // ── The headline: a NULL column no longer hides the row from a negation ──── + + describe('a NULL column does not satisfy the negated condition', () => { + it('$not on an implicit equality returns the NULL rows', async () => { + // Was ['2'] — rows 3 and 4 fell into UNKNOWN and disappeared. + expect(await ids({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + }); + + it('the guard rides the leaf, so the emitted SQL is NOT-of-a-total predicate', () => { + const sql = sqlFor({ $not: { stage: 'won' } }); + expect(sql).toContain('`stage` is not null'); + expect(sql).toContain("`stage` = 'won'"); + expect(sql).toMatch(/^select `id` from `deal` where not \(/); + }); + + it('$not over MULTIPLE columns admits a row that is NULL in EITHER', async () => { + // `NOT (a = 1 AND b = 2)` is UNKNOWN when either column is NULL, so both + // row 3 (NULL stage) and row 4 (NULL stage AND NULL owner) were dropped. + expect(await ids({ $not: { stage: 'won', owner: 'u1' } })).toEqual(['2', '3', '4']); + const sql = sqlFor({ $not: { stage: 'won', owner: 'u1' } }); + expect(sql).toContain('`stage` is not null'); + expect(sql).toContain('`owner` is not null'); + }); + + it('the RLS shape: a CEL `!(stage == "won")` scope stops hiding stage-less rows', async () => { + // What `cel-to-filter.ts` lowers `!(stage == 'won')` to. Before this fix + // the same rule showed 1 row on a SQL datasource and 3 on the in-memory + // one — the visible-set divergence #5146 reports. + expect(await ids({ $not: { stage: 'won' } })).toHaveLength(3); + }); + }); + + // ── Nesting: why the guard cannot be hoisted next to the NOT ─────────────── + + describe('the guard composes through nested combinators', () => { + it('$not of a $or excludes a NULL row whose OTHER branch matches', async () => { + // Row 3 has a NULL stage but owner = 'u1', so the inner $or IS satisfied + // and the negation must reject it. A `NOT (…) OR stage IS NULL` hoisted to + // the top would hand row 3 back — a widening, and the exact reason the + // guard is compiled onto each leaf instead. + expect(await ids({ $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '4']); + }); + + it('$not of a $and admits every row that fails either conjunct', async () => { + expect(await ids({ $not: { $and: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '3', '4']); + }); + + it('a double negation is the identity again, NULL rows included', async () => { + // `NOT NOT (stage = 'won')` used to answer UNKNOWN twice over. With total + // leaves it collapses to the positive filter — including for NULL rows, + // which must be EXCLUDED here. + 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']); + }); + }); + + // ── Polarity: the guard follows the operator, it is never blanket ────────── + + describe('each operator is guarded in the direction it answers for a missing value', () => { + it('$not of $ne is NOT widened — it still means "the column IS that value"', async () => { + // `{$not: {stage: {$ne: 'won'}}}` ≡ `{stage: 'won'}`. A blanket + // `OR stage IS NULL` would have handed back rows 3 and 4, i.e. rows the + // filter excludes — the silent widening class of #2704 / #5134. + expect(await ids({ $not: { stage: { $ne: 'won' } } })).toEqual(['1']); + expect(sqlFor({ $not: { stage: { $ne: 'won' } } })).toContain('`stage` is null'); + }); + + it('$not of $nin is not widened either', async () => { + expect(await ids({ $not: { stage: { $nin: ['won'] } } })).toEqual(['1']); + }); + + it('$not of $in returns the NULL rows', async () => { + expect(await ids({ $not: { stage: { $in: ['won'] } } })).toEqual(['2', '3', '4']); + }); + + it('$not of an ordering comparison returns the NULL rows', async () => { + // Row 3's amount is NULL: `NOT (amount > 15)` was UNKNOWN, now TRUE. + expect(await ids({ $not: { amount: { $gt: 15 } } })).toEqual(['1', '3']); + }); + + it('$not of $contains returns the NULL rows', async () => { + expect(await ids({ $not: { stage: { $contains: 'w' } } })).toEqual(['2', '3', '4']); + }); + + it('$not of $notContains keeps this driver\'s existing answer', async () => { + // The one operator where the two JS backends disagree on a null-valued + // field (`driver-memory` says a NULL does not satisfy `$notContains`, + // `formula` says it does). The rewrite follows `formula`, which is what + // this driver already answered — so nothing here is decided by accident; + // the disagreement is filed on its own. + 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 added around a predicate that can never be UNKNOWN. + expect(sqlFor({ $not: { stage: { $null: true } } })).toBe( + 'select `id` from `deal` where not (`stage` is null)', + ); + }); + + it('$not of an explicit `null` comparand is untouched', async () => { + // `{ stage: null }` and `{ stage: { $eq: null } }` compile to `IS NULL`, + // which is total already. + expect(await ids({ $not: { stage: null } })).toEqual(['1', '2']); + expect(await ids({ $not: { stage: { $eq: null } } })).toEqual(['1', '2']); + expect(await ids({ $not: { stage: { $ne: null } } })).toEqual(['3', '4']); + }); + }); + + // ── Nothing outside `$not` moves ─────────────────────────────────────────── + + describe('only the $not path is rewritten', () => { + it('a plain comparison compiles to exactly the SQL it always did', () => { + expect(sqlFor({ stage: 'won' })).toBe("select `id` from `deal` where `stage` = 'won'"); + expect(sqlFor({ stage: { $ne: 'won' } })).toBe("select `id` from `deal` where `stage` <> 'won'"); + expect(sqlFor({ amount: { $gt: 15 } })).toBe('select `id` from `deal` where `amount` > 15'); + expect(sqlFor({ stage: { $in: ['won'] } })).toBe("select `id` from `deal` where `stage` in ('won')"); + }); + + it('a plain comparison returns the rows it always did', async () => { + expect(await ids({ stage: 'won' })).toEqual(['1']); + // Still SQL semantics outside a negation: `<> 'won'` drops the NULL rows. + // That divergence from the JS backends is real but out of #5146's scope — + // it is filed separately rather than smuggled in here. + expect(await ids({ stage: { $ne: 'won' } })).toEqual(['2']); + expect(await ids({})).toEqual(ALL); + }); + + it('a $or / $and of plain comparisons is unchanged', () => { + expect(sqlFor({ $or: [{ stage: 'won' }, { owner: 'u2' }] })).toBe( + "select `id` from `deal` where ((`stage` = 'won') or (`owner` = 'u2'))", + ); + }); + }); + + // ── #5134 / #5243 boolean identities still hold ─────────────────────────── + + describe('the boolean identities of #5134 are preserved', () => { + it('$not: {} is still FALSE (zero rows), not the whole table', async () => { + expect(await ids({ $not: {} })).toEqual([]); + expect(sqlFor({ $not: {} })).toContain('1 = 0'); + }); + + it('$not of a FALSE group is still TRUE', async () => { + expect(await ids({ $not: { $or: [] } })).toEqual(ALL); + }); + + it('$not of a TRUE group is still FALSE', async () => { + expect(await ids({ $not: { $and: [] } })).toEqual([]); + }); + + it('a non-node $not operand is still refused, not rewritten', async () => { + await expect(ids({ $not: null })).rejects.toThrow(/filter\.\$not/); + await expect(ids({ $not: 'x' })).rejects.toThrow(/filter\.\$not/); + await expect(ids({ $not: [] })).rejects.toThrow(/filter\.\$not/); + }); + + it('a field constrained by zero operators is still not ruled on (#5240)', async () => { + // `{ stage: {} }` compiles to no SQL; guarding it would have turned that + // into a live `IS NULL` and decided #5240 from here. + expect(await ids({ $not: { stage: {} } })).toEqual(ALL); + expect(sqlFor({ $not: { stage: {} } })).toBe('select `id` from `deal`'); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts b/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts index 0386994792..1b17b868de 100644 --- a/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts @@ -101,13 +101,18 @@ describe('SqlDriver — null / empty operators (#2704)', () => { it('$not (CEL `!expr` scope filter) → negated sub-condition, not a bogus "$not" column', async () => { // `!(assignee == 'alice')` → { $not: { assignee: { $eq: 'alice' } } }. - // SQL `NOT (assignee = 'alice')` excludes alice AND is null-safe only for - // the rows it can evaluate — rows 2/4 have null assignee so `NOT (null = 'alice')` - // is UNKNOWN and they are excluded, leaving carol. + // + // [#5146] This case used to expect ['3'] alone: `NOT (assignee = 'alice')` + // is UNKNOWN for the null-assignee rows 2/4, so SQL dropped them while + // `driver-memory` and `formula` returned them — one permission rule, two + // visible sets. `$not` is now compiled over a TOTAL predicate + // (`NOT (assignee IS NOT NULL AND assignee = 'alice')`), so "has no + // assignee" counts as "is not alice", as it always did on the other two + // backends. The unassigned rows are the behaviour change. const rows = await driver.find('tasks', { where: { $not: { assignee: { $eq: 'alice' } } }, } as any); - expect(ids(rows)).toEqual(['3']); + expect(ids(rows)).toEqual(['2', '3', '4']); }); it('unknown $-operator throws instead of a silent equality compare', async () => { diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index ef0cf19fe5..1544e9337a 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -754,6 +754,175 @@ function reduceFilterKey(key: string, value: unknown, path: string): FilterVerdi return 'clause'; } +// ── [#5146] NULL-safe `$not` ───────────────────────────────────────────────── + +/** + * [#5146] What a single field constraint needs so its compiled SQL is TOTAL — + * TRUE or FALSE for every row, never UNKNOWN. + * + * - `'none'` — the predicate is already total (`IS NULL` / `IS NOT NULL`). + * - `'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'; + +/** + * [#5146] Does a NULL column satisfy this one operator, under the semantics the + * JS backends (`driver-memory` `match`, `formula` `matchesFilterCondition`) + * give it? + * + * They evaluate a missing/null field in ordinary two-valued JS: `undefined !== + * 'won'` is simply `true`. This table is that answer, per operator — measured + * against both, not assumed. The default is the large positive-comparison + * family (`$gt`/`$in`/`$contains`/…), every member of which answers `false` for + * a value that is not there. + */ +function nullValueSatisfiesOperator(op: string, value: unknown): boolean { + switch (op) { + // `$eq: null` IS the null predicate; any other comparand is a value test. + case '$eq': return value === null; + case '$ne': return value !== null; + // The emitter reads `$null`/`$exists` by identity against `false`, so the + // guard must read them the same way or the two can disagree. + case '$null': return value !== false; + case '$exists': return value === false; + // Negative-polarity set/substring tests: "not among" / "does not contain" + // hold vacuously for a value that is absent. + case '$nin': return true; + // `$notContains` is the one operator where the two JS backends disagree on a + // null-valued field (`driver-memory` answers false because `typeof null !== + // 'string'`; `formula` answers true). `formula` is followed here because it + // is what this driver already answers for the shape today, so the ruling on + // that disagreement stays where it belongs — the issue that records it — + // instead of being made silently by this rewrite. + case '$notContains': return true; + default: return false; + } +} + +/** [#5146] Is this operator's compiled SQL already total for a NULL column? */ +function operatorIsNullTotal(op: string, value: unknown): boolean { + switch (op) { + // Compile to `IS NULL` / `IS NOT NULL` — two-valued by construction. + case '$null': + case '$exists': + return true; + // A null comparand makes these null PREDICATES too (see the `$eq`/`$ne` + // arms of the emitter below), not comparisons. + case '$eq': + case '$ne': + return value === null; + default: + return false; + } +} + +/** + * [#5146] 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 `IS NULL` — already total. + if (spec === null) return 'none'; + // A scalar / Date / array comparand is an implicit `=`; a NULL column fails it. + if (typeof spec !== 'object' || spec instanceof Date || Array.isArray(spec)) return 'requireValue'; + const entries = Object.entries(spec as Record); + // `{ field: {} }` compiles to no SQL at all. Guarding it would turn a shape + // that emits nothing into a live `IS NULL` predicate — i.e. would RULE on + // #5240 from here. Left exactly as it compiles today. + 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'; +} + +/** + * [#5146] Rewrite the operand of a `$not` so every leaf compiles to a TOTAL + * predicate, which is what makes `NOT (…)` mean the same thing here as it does + * in `driver-memory` / `formula`. + * + * # The defect + * + * SQL is three-valued: `NULL = 'won'` is UNKNOWN, `NOT UNKNOWN` is still + * UNKNOWN, and a `WHERE` keeps only TRUE — so `{ $not: { stage: 'won' } }` + * dropped every row whose `stage` is NULL. The JS backends evaluate the same + * filter in two-valued logic (`undefined !== 'won'` → the row matches), so ONE + * declared operator gave two different answers depending on which driver ran + * it. On a CEL `!expr` read scope lowered by `cel-to-filter.ts` that is not a + * count that differs — it is the SAME permission rule admitting a different + * set of rows per backend. Ruled NULL-safe in #5146: "the column has no value" + * counts as NOT satisfying the negated condition, matching the 2:1 majority. + * + * # Why the guard is pushed to the LEAF, not hung off the `NOT` + * + * The issue states the fix as `NOT (…) OR col IS NULL`, and for the flat shape + * that motivates it the two are identical — `NOT (a IS NOT NULL AND a = 'won')` + * is `NOT (a = 'won') OR a IS NULL`. They stop being identical as soon as the + * operand nests: hoisting the guard to the top of 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 + * without special cases. + * + * # Why polarity is per operator + * + * A blanket "OR col IS NULL" would also WIDEN the negative-polarity operators: + * `{ $not: { a: { $ne: 5 } } }` means "a is 5", and both JS backends exclude a + * NULL row from it (`null !== 5` holds, so the operand matches, so the negation + * does not). Adding an unconditional null escape there would hand back exactly + * the rows the filter excludes — the silent widening class this driver keeps + * paying for (#2704, #5134). So each leaf is guarded in the direction its own + * operator answers, per {@link nullValueSatisfiesOperator}. + * + * The rewrite only ever runs INSIDE a `$not`; a plain comparison's SQL is + * untouched, so `{ a: 1 }` still compiles to `a = 1` and nothing outside a + * negation changes shape or loses an index. + * + * A nested `$not` is deliberately left alone: its own branch totalises its + * operand, and `NOT ` is itself total, so recursing into it here would + * only 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)) { + out[key] = value.map((element) => nullSafeNegationOperand(element as Record)); + continue; + } + if (key.startsWith('$')) { + // `$not` (handled by its own branch) and anything else `$`-prefixed keep + // whatever this driver does with them today — the rewrite rules on NULL, + // not on the operator vocabulary. + out[key] = value; + continue; + } + const guard = nullGuardForFieldSpec(value); + if (guard === 'none') { + out[key] = value; + } else if (guard === 'requireValue') { + // `col IS NOT NULL AND (…)` — both conjuncts of the enclosing node. + guarded.push({ [key]: { $null: false } }, { [key]: value }); + } else { + // `col IS NULL OR (…)` — one conjunct, so the OR binds tighter than the + // AND the node's keys form. + guarded.push({ $or: [{ [key]: { $null: true } }, { [key]: value }] }); + } + } + if (guarded.length > 0) { + const existing = Array.isArray(out.$and) ? out.$and : []; + out.$and = [...existing, ...guarded]; + } + return out; +} + // ── Introspection Types ────────────────────────────────────────────────────── export interface IntrospectedColumn { @@ -6067,6 +6236,14 @@ export class SqlDriver implements IDataDriver { * `'false'` members of a `$or` are dropped as their identities, and a node * that reduces to `'false'` never reaches the loop at all. So Knex is never * again in a position to silently discard a group. + * + * # NULL-safe negation (#5146) + * + * `$not` negates a predicate that {@link nullSafeNegationOperand} has first + * made TOTAL, because SQL's `NOT UNKNOWN` is UNKNOWN and a `WHERE` drops it — + * which used to hide every row whose compared column was NULL, while + * `driver-memory` and `formula` returned those same rows. Only the `$not` + * path is rewritten; an ordinary comparison compiles exactly as before. */ protected applyFilterCondition(builder: Knex.QueryBuilder, condition: any, logicalOp: 'and' | 'or' = 'and', tableHint?: string | null) { if (!condition || typeof condition !== 'object') return; @@ -6140,9 +6317,17 @@ export class SqlDriver implements IDataDriver { // group is FALSE and never reaches here (the node reduced to FALSE), and // a non-node operand was refused by the reduction, so `value` is a node. if (reduceFilterKey(key, value, 'filter') === 'true') continue; + // #5146 — negate a TOTAL predicate, so a row whose column is NULL gets + // the same answer here as it does in driver-memory / formula instead of + // vanishing into SQL's UNKNOWN. See {@link nullSafeNegationOperand} for + // why the guard sits on each leaf rather than beside the `NOT`, and why + // its direction is per operator. The reduction above ran on the ORIGINAL + // operand; the rewrite preserves every verdict (each guarded conjunct + // still carries a field key, so a `'clause'` stays a `'clause'`). + const negated = nullSafeNegationOperand(value as Record); const notMethod = logicalOp === 'or' ? 'orWhereNot' : 'whereNot'; (builder as any)[notMethod]((qb: any) => { - this.applyFilterCondition(qb, value, 'and', table); + this.applyFilterCondition(qb, negated, 'and', table); }); } else if (typeof value === 'object' && value !== null && !Array.isArray(value)) { const localField = this.mapSortField(key);