diff --git a/.changeset/analytics-null-comparand-predicate.md b/.changeset/analytics-null-comparand-predicate.md new file mode 100644 index 0000000000..1fc31f1459 --- /dev/null +++ b/.changeset/analytics-null-comparand-predicate.md @@ -0,0 +1,59 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): a `null` comparand in an analytics `where` is a null predicate, not `= ''` (#5332) + +`{stage: null}` compiled to `stage IS NULL`, while `{stage: {$eq: null}}` — the +same predicate — compiled to `stage = $1` binding the empty **string**. One +meaning had two answers inside one file: the bare-`null` spelling took +`fieldLeaves`' `raw === null` branch, the operator spelling fell through to the +`MONGO_TO_CUBE_OP` map, and `stringifyForCube(null)` handed it `''`. + +Measured before the fix, on cube `deals` / column `stage`: + +| `where` | WHERE | bindings | +|---|---|---| +| `{stage: null}` | `stage IS NULL` | `[]` | +| `{stage: {$eq: null}}` | `stage = $1` | `['']` | +| `{stage: {$ne: null}}` | `stage != $1` | `['']` | +| `{stage: {$null: true}}` | `stage IS NULL` | `[]` | + +The failure was **silent, not loud**: an "is empty" dashboard widget drew zero +rows — never an error — because a real value can never equal a NULL column, and +the author saw "no data" rather than anything to debug. On a text column the +`$ne` direction was worse than empty: in SQLite / MySQL `''` is a value rows +genuinely store, so "stage is not empty" compiled to `stage != ''` and excluded +exactly the rows it was asked to keep, while "stage is empty" returned the one +row that is emphatically not null. + +`$eq: null` and `$null: true` are not near-synonyms to be reconciled by taste — +`driver-mongodb`'s translator **rewrites** the latter into the former, so they +are one predicate in the contract, and `read-scope-sql.ts` (this package's other +SQL compiler), `driver-sql`, `driver-memory` and `formula` all compile them +alike. This module was the one dissenting half of one package; `fieldLeaves` now +emits the same `notSet` / `set` leaves for all three spellings, so both +strategies, the ObjectQL engine filter and the `/analytics/sql` display echo +follow with no new cases. + +The #5146 NULL-safe `$not` guard table moved in the **same** commit, because it +describes this file's emitter rather than a sibling's: while `$eq: null` was a +value comparison the guard correctly classified it as one, and left alone it +would have wrapped `stage IS NOT NULL AND stage IS NULL` — an always-false +conjunction — and negated it to **every** row for a filter meaning "stage is not +empty". `nullValueSatisfiesOperator` and `operatorIsNullTotal` now carry the +`value === null` arms their `read-scope-sql` counterparts have, and +`{$not: {stage: {$eq: null}}}` returns the rows the other three backends already +return for it. + +Scoped deliberately to the two spellings `filter.zod.ts` gives a null *meaning*. +`stringifyForCube`'s `v == null` arm is untouched: it still serves comparand +positions no ruling covers (`$gt: null`, `$in: [null]`), where `''` is a +placeholder rather than an answer. An empty-string comparand also stays a value +comparison — `{stage: {$eq: ''}}` still binds `''` — since reading `''` as null +would be the same defect with its sign flipped. + +Authoring is unchanged; only the compiled predicate is. A widget that worked +around the old behaviour by filtering on the literal empty string (`{$eq: ''}`) +keeps working and still means the empty string; one that wrote `{$eq: null}` and +saw nothing now gets its rows. 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 index 70488b115f..3f2fa0dfaa 100644 --- 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 @@ -55,6 +55,16 @@ * `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. + * + * # The FOURTH square, added by #5332 + * + * The last block pins what #5325 could not: a `null` COMPARAND. `{stage: null}` + * compiled to `IS NULL` while `{stage: {$eq: null}}` — the same predicate, and + * literally what `driver-mongodb` rewrites `{$null: true}` into — compiled to + * `stage = ''`, so the file's own emitter answered one question two ways. + * Pinning the `$not` row set for it while that held would have frozen the wrong + * answer, which is why the two ids in `'the OPERATOR spellings of a null + * comparand are untouched too'` were deliberately absent until now. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; @@ -63,6 +73,7 @@ import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contract import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; +import { normalizeAnalyticsFilterTree } from '../strategies/filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; /** @@ -384,6 +395,30 @@ describe('[#5325] analytics `where` — NULL-safe `$not` and the boolean identit expect(await ids({ $not: { stage: null } })).toEqual(['1', '2']); }); + it('[#5332] the OPERATOR spellings of a `null` comparand are untouched too', async () => { + // The pin this file deliberately WITHHELD. While `$eq: null` compiled to + // `stage = ''` the guard table read it as an ordinary value comparison, so + // `{$not: {stage: {$eq: null}}}` became `NOT (stage IS NOT NULL AND stage = + // '')` — a negated always-false conjunction, i.e. EVERY row, for a filter + // meaning "stage is not empty" — and `{$not: {stage: {$ne: null}}}` became + // `NOT (stage IS NULL OR stage != '')`, i.e. NO row, for "stage is empty". + // Pinning either then would have frozen the wrong answer, so #5325 left + // both out and filed the comparand as #5332; these are its ids. + // + // Measured, not reasoned — the same two id sets the other three backends + // already assert for these filters on this fixture: + // `read-scope-not-null-safe.test.ts` (this package's other compiler), + // `driver-sql/sql-driver-not-null-safe.test.ts` and + // `formula/matches-filter-not-null-safe.test.ts`. + expect(await ids({ $not: { stage: { $eq: null } } })).toEqual(['1', '2']); + expect(await ids({ $not: { stage: { $ne: null } } })).toEqual(['3', '4']); + // Total already, so NO guard conjunct is wrapped around either — the same + // treatment `{$null: true}` gets two tests up. + expect((await sqlFor({ $not: { stage: { $eq: null } } })).sql).toContain('WHERE NOT (stage IS NULL)'); + expect((await sqlFor({ $not: { stage: { $ne: null } } })).sql).toContain('WHERE NOT (stage IS NOT NULL)'); + expect((await sqlFor({ $not: { stage: { $eq: null } } })).params).toEqual([]); + }); + 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. @@ -598,4 +633,152 @@ describe('[#5325] analytics `where` — NULL-safe `$not` and the boolean identit expect(await ids({})).toEqual(ALL); }); }); + + // ── [#5332] A `null` comparand is a null PREDICATE, not the empty string ─── + + /** + * [#5332] `{stage: {$eq: null}}` compiled to `stage = ''`, `{$ne: null}` to + * `stage != ''`, while `{stage: null}` in the same file compiled to `IS NULL`. + * + * One meaning, two answers, one file. The measured table from the issue is the + * first block below, asserted on the generated SQL and its bindings because + * that is where the divergence lived; the row sets follow. + * + * `{$eq: null}` is not merely a near-synonym of `{$null: true}`: + * `driver-mongodb`'s translator REWRITES the latter into the former + * (`mongodb-filter.ts`'s `$null` arm), so the two are one predicate in the + * contract, and `read-scope-sql.ts`, `driver-sql`, `driver-memory` and + * `formula` all compile them alike. This module was the one dissenting half of + * one package. + */ + describe('[#5332] `$eq: null` / `$ne: null` are `IS NULL` / `IS NOT NULL`', () => { + it("the issue's measured table: all four spellings, SQL and bindings", async () => { + // | `where` | was | now | + // |--------------------------|------------------|------------------| + // | `{stage: null}` | `IS NULL` ✅ | unchanged | + // | `{stage: {$eq: null}}` | `= $1` / `['']` | `IS NULL` | + // | `{stage: {$ne: null}}` | `!= $1` / `['']` | `IS NOT NULL` | + // | `{stage: {$null: true}}` | `IS NULL` ✅ | unchanged | + const bare = await sqlFor({ stage: null }); + expect(bare.sql).toContain('WHERE stage IS NULL'); + expect(bare.params).toEqual([]); + + const eqNull = await sqlFor({ stage: { $eq: null } }); + expect(eqNull.sql).toContain('WHERE stage IS NULL'); + expect(eqNull.params).toEqual([]); + + const neNull = await sqlFor({ stage: { $ne: null } }); + expect(neNull.sql).toContain('WHERE stage IS NOT NULL'); + expect(neNull.params).toEqual([]); + + const nullTrue = await sqlFor({ stage: { $null: true } }); + expect(nullTrue.sql).toContain('WHERE stage IS NULL'); + expect(nullTrue.params).toEqual([]); + }); + + it('the row sets agree with the other three spellings', async () => { + // Was `[]` for `$eq: null` — an "is empty" widget drew NOTHING, with no + // error to read, because `stage = ''` cannot match a NULL column. + expect(await ids({ stage: { $eq: null } })).toEqual(['3', '4']); + expect(await ids({ stage: { $ne: null } })).toEqual(['1', '2']); + // The four spellings of one predicate, row for row. + expect(await ids({ stage: { $eq: null } })).toEqual(await ids({ stage: null })); + expect(await ids({ stage: { $eq: null } })).toEqual(await ids({ stage: { $null: true } })); + expect(await ids({ stage: { $ne: null } })).toEqual(await ids({ stage: { $null: false } })); + expect(await ids({ stage: { $ne: null } })).toEqual(await ids({ stage: { $exists: true } })); + }); + + it('the tree carries the same two leaves the other spellings produce', async () => { + // The emitter, without a database in the way: `notSet` / `set` with EMPTY + // `values`, which is what makes every compiler of this tree — both + // strategies AND the display-SQL echo — answer alike without a third arm. + expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: null } } })).toEqual({ + kind: 'leaf', member: 'stage', operator: 'notSet', values: [], + }); + expect(normalizeAnalyticsFilterTree({ where: { stage: { $ne: null } } })).toEqual({ + kind: 'leaf', member: 'stage', operator: 'set', values: [], + }); + expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: null } } })) + .toEqual(normalizeAnalyticsFilterTree({ where: { stage: null } })); + expect(normalizeAnalyticsFilterTree({ where: { stage: { $ne: null } } })) + .toEqual(normalizeAnalyticsFilterTree({ where: { stage: { $null: false } } })); + }); + + it('an EMPTY STRING comparand is still a value comparison — the fix does not over-reach', async () => { + // The mirror danger. `''` and `null` are different facts, and the defect + // was reading one as the other; conflating them in the other direction + // would be the same mistake with the sign flipped. + const { sql, params } = await sqlFor({ stage: { $eq: '' } }); + expect(sql).toContain('WHERE stage = $1'); + expect(params).toEqual(['']); + expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: '' } } })).toEqual({ + kind: 'leaf', member: 'stage', operator: 'equals', values: [''], + }); + // And a non-null comparand of the same operators is untouched. + expect(await ids({ stage: { $eq: 'won' } })).toEqual(['1']); + expect(await ids({ stage: { $ne: 'won' } })).toEqual(['2']); + }); + + it('the ObjectQL path hands the engine a null predicate, not `\'\'`', async () => { + expect(await engineIds({ stage: { $eq: null } })).toEqual(['3', '4']); + // `convertFilter` maps `notSet` to a bare `null` — `{stage: null}`, the + // spelling every driver reads as IS NULL. It used to receive + // `{stage: ''}` (via `coerceFilterValueForObjectQL('')`), i.e. the empty + // string compared against stored `null` — never a match on any driver. + expect(lastEngineFilter).toEqual({ stage: null }); + expect(await engineIds({ stage: { $ne: null } })).toEqual(['1', '2']); + expect(lastEngineFilter).toEqual({ stage: { $ne: null } }); + }); + + it('the echoed display SQL renders the predicate it executes', async () => { + const echo = async (where: unknown) => + new ObjectQLStrategy().generateSql(query(where), objectqlCtx); + // Was `stage = $1` / `['']` — an echo that could not reproduce the result + // it was shown next to. + const eqNull = await echo({ stage: { $eq: null } }); + expect(eqNull.sql).toContain('IS NULL'); + expect(eqNull.params).toEqual([]); + const neNull = await echo({ stage: { $ne: null } }); + expect(neNull.sql).toContain('IS NOT NULL'); + expect(neNull.params).toEqual([]); + }); + + it('on a TEXT column the `$ne` direction stops excluding the `\'\'` rows', async () => { + // The issue's severity argument, executed. In SQLite / MySQL `''` is a + // REAL value a row can store, so `stage != ''` — what `{$ne: null}` used + // to compile to — dropped exactly the rows "stage is not empty" keeps, + // and `stage = ''` matched the one row that is NOT null. + // + // A table of its own because the shared FIXTURE is row-for-row + // `driver-sql`'s and carries no empty string; adding one there would move + // every other file's expectations. + db.run(`CREATE TABLE "deal_text" ("id" TEXT PRIMARY KEY, "stage" TEXT);`); + const insert = db.prepare(`INSERT INTO "deal_text" ("id","stage") VALUES (?,?)`); + for (const r of [['e1', 'won'], ['e2', ''], ['e3', null]]) insert.run(r as any[]); + insert.free(); + try { + const textCube = { ...CUBE, name: 'deals_text', sql: 'deal_text' } as unknown as Cube; + const textCtx = { + ...nativeCtx, + getCube: (name: string) => (name === 'deals_text' ? textCube : undefined), + } as StrategyContext; + const textIds = async (where: unknown): Promise => { + const result = await new NativeSQLStrategy().execute( + { ...query(where), cube: 'deals_text' } as AnalyticsQuery, + textCtx, + ); + return result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + }; + // `IS NULL` picks the null row, NOT the empty-string one. Was `['e2']` — + // the one row that is emphatically not empty of a value. + expect(await textIds({ stage: { $eq: null } })).toEqual(['e3']); + // `IS NOT NULL` keeps the empty-string row. Was `['e1']`. + expect(await textIds({ stage: { $ne: null } })).toEqual(['e1', 'e2']); + // …and an author who really means the empty string still gets it. + expect(await textIds({ stage: { $eq: '' } })).toEqual(['e2']); + } finally { + db.run(`DROP TABLE "deal_text";`); + } + }); + }); }); diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index 1506449a6b..1b358aeb67 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -94,6 +94,25 @@ * `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. * + * # A `null` COMPARAND is a null predicate, not a value (#5332) + * + * `{stage: null}` compiled to `IS NULL` while `{stage: {$eq: null}}` compiled to + * `stage = ''` — one meaning, two answers, inside this one file. The cause was + * that the `$eq` / `$ne` pair fell through to {@link MONGO_TO_CUBE_OP} like any + * other comparison and `stringifyForCube(null)` handed it the empty STRING, so a + * "stage is empty" widget compared a real value against columns that are NULL + * and charted zero rows — silently, with nothing for the author to read. On a + * text column the `$ne` direction was worse than empty: `''` is a value rows + * genuinely store, so "stage is not empty" EXCLUDED exactly the rows it was asked + * to keep. + * + * The pair is not merely similar to `{$null: true|false}` — `driver-mongodb` + * TRANSLATES `$null` into it — so {@link fieldLeaves} now emits the same + * `notSet` / `set` leaves for all three spellings, and the #5146 guard table + * moved in the same commit (see {@link nullValueSatisfiesOperator}); a guard that + * still described the old emitter would have negated an always-false conjunction + * and answered `{$not: {stage: {$eq: null}}}` with every row. + * * # A `where` ARRAY is lowered here, not dropped (#5334) * * `FilterArray` — `['stage', '=', 'won']`, `['and', […], […]]`, `[[…], […]]` — @@ -210,6 +229,16 @@ const MONGO_TO_CUBE_OP: Record = { * `'1'`/`'0'` was indistinguishable from a numeric 1/0 and made every boolean * equality filter / boolean group-by compare a number against a boolean — and * never match. + * + * The `v == null → ''` arm is NOT a spelling of "is null": `values` is + * `string[]`, which has no null, so every leaf that MEANS null is emitted as + * `notSet` / `set` with EMPTY `values` and never calls this function — the + * `raw === null` branch, `$null` / `$exists`, and since #5332 a `null` comparand + * of `$eq` / `$ne`, which used to arrive here and become `= ''`. What still + * reaches this arm is a comparand position no ruling covers (`$gt: null`, + * `$in: [null]`), where `''` is a placeholder rather than an answer; #5332 scoped + * itself to the two spellings `filter.zod.ts` gives a null MEANING and left this + * arm untouched. */ function stringifyForCube(v: unknown): string { if (v == null) return ''; @@ -372,6 +401,34 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { continue; } + // [#5332] A `null` COMPARAND is a null PREDICATE, not a value + // comparison: `$eq: null` is `IS NULL` (`notSet`) and `$ne: null` is + // `IS NOT NULL` (`set`) — the same two leaves the `raw === null` branch + // above and `{$null: true|false}` beside it already produce. `$eq: null` + // and `$null: true` are not merely similar spellings; `driver-mongodb` + // TRANSLATES the latter into the former (`mongodb-filter.ts`'s `$null` + // arm), so they are one predicate in the contract, and + // `read-scope-sql.ts`'s `compileOperator`, `driver-sql`, `driver-memory` + // and `formula` all compile them alike. + // + // Without this branch the pair fell through to MONGO_TO_CUBE_OP and + // `stringifyForCube(null)` → `''`, i.e. `stage = ''` / `stage != ''` + // (#5332). One meaning had two answers inside ONE file, and the wrong + // one bound a real value a NULL column can never equal: an "is empty" + // widget charted ZERO rows with no error to read, and on a text column — + // where `''` is a value rows genuinely store — `$ne: null` additionally + // EXCLUDED the empty-string rows it was asked to keep. + // + // Identity against `null`, matching `read-scope-sql` and `driver-sql`: + // `null` is what an authored `FilterCondition` can carry (JSON has no + // `undefined`, and `$eq: undefined` is a key the author did not mean to + // write). `stringifyForCube`'s wider `v == null` test is untouched — it + // still serves the comparand positions this branch does not claim. + if ((opKey === '$eq' || opKey === '$ne') && wrapper[opKey] === null) { + leaf(opKey === '$eq' ? 'notSet' : 'set', []); + 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 @@ -560,12 +617,16 @@ type NullGuard = 'none' | 'requireValue' | 'allowNull'; * 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. + * + * `$eq` / `$ne` DO carry `read-scope-sql`'s `value === null` arms — since #5332, + * and only since then. While {@link fieldLeaves} stringified a `null` comparand + * to `''`, these two arms had to describe THAT emitter: `{$eq: null}` was an + * ordinary value comparison here, the guard said so, and the TSDoc recorded the + * `''` comparand as a separate defect deliberately left undecided. #5332 decided + * it — the emitter now compiles the pair to `notSet` / `set` — so the arms moved + * with it, in the same commit. The invariant is not "copy the sibling table", it + * is "each guard matches its OWN emitter"; the two tables agreeing again is the + * consequence of the emitters agreeing, not the reason for the edit. * * The default is the large positive-comparison family (`$gt` / `$in` / * `$contains` / …), every member of which answers `false` for a value that is @@ -575,7 +636,13 @@ type NullGuard = 'none' | 'requireValue' | 'allowNull'; */ function nullValueSatisfiesOperator(op: string, value: unknown): boolean { switch (op) { - case '$ne': return true; + // [#5332] `$eq: null` IS the null predicate — a NULL column satisfies it, + // and no other comparand does. + case '$eq': return value === null; + // Mirror image: `$ne: null` compiles to `set` (`IS NOT NULL`), which a NULL + // column FAILS. Any other comparand is the two-valued JS `!==`, which an + // absent value passes — the arm this used to be for every comparand. + case '$ne': return value !== null; case '$null': return value === true; case '$exists': return value === false; // Negative-polarity set / substring tests hold vacuously for an absent value. @@ -597,6 +664,14 @@ function operatorIsNullTotal(op: string, value: unknown): boolean { case '$null': case '$exists': return true; + // [#5332] A `null` comparand makes these null PREDICATES too — `notSet` / + // `set`, not comparisons — so they are total by construction and take NO + // guard. Left out, `{$not: {stage: {$eq: null}}}` wrapped `stage IS NOT NULL + // AND stage IS NULL` (an always-false conjunction) and negated it to EVERY + // row, for a filter meaning "stage is not empty". + case '$eq': + case '$ne': + return value === null; // 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.