Skip to content

Commit 152d7fb

Browse files
os-zhuangclaude
andauthored
fix(service-analytics): a null comparand is a null predicate, not = '' (#5332) (#5525)
`{stage: null}` compiled to `stage IS NULL` while `{stage: {$eq: null}}` — the same predicate — compiled to `stage = $1` binding the empty STRING, because the operator spelling fell through to `MONGO_TO_CUBE_OP` and `stringifyForCube(null)` returned `''`. One meaning, two answers, inside one file. The failure was silent: an "is empty" widget drew zero rows with no error to read, and on a text column — where `''` is a value rows genuinely store — the `$ne` direction excluded exactly the rows "is not empty" was asked to keep. `$eq: null` is not a near-synonym of `$null: true`: driver-mongodb's translator rewrites the latter into the former, so they are one predicate in the contract, and `read-scope-sql.ts`, `driver-sql`, `driver-memory` and `formula` all compile them alike. `fieldLeaves` now emits the same `notSet` / `set` leaves for all three spellings, so both strategies, the engine filter and the display-SQL echo follow with no new cases. The #5146 guard table moves in the SAME commit because it describes this file's emitter: left alone it would have wrapped `stage IS NOT NULL AND stage IS NULL` and negated that always-false conjunction to EVERY row for `{$not: {stage: {$eq: null}}}`. `nullValueSatisfiesOperator` and `operatorIsNullTotal` now carry the `value === null` arms their `read-scope-sql` counterparts have. Scoped 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]`) — and `{$eq: ''}` stays a value comparison, since reading `''` as null is the same defect sign-flipped. Claude-Session: https://claude.ai/code/session_01BWS4heBoAitLmzCLhcYdbK Co-authored-by: Claude <noreply@anthropic.com>
1 parent 95fab22 commit 152d7fb

3 files changed

Lines changed: 324 additions & 7 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(service-analytics): a `null` comparand in an analytics `where` is a null predicate, not `= ''` (#5332)
6+
7+
`{stage: null}` compiled to `stage IS NULL`, while `{stage: {$eq: null}}` — the
8+
same predicate — compiled to `stage = $1` binding the empty **string**. One
9+
meaning had two answers inside one file: the bare-`null` spelling took
10+
`fieldLeaves`' `raw === null` branch, the operator spelling fell through to the
11+
`MONGO_TO_CUBE_OP` map, and `stringifyForCube(null)` handed it `''`.
12+
13+
Measured before the fix, on cube `deals` / column `stage`:
14+
15+
| `where` | WHERE | bindings |
16+
|---|---|---|
17+
| `{stage: null}` | `stage IS NULL` | `[]` |
18+
| `{stage: {$eq: null}}` | `stage = $1` | `['']` |
19+
| `{stage: {$ne: null}}` | `stage != $1` | `['']` |
20+
| `{stage: {$null: true}}` | `stage IS NULL` | `[]` |
21+
22+
The failure was **silent, not loud**: an "is empty" dashboard widget drew zero
23+
rows — never an error — because a real value can never equal a NULL column, and
24+
the author saw "no data" rather than anything to debug. On a text column the
25+
`$ne` direction was worse than empty: in SQLite / MySQL `''` is a value rows
26+
genuinely store, so "stage is not empty" compiled to `stage != ''` and excluded
27+
exactly the rows it was asked to keep, while "stage is empty" returned the one
28+
row that is emphatically not null.
29+
30+
`$eq: null` and `$null: true` are not near-synonyms to be reconciled by taste —
31+
`driver-mongodb`'s translator **rewrites** the latter into the former, so they
32+
are one predicate in the contract, and `read-scope-sql.ts` (this package's other
33+
SQL compiler), `driver-sql`, `driver-memory` and `formula` all compile them
34+
alike. This module was the one dissenting half of one package; `fieldLeaves` now
35+
emits the same `notSet` / `set` leaves for all three spellings, so both
36+
strategies, the ObjectQL engine filter and the `/analytics/sql` display echo
37+
follow with no new cases.
38+
39+
The #5146 NULL-safe `$not` guard table moved in the **same** commit, because it
40+
describes this file's emitter rather than a sibling's: while `$eq: null` was a
41+
value comparison the guard correctly classified it as one, and left alone it
42+
would have wrapped `stage IS NOT NULL AND stage IS NULL` — an always-false
43+
conjunction — and negated it to **every** row for a filter meaning "stage is not
44+
empty". `nullValueSatisfiesOperator` and `operatorIsNullTotal` now carry the
45+
`value === null` arms their `read-scope-sql` counterparts have, and
46+
`{$not: {stage: {$eq: null}}}` returns the rows the other three backends already
47+
return for it.
48+
49+
Scoped deliberately to the two spellings `filter.zod.ts` gives a null *meaning*.
50+
`stringifyForCube`'s `v == null` arm is untouched: it still serves comparand
51+
positions no ruling covers (`$gt: null`, `$in: [null]`), where `''` is a
52+
placeholder rather than an answer. An empty-string comparand also stays a value
53+
comparison — `{stage: {$eq: ''}}` still binds `''` — since reading `''` as null
54+
would be the same defect with its sign flipped.
55+
56+
Authoring is unchanged; only the compiled predicate is. A widget that worked
57+
around the old behaviour by filtering on the literal empty string (`{$eq: ''}`)
58+
keeps working and still means the empty string; one that wrote `{$eq: null}` and
59+
saw nothing now gets its rows.

packages/services/service-analytics/src/__tests__/filter-normalizer-not-null-safe.test.ts

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@
5555
* `native-sql-filter-logic-conformance.test.ts`: a native binding is loadable
5656
* only by the exact Node ABI it was built for and aborts the vitest worker on
5757
* CI's Node, taking the file's cases silently with it.
58+
*
59+
* # The FOURTH square, added by #5332
60+
*
61+
* The last block pins what #5325 could not: a `null` COMPARAND. `{stage: null}`
62+
* compiled to `IS NULL` while `{stage: {$eq: null}}` — the same predicate, and
63+
* literally what `driver-mongodb` rewrites `{$null: true}` into — compiled to
64+
* `stage = ''`, so the file's own emitter answered one question two ways.
65+
* Pinning the `$not` row set for it while that held would have frozen the wrong
66+
* answer, which is why the two ids in `'the OPERATOR spellings of a null
67+
* comparand are untouched too'` were deliberately absent until now.
5868
*/
5969

6070
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
@@ -63,6 +73,7 @@ import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contract
6373

6474
import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js';
6575
import { ObjectQLStrategy } from '../strategies/objectql-strategy.js';
76+
import { normalizeAnalyticsFilterTree } from '../strategies/filter-normalizer.js';
6677
import { compileScopedFilterToSql } from '../read-scope-sql.js';
6778

6879
/**
@@ -384,6 +395,30 @@ describe('[#5325] analytics `where` — NULL-safe `$not` and the boolean identit
384395
expect(await ids({ $not: { stage: null } })).toEqual(['1', '2']);
385396
});
386397

398+
it('[#5332] the OPERATOR spellings of a `null` comparand are untouched too', async () => {
399+
// The pin this file deliberately WITHHELD. While `$eq: null` compiled to
400+
// `stage = ''` the guard table read it as an ordinary value comparison, so
401+
// `{$not: {stage: {$eq: null}}}` became `NOT (stage IS NOT NULL AND stage =
402+
// '')` — a negated always-false conjunction, i.e. EVERY row, for a filter
403+
// meaning "stage is not empty" — and `{$not: {stage: {$ne: null}}}` became
404+
// `NOT (stage IS NULL OR stage != '')`, i.e. NO row, for "stage is empty".
405+
// Pinning either then would have frozen the wrong answer, so #5325 left
406+
// both out and filed the comparand as #5332; these are its ids.
407+
//
408+
// Measured, not reasoned — the same two id sets the other three backends
409+
// already assert for these filters on this fixture:
410+
// `read-scope-not-null-safe.test.ts` (this package's other compiler),
411+
// `driver-sql/sql-driver-not-null-safe.test.ts` and
412+
// `formula/matches-filter-not-null-safe.test.ts`.
413+
expect(await ids({ $not: { stage: { $eq: null } } })).toEqual(['1', '2']);
414+
expect(await ids({ $not: { stage: { $ne: null } } })).toEqual(['3', '4']);
415+
// Total already, so NO guard conjunct is wrapped around either — the same
416+
// treatment `{$null: true}` gets two tests up.
417+
expect((await sqlFor({ $not: { stage: { $eq: null } } })).sql).toContain('WHERE NOT (stage IS NULL)');
418+
expect((await sqlFor({ $not: { stage: { $ne: null } } })).sql).toContain('WHERE NOT (stage IS NOT NULL)');
419+
expect((await sqlFor({ $not: { stage: { $eq: null } } })).params).toEqual([]);
420+
});
421+
387422
it('an empty `$in` / `$nin` under a `$not` keeps its constant value', async () => {
388423
// Both are boolean CONSTANTS, so they are total and take no guard — and a
389424
// 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
598633
expect(await ids({})).toEqual(ALL);
599634
});
600635
});
636+
637+
// ── [#5332] A `null` comparand is a null PREDICATE, not the empty string ───
638+
639+
/**
640+
* [#5332] `{stage: {$eq: null}}` compiled to `stage = ''`, `{$ne: null}` to
641+
* `stage != ''`, while `{stage: null}` in the same file compiled to `IS NULL`.
642+
*
643+
* One meaning, two answers, one file. The measured table from the issue is the
644+
* first block below, asserted on the generated SQL and its bindings because
645+
* that is where the divergence lived; the row sets follow.
646+
*
647+
* `{$eq: null}` is not merely a near-synonym of `{$null: true}`:
648+
* `driver-mongodb`'s translator REWRITES the latter into the former
649+
* (`mongodb-filter.ts`'s `$null` arm), so the two are one predicate in the
650+
* contract, and `read-scope-sql.ts`, `driver-sql`, `driver-memory` and
651+
* `formula` all compile them alike. This module was the one dissenting half of
652+
* one package.
653+
*/
654+
describe('[#5332] `$eq: null` / `$ne: null` are `IS NULL` / `IS NOT NULL`', () => {
655+
it("the issue's measured table: all four spellings, SQL and bindings", async () => {
656+
// | `where` | was | now |
657+
// |--------------------------|------------------|------------------|
658+
// | `{stage: null}` | `IS NULL` ✅ | unchanged |
659+
// | `{stage: {$eq: null}}` | `= $1` / `['']` | `IS NULL` |
660+
// | `{stage: {$ne: null}}` | `!= $1` / `['']` | `IS NOT NULL` |
661+
// | `{stage: {$null: true}}` | `IS NULL` ✅ | unchanged |
662+
const bare = await sqlFor({ stage: null });
663+
expect(bare.sql).toContain('WHERE stage IS NULL');
664+
expect(bare.params).toEqual([]);
665+
666+
const eqNull = await sqlFor({ stage: { $eq: null } });
667+
expect(eqNull.sql).toContain('WHERE stage IS NULL');
668+
expect(eqNull.params).toEqual([]);
669+
670+
const neNull = await sqlFor({ stage: { $ne: null } });
671+
expect(neNull.sql).toContain('WHERE stage IS NOT NULL');
672+
expect(neNull.params).toEqual([]);
673+
674+
const nullTrue = await sqlFor({ stage: { $null: true } });
675+
expect(nullTrue.sql).toContain('WHERE stage IS NULL');
676+
expect(nullTrue.params).toEqual([]);
677+
});
678+
679+
it('the row sets agree with the other three spellings', async () => {
680+
// Was `[]` for `$eq: null` — an "is empty" widget drew NOTHING, with no
681+
// error to read, because `stage = ''` cannot match a NULL column.
682+
expect(await ids({ stage: { $eq: null } })).toEqual(['3', '4']);
683+
expect(await ids({ stage: { $ne: null } })).toEqual(['1', '2']);
684+
// The four spellings of one predicate, row for row.
685+
expect(await ids({ stage: { $eq: null } })).toEqual(await ids({ stage: null }));
686+
expect(await ids({ stage: { $eq: null } })).toEqual(await ids({ stage: { $null: true } }));
687+
expect(await ids({ stage: { $ne: null } })).toEqual(await ids({ stage: { $null: false } }));
688+
expect(await ids({ stage: { $ne: null } })).toEqual(await ids({ stage: { $exists: true } }));
689+
});
690+
691+
it('the tree carries the same two leaves the other spellings produce', async () => {
692+
// The emitter, without a database in the way: `notSet` / `set` with EMPTY
693+
// `values`, which is what makes every compiler of this tree — both
694+
// strategies AND the display-SQL echo — answer alike without a third arm.
695+
expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: null } } })).toEqual({
696+
kind: 'leaf', member: 'stage', operator: 'notSet', values: [],
697+
});
698+
expect(normalizeAnalyticsFilterTree({ where: { stage: { $ne: null } } })).toEqual({
699+
kind: 'leaf', member: 'stage', operator: 'set', values: [],
700+
});
701+
expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: null } } }))
702+
.toEqual(normalizeAnalyticsFilterTree({ where: { stage: null } }));
703+
expect(normalizeAnalyticsFilterTree({ where: { stage: { $ne: null } } }))
704+
.toEqual(normalizeAnalyticsFilterTree({ where: { stage: { $null: false } } }));
705+
});
706+
707+
it('an EMPTY STRING comparand is still a value comparison — the fix does not over-reach', async () => {
708+
// The mirror danger. `''` and `null` are different facts, and the defect
709+
// was reading one as the other; conflating them in the other direction
710+
// would be the same mistake with the sign flipped.
711+
const { sql, params } = await sqlFor({ stage: { $eq: '' } });
712+
expect(sql).toContain('WHERE stage = $1');
713+
expect(params).toEqual(['']);
714+
expect(normalizeAnalyticsFilterTree({ where: { stage: { $eq: '' } } })).toEqual({
715+
kind: 'leaf', member: 'stage', operator: 'equals', values: [''],
716+
});
717+
// And a non-null comparand of the same operators is untouched.
718+
expect(await ids({ stage: { $eq: 'won' } })).toEqual(['1']);
719+
expect(await ids({ stage: { $ne: 'won' } })).toEqual(['2']);
720+
});
721+
722+
it('the ObjectQL path hands the engine a null predicate, not `\'\'`', async () => {
723+
expect(await engineIds({ stage: { $eq: null } })).toEqual(['3', '4']);
724+
// `convertFilter` maps `notSet` to a bare `null` — `{stage: null}`, the
725+
// spelling every driver reads as IS NULL. It used to receive
726+
// `{stage: ''}` (via `coerceFilterValueForObjectQL('')`), i.e. the empty
727+
// string compared against stored `null` — never a match on any driver.
728+
expect(lastEngineFilter).toEqual({ stage: null });
729+
expect(await engineIds({ stage: { $ne: null } })).toEqual(['1', '2']);
730+
expect(lastEngineFilter).toEqual({ stage: { $ne: null } });
731+
});
732+
733+
it('the echoed display SQL renders the predicate it executes', async () => {
734+
const echo = async (where: unknown) =>
735+
new ObjectQLStrategy().generateSql(query(where), objectqlCtx);
736+
// Was `stage = $1` / `['']` — an echo that could not reproduce the result
737+
// it was shown next to.
738+
const eqNull = await echo({ stage: { $eq: null } });
739+
expect(eqNull.sql).toContain('IS NULL');
740+
expect(eqNull.params).toEqual([]);
741+
const neNull = await echo({ stage: { $ne: null } });
742+
expect(neNull.sql).toContain('IS NOT NULL');
743+
expect(neNull.params).toEqual([]);
744+
});
745+
746+
it('on a TEXT column the `$ne` direction stops excluding the `\'\'` rows', async () => {
747+
// The issue's severity argument, executed. In SQLite / MySQL `''` is a
748+
// REAL value a row can store, so `stage != ''` — what `{$ne: null}` used
749+
// to compile to — dropped exactly the rows "stage is not empty" keeps,
750+
// and `stage = ''` matched the one row that is NOT null.
751+
//
752+
// A table of its own because the shared FIXTURE is row-for-row
753+
// `driver-sql`'s and carries no empty string; adding one there would move
754+
// every other file's expectations.
755+
db.run(`CREATE TABLE "deal_text" ("id" TEXT PRIMARY KEY, "stage" TEXT);`);
756+
const insert = db.prepare(`INSERT INTO "deal_text" ("id","stage") VALUES (?,?)`);
757+
for (const r of [['e1', 'won'], ['e2', ''], ['e3', null]]) insert.run(r as any[]);
758+
insert.free();
759+
try {
760+
const textCube = { ...CUBE, name: 'deals_text', sql: 'deal_text' } as unknown as Cube;
761+
const textCtx = {
762+
...nativeCtx,
763+
getCube: (name: string) => (name === 'deals_text' ? textCube : undefined),
764+
} as StrategyContext;
765+
const textIds = async (where: unknown): Promise<string[]> => {
766+
const result = await new NativeSQLStrategy().execute(
767+
{ ...query(where), cube: 'deals_text' } as AnalyticsQuery,
768+
textCtx,
769+
);
770+
return result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y));
771+
};
772+
// `IS NULL` picks the null row, NOT the empty-string one. Was `['e2']` —
773+
// the one row that is emphatically not empty of a value.
774+
expect(await textIds({ stage: { $eq: null } })).toEqual(['e3']);
775+
// `IS NOT NULL` keeps the empty-string row. Was `['e1']`.
776+
expect(await textIds({ stage: { $ne: null } })).toEqual(['e1', 'e2']);
777+
// …and an author who really means the empty string still gets it.
778+
expect(await textIds({ stage: { $eq: '' } })).toEqual(['e2']);
779+
} finally {
780+
db.run(`DROP TABLE "deal_text";`);
781+
}
782+
});
783+
});
601784
});

0 commit comments

Comments
 (0)