Skip to content

Commit f7df82c

Browse files
os-zhuangclaude
andauthored
fix(driver-memory): the analytics face stops round-tripping comparands through string[] (#5373) (#5431)
`MemoryAnalyticsService` lowered `AnalyticsQuery.where` into a cube-style `{member, operator, values}` list whose `values` was `string[]`, so every comparand made a JS value → string → JS value round trip. That round trip is lossy for anything not already a string, and both of the issue's symptoms are the one root cause: - `stringifyForCube(true)` → `'1'` → `coerceFilterValue('1')` → the NUMBER 1 (the `/^-?\d+$/` arm wins), compared against a stored `true`. mingo compares cross-type as never-equal, so `{is_active: true}` matched zero rows. - `flattenFilterCondition` opened with `if (raw == null) continue`, so `{closed_at: null}` produced no cube entry at all and the predicate vanished. Fewer constraints means MORE rows: the query widened to the full table. This is the #3948 direction and the more dangerous half — a widened chart looks exactly like a working chart. Route B of the issue's three: stop round-tripping. `values` is `unknown[]`; stringification happens only at the `generateSql` exit, where a SQL literal is genuinely needed. A/C were rejected because A layers a tag on an encoding the issue already calls suspect, and C would refuse `{is_active: true, stage: {$nin: ['lost']}}` — `AnalyticsQuerySchema.where`'s own docstring example. B is affordable because the triple is a purely INTERNAL intermediate. Verified rather than assumed before building on it: `normalizeFilters`, `flattenFilterCondition`, `stringifyForCube`, `coerceFilterValue` and `toSqlLiteral` are all private and referenced only in this file; `index.ts` exports only the class; `IAnalyticsService` exposes no such shape; and the API layer actively REJECTS a `{member, operator, values}` array on the wire (`spec/src/api/analytics.test.ts`, `runtime/src/http-dispatcher.test.ts` both assert the rejection). Zero spec bytes touched. The encoding could not simply be made lossless: its own justification for `'1'`/`'0'` ("downstream consumers expecting SQLite-style numeric booleans") is true for the SQL exit and false for the in-memory one, and both exits shared it. Both exits are fixed, because a fix that satisfied mingo while emitting SQL meaning something else would only move the loss. `toSqlLiteral` now takes the real value instead of guessing a type back out of text — a TEXT `'100'` is quoted where it used to emit `code = 100` — and a null comparand becomes `IS NULL` / `IS NOT NULL` rather than SQL's never-true `= NULL`. Temporal comparands still convert, now via the driver's own storage-form rule (new narrow `filterComparandStorageForm`, keyed on the declared field kind, #4047) instead of an ad-hoc `toISOString()`, so a `Date` still meets a declared `datetime` column and no second derivation of that rule appears in this face (#5240). The issue's unverified third symptom is REAL and fixed by the same change: `{code: '100'}` against a TEXT column storing `'100'` round-tripped to the number 100 and matched zero rows. Measured, along with two more of the same root cause the issue did not list — `{is_active: {$ne: true}}` and `{closed_at: {$ne: null}}` each returned the whole table. Measured on the issue's 3-row fixture, analytics vs `find()`: | where | before | after | find() | |---|---|---|---| | `{is_active: true}` | 0 | 2 | 2 | | `{is_active: false}` | 0 | 1 | 1 | | `{closed_at: null}` | 3 | 2 | 2 | | `{code: '100'}` | 0 | 2 | 2 | | `{is_active: {$ne: true}}` | 3 | 1 | 1 | | `{closed_at: {$ne: null}}` | 3 | 1 | 1 | Tests go in the shared conformance file beside the #5324/#5345 shape table rather than a suite of their own. `FILTER_LOGIC_CASES` varies filter SHAPE over an all-string fixture — deliberately, so nothing in it is about coercion — which is exactly why every case stayed green through this defect. The new block varies comparand TYPE and holds the same invariant: agree with `find()`, or refuse. Reverting only the source change fails 11 of the new assertions, across both exits. Out of scope, not fixed here: #5374 (`$notContains` → bare mingo `{$not: 'x'}`), held to follow serially since its call site is the value-compilation point this PR changes. Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0c52202 commit f7df82c

4 files changed

Lines changed: 489 additions & 65 deletions

File tree

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
---
2+
"@objectstack/driver-memory": minor
3+
---
4+
5+
fix(driver-memory): the analytics (cube) face stops round-tripping filter comparands through `string[]`, which was losing booleans, `null` and numeric-looking strings (#5373)
6+
7+
**This is an observable behaviour change on a shipped surface: widgets whose
8+
`where` carries a boolean, a `null`, or a numeric-looking string comparand will
9+
show different — correct — numbers.** Some of them go from zero rows to a real
10+
answer; others go from the whole table down to the rows actually asked for.
11+
12+
## What was happening
13+
14+
`MemoryAnalyticsService` lowers `AnalyticsQuery.where` into a cube-style
15+
`{member, operator, values}` list whose `values` was typed `string[]`, because
16+
the cube WIRE format serialises filter values as strings. So every comparand
17+
made a JS value → string → JS value round trip on its way to the pipeline, and
18+
that round trip is lossy for anything that is not already a string:
19+
20+
| `where` | stringified | recovered as | compared against | rows |
21+
|---|---|---|---|---|
22+
| `{is_active: true}` | `'1'` | the number `1` | stored `true` | **0** |
23+
| `{is_active: false}` | `'0'` | the number `0` | stored `false` | **0** |
24+
| `{closed_at: null}` || *(dropped entirely)* || **the whole table** |
25+
| `{closed_at: {$ne: null}}` | `''` | `''` | stored `null` | **the whole table** |
26+
| `{code: '100'}` (TEXT column) | `'100'` | the number `100` | stored `'100'` | **0** |
27+
| `{is_active: {$ne: true}}` | `'1'` | the number `1` | stored `true`/`false` | **the whole table** |
28+
29+
mingo compares across JS types the way MongoDB compares across BSON types —
30+
never equal — so none of these is an error. Each is a wrong row set, silently.
31+
32+
The two directions fail differently, and the widening one is worse. A boolean
33+
filter that returns nothing renders an empty chart, which someone notices. A
34+
`null` filter that returns everything renders a *normal-looking* chart: a
35+
"closed_at is empty" widget quietly counted the closed records too. That is the
36+
direction #3948 outlawed, and on an RLS read scope it is an unauthorized read
37+
rather than a wrong number.
38+
39+
`{is_active: true, stage: {$nin: ['lost']}}` is `AnalyticsQuerySchema.where`'s
40+
own docstring example. It returned zero rows on this face.
41+
42+
## Why the encoding could not simply be fixed
43+
44+
`stringifyForCube` encoded booleans as `'1'`/`'0'` "so that downstream consumers
45+
expecting SQLite-style numeric booleans match correctly". That justification is
46+
sound for the SQL-generating exit and false for the in-memory one — and both
47+
exits shared the single encoding. There is no string spelling of `true` that is
48+
right for `WHERE is_active = ?` and for a mingo `$eq` against a stored boolean at
49+
the same time, so making the round trip lossless would have meant tagging values
50+
in a format the two exits then have to agree to decode.
51+
52+
So the round trip is **gone** instead. `values` is `unknown[]`; the comparand
53+
stays whatever the author wrote, and each exit converts at its own boundary
54+
where it knows what it needs. This is affordable because the triple is a purely
55+
internal intermediate: `AnalyticsQuery.where` is a `FilterCondition` and nothing
56+
else (#5375 removed the leg that also accepted a cube-style array as input), and
57+
the API layer actively rejects a `{member, operator, values}` array on the wire.
58+
No caller, no spec schema and no serialized form observes its shape — this
59+
change touches zero spec bytes.
60+
61+
## What changes for you
62+
63+
Filters are evaluated against the values you wrote:
64+
65+
- `{is_active: true}` selects the true rows instead of none.
66+
- `{closed_at: null}` selects the null rows instead of every row, and
67+
`{closed_at: {$ne: null}}` selects the complement instead of every row.
68+
- `{code: '100'}` on a TEXT column matches the string `'100'` instead of nothing.
69+
- `{qty: 100}` on a numeric column is unchanged — it was already right.
70+
71+
`generateSql()` is corrected on the same cases, because a fix that satisfied
72+
mingo while emitting SQL meaning something else would only have moved the bug:
73+
74+
- a numeric-looking string is now quoted (`code = '100'`, previously `code = 100`)
75+
while a real number still is not (`qty = 100`);
76+
- a null comparand becomes a nullness test (`closed_at IS NULL` /
77+
`closed_at IS NOT NULL`) rather than the `= NULL` that is never true in SQL,
78+
or — as before this fix — no clause at all;
79+
- booleans keep the SQLite-style `1`/`0` spelling, which was always right for
80+
this half.
81+
82+
Temporal comparands still convert, and now do so through the driver's own
83+
storage-form rule (`filterComparandStorageForm`, keyed on the declared field
84+
kind, #4047) rather than an ad-hoc `toISOString()`. A `Date` against a declared
85+
`datetime` column therefore keeps meeting the canonical UTC ISO text the driver
86+
wrote — a second derivation of that rule inside the analytics face is exactly
87+
the in-package divergence #5240 ruled against.
88+
89+
Nothing else moves: operator vocabulary, the #5345 refusals, `$and` folding,
90+
nested-relation flattening, time dimensions and the empty filter are unchanged.
91+
92+
## Coverage
93+
94+
The cases live in the shared conformance file beside the #5324/#5345 shape
95+
table, not in a suite of their own. `FILTER_LOGIC_CASES` varies the filter's
96+
SHAPE over an all-string fixture — deliberately, so nothing in it is about
97+
coercion — which is why every one of its cases stayed green through this defect.
98+
The new block varies the comparand's TYPE over the fixture measured in the
99+
issue, and holds the same invariant: the analytics face must return the same ids
100+
as `find()`, or refuse. Reverting only the source change fails 11 of the new
101+
assertions, across both exits.

0 commit comments

Comments
 (0)