diff --git a/.changeset/analytics-cube-comparand-roundtrip.md b/.changeset/analytics-cube-comparand-roundtrip.md new file mode 100644 index 0000000000..99c839338c --- /dev/null +++ b/.changeset/analytics-cube-comparand-roundtrip.md @@ -0,0 +1,101 @@ +--- +"@objectstack/driver-memory": minor +--- + +fix(driver-memory): the analytics (cube) face stops round-tripping filter comparands through `string[]`, which was losing booleans, `null` and numeric-looking strings (#5373) + +**This is an observable behaviour change on a shipped surface: widgets whose +`where` carries a boolean, a `null`, or a numeric-looking string comparand will +show different — correct — numbers.** Some of them go from zero rows to a real +answer; others go from the whole table down to the rows actually asked for. + +## What was happening + +`MemoryAnalyticsService` lowers `AnalyticsQuery.where` into a cube-style +`{member, operator, values}` list whose `values` was typed `string[]`, because +the cube WIRE format serialises filter values as strings. So every comparand +made a JS value → string → JS value round trip on its way to the pipeline, and +that round trip is lossy for anything that is not already a string: + +| `where` | stringified | recovered as | compared against | rows | +|---|---|---|---|---| +| `{is_active: true}` | `'1'` | the number `1` | stored `true` | **0** | +| `{is_active: false}` | `'0'` | the number `0` | stored `false` | **0** | +| `{closed_at: null}` | — | *(dropped entirely)* | — | **the whole table** | +| `{closed_at: {$ne: null}}` | `''` | `''` | stored `null` | **the whole table** | +| `{code: '100'}` (TEXT column) | `'100'` | the number `100` | stored `'100'` | **0** | +| `{is_active: {$ne: true}}` | `'1'` | the number `1` | stored `true`/`false` | **the whole table** | + +mingo compares across JS types the way MongoDB compares across BSON types — +never equal — so none of these is an error. Each is a wrong row set, silently. + +The two directions fail differently, and the widening one is worse. A boolean +filter that returns nothing renders an empty chart, which someone notices. A +`null` filter that returns everything renders a *normal-looking* chart: a +"closed_at is empty" widget quietly counted the closed records too. That is the +direction #3948 outlawed, and on an RLS read scope it is an unauthorized read +rather than a wrong number. + +`{is_active: true, stage: {$nin: ['lost']}}` is `AnalyticsQuerySchema.where`'s +own docstring example. It returned zero rows on this face. + +## Why the encoding could not simply be fixed + +`stringifyForCube` encoded booleans as `'1'`/`'0'` "so that downstream consumers +expecting SQLite-style numeric booleans match correctly". That justification is +sound for the SQL-generating exit and false for the in-memory one — and both +exits shared the single encoding. There is no string spelling of `true` that is +right for `WHERE is_active = ?` and for a mingo `$eq` against a stored boolean at +the same time, so making the round trip lossless would have meant tagging values +in a format the two exits then have to agree to decode. + +So the round trip is **gone** instead. `values` is `unknown[]`; the comparand +stays whatever the author wrote, and each exit converts at its own boundary +where it knows what it needs. This is affordable because the triple is a purely +internal intermediate: `AnalyticsQuery.where` is a `FilterCondition` and nothing +else (#5375 removed the leg that also accepted a cube-style array as input), and +the API layer actively rejects a `{member, operator, values}` array on the wire. +No caller, no spec schema and no serialized form observes its shape — this +change touches zero spec bytes. + +## What changes for you + +Filters are evaluated against the values you wrote: + +- `{is_active: true}` selects the true rows instead of none. +- `{closed_at: null}` selects the null rows instead of every row, and + `{closed_at: {$ne: null}}` selects the complement instead of every row. +- `{code: '100'}` on a TEXT column matches the string `'100'` instead of nothing. +- `{qty: 100}` on a numeric column is unchanged — it was already right. + +`generateSql()` is corrected on the same cases, because a fix that satisfied +mingo while emitting SQL meaning something else would only have moved the bug: + +- a numeric-looking string is now quoted (`code = '100'`, previously `code = 100`) + while a real number still is not (`qty = 100`); +- a null comparand becomes a nullness test (`closed_at IS NULL` / + `closed_at IS NOT NULL`) rather than the `= NULL` that is never true in SQL, + or — as before this fix — no clause at all; +- booleans keep the SQLite-style `1`/`0` spelling, which was always right for + this half. + +Temporal comparands still convert, and now do so through the driver's own +storage-form rule (`filterComparandStorageForm`, keyed on the declared field +kind, #4047) rather than an ad-hoc `toISOString()`. A `Date` against a declared +`datetime` column therefore keeps meeting the canonical UTC ISO text the driver +wrote — a second derivation of that rule inside the analytics face is exactly +the in-package divergence #5240 ruled against. + +Nothing else moves: operator vocabulary, the #5345 refusals, `$and` folding, +nested-relation flattening, time dimensions and the empty filter are unchanged. + +## Coverage + +The cases live in the shared conformance file beside the #5324/#5345 shape +table, not in a suite of their own. `FILTER_LOGIC_CASES` varies the filter's +SHAPE over an all-string fixture — deliberately, so nothing in it is about +coercion — which is why every one of its cases stayed green through this defect. +The new block varies the comparand's TYPE over the fixture measured in the +issue, and holds the same invariant: the analytics face must return the same ids +as `find()`, or refuse. Reverting only the source change fails 11 of the new +assertions, across both exits. diff --git a/packages/plugins/driver-memory/src/memory-analytics.ts b/packages/plugins/driver-memory/src/memory-analytics.ts index a9ec1f704d..c56f5f184e 100644 --- a/packages/plugins/driver-memory/src/memory-analytics.ts +++ b/packages/plugins/driver-memory/src/memory-analytics.ts @@ -28,10 +28,11 @@ import { * * A row here means the face ATTEMPTS the operator, not that the predicate it * builds is correct — `$notContains` lowers to a bare mingo `{$not: 'x'}` that - * constrains nothing (#5374), and the comparand round-trip through `string[]` - * loses booleans and `null` (#5373). Both are out of #5345's scope (which ruled - * on operators with NO mapping) and are filed rather than fixed here; do not - * read this list as eleven operators known to work. + * constrains nothing (#5374). That one is out of #5345's scope (which ruled on + * operators with NO mapping) and is filed rather than fixed here; do not read + * this list as eleven operators known to work. The comparand half of that + * caveat is closed: #5373 removed the `string[]` round-trip that lost booleans + * and `null` (see {@link NormalizedCubeFilter}). */ const MONGO_TO_CUBE_OPERATOR: Readonly> = Object.freeze({ $eq: 'equals', @@ -62,6 +63,51 @@ export const ANALYTICS_FILTER_CAPABILITIES: FilterFaceCapabilities = Object.free combinators: new Set(['$and']), }); +/** + * [#5373] One lowered constraint: the private intermediate between + * {@link MemoryAnalyticsService.normalizeFilters} and the two exits that consume + * it (`query()` → a mingo `$match`, `generateSql()` → a SQL literal). + * + * # Why `values` is `unknown[]` and not `string[]` + * + * It was `string[]`, because the cube WIRE format serialises filter values as + * strings. So every comparand made a JS value → string → JS value round trip, + * and that round trip is lossy for everything that is not already a string: + * + * | authored | stringified | recovered | compared against | result | + * |---|---|---|---|---| + * | `true` | `'1'` | `1` (the `/^-?\d+$/` arm wins) | stored `true` | **0 rows** | + * | `null` | `''` | `''` | stored `null` | `$ne` matched everything | + * | `'100'` (text column) | `'100'` | `100` | stored `'100'` | **0 rows** | + * + * mingo compares across JS types the way MongoDB compares across BSON types — + * never equal — so each of those is a wrong row set rather than an error. The + * encoding's own justification (booleans as `'1'`/`'0'`, "so downstream + * consumers expecting SQLite-style numeric booleans match correctly") was true + * for the SQL-generating exit and false for the in-memory one, and both exits + * shared the one encoding. There is no string form that is correct for both. + * + * So the round trip is gone rather than made lossless: the value stays whatever + * the author wrote, and each exit converts at ITS boundary, where it knows what + * it needs — `toSqlLiteral` in `generateSql()`, nothing at all in `query()`. + * + * This is an INTERNAL representation, which is what makes that affordable. + * `AnalyticsQuery.where` is a `FilterCondition` and nothing else (#5375 removed + * the leg that also accepted a cube-style array as input), and the API layer + * actively REJECTS a `{member, operator, values}` array on the wire — so no + * caller, no spec schema and no serialized form observes this triple's shape. + */ +interface NormalizedCubeFilter { + member: string; + operator: string; + /** + * The comparands, as authored. Temporal values are put into the field's + * storage form at the exits ({@link MemoryAnalyticsService.comparandsFor}), + * never here — that rule needs the resolved field path, which only an exit has. + */ + values: unknown[]; +} + /** * Configuration for MemoryAnalyticsService */ @@ -138,11 +184,13 @@ export class MemoryAnalyticsService implements IAnalyticsService { const fieldPath = this.resolveFieldPath(cube, filter.member); if (filter.values && filter.values.length > 0) { - // Coerce each filter value to a sensible runtime type so - // `$eq` against in-memory numeric/boolean records still - // matches. The cube spec serialises values as `string[]`, - // but the in-memory driver compares with strict equality. - const coerced = filter.values.map(v => this.coerceFilterValue(v)); + // [#5373] The comparands as authored, in the storage form of the field + // they are compared against. There is no type recovery step any more, + // because there is no longer a stringification to recover FROM: a + // boolean reaches mingo as a boolean and `null` as `null`, so a + // predicate over `is_active` or `closed_at` selects the same rows + // `find()` selects instead of none / all of them. + const coerced = this.comparandsFor(cube, filter.member, filter.values); if (mongoOp === '$in') { matchStage[fieldPath] = { $in: coerced }; } else if (mongoOp === '$nin') { @@ -405,8 +453,18 @@ export class MemoryAnalyticsService implements IAnalyticsService { const fieldPath = this.resolveFieldPath(cube, filter.member); const sqlOp = this.operatorToSql(filter.operator); if (filter.values && filter.values.length > 0) { - const literal = this.toSqlLiteral(filter.values[0]); - whereClauses.push(`${fieldPath} ${sqlOp} ${literal}`); + const comparand = this.comparandsFor(cube, filter.member, filter.values)[0]; + // [#5373] A null comparand is a NULLNESS test, not a comparison. SQL's + // `= NULL` is never true (and `!= NULL` never true either), so emitting + // one would move the very loss this issue is about from the mingo exit + // to this one: `{closed_at: null}` would compile to a WHERE that + // selects nothing while `query()` selects the two null rows. The two + // exits have to mean the same thing. + if (comparand == null && (filter.operator === 'equals' || filter.operator === 'notEquals')) { + whereClauses.push(`${fieldPath} IS ${filter.operator === 'notEquals' ? 'NOT ' : ''}NULL`); + } else { + whereClauses.push(`${fieldPath} ${sqlOp} ${this.toSqlLiteral(comparand)}`); + } } } } @@ -465,10 +523,10 @@ export class MemoryAnalyticsService implements IAnalyticsService { * which sibling branch was walked first. Both public entry points (`query()` * and `generateSql()`) go through this method, so both refuse identically. */ - private normalizeFilters(query: unknown): Array<{ member: string; operator: string; values: string[] }> { + private normalizeFilters(query: unknown): NormalizedCubeFilter[] { if (!query || typeof query !== 'object') return []; - const out: Array<{ member: string; operator: string; values: string[] }> = []; + const out: NormalizedCubeFilter[] = []; const where = (query as { where?: unknown }).where; if (where && typeof where === 'object' && !Array.isArray(where)) { @@ -481,12 +539,28 @@ export class MemoryAnalyticsService implements IAnalyticsService { private flattenFilterCondition( cond: Record, - out: Array<{ member: string; operator: string; values: string[] }>, + out: NormalizedCubeFilter[], path: string, ): void { for (const [key, raw] of Object.entries(cond)) { const here = `${path}.${key}`; - if (raw == null) continue; + + // [#5373] There is deliberately no `if (raw == null) continue` here. + // + // There was, and it was the more dangerous half of this issue: `null` is a + // COMPARAND, not an absent constraint, so `{closed_at: null}` produced no + // cube entry at all and the predicate simply vanished. One fewer + // constraint means MORE rows — a "closed_at is empty" widget silently + // aggregated the whole table, including the closed records it was written + // to exclude, and a widened chart looks exactly like a working chart. That + // is the #3948 direction, and on an RLS read scope it is an unauthorized + // read rather than a wrong number. + // + // `undefined` falls through with it, matching the live query path, which + // has never distinguished the two (`normalizeFilterCondition` sends both + // to `toStorageForm` and lets mingo's null-equality rule decide). Agreeing + // with that path is the invariant (#5240); inventing a third reading of + // `{field: undefined}` here would break it in the other direction. // Logical combinators. `$and` folds into the same implicit-AND list; the // gate above has already proven it is an array of filter nodes. @@ -506,17 +580,19 @@ export class MemoryAnalyticsService implements IAnalyticsService { } // Operator wrapper: { field: { $op: value, ... } } - if (typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { + // + // `raw !== null` carries real weight now that the blanket `raw == null` + // skip above is gone: `typeof null === 'object'`, so a null comparand + // would otherwise be read as an operator map and reach `Object.keys(null)`. + // It is a comparand — it belongs to the implicit-equality arm below. + if (raw !== null && typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { const wrapper = raw as Record; const opEntries = Object.keys(wrapper).filter(k => k.startsWith('$')); if (opEntries.length > 0) { for (const opKey of opEntries) { const cubeOp = this.mongoOperatorToCubeOperator(opKey, key, `${here}.${opKey}`); const v = wrapper[opKey]; - const values = Array.isArray(v) - ? v.map(x => this.stringifyForCube(x)) - : [this.stringifyForCube(v)]; - out.push({ member: key, operator: cubeOp, values }); + out.push({ member: key, operator: cubeOp, values: Array.isArray(v) ? [...v] : [v] }); } continue; } @@ -529,13 +605,10 @@ export class MemoryAnalyticsService implements IAnalyticsService { } // Implicit equality: { field: scalar | array } - const values = Array.isArray(raw) - ? raw.map(x => this.stringifyForCube(x)) - : [this.stringifyForCube(raw)]; out.push({ member: key, operator: Array.isArray(raw) ? 'in' : 'equals', - values, + values: Array.isArray(raw) ? [...raw] : [raw], }); } } @@ -558,52 +631,53 @@ export class MemoryAnalyticsService implements IAnalyticsService { } /** - * Stringify a filter value for cube-style storage. Booleans become - * `'1'/'0'` so that downstream consumers expecting SQLite-style - * numeric booleans match correctly. The in-memory pipeline uses - * {@link coerceFilterValue} to recover real JS types from these - * strings. - */ - private stringifyForCube(v: unknown): string { - if (v == null) return ''; - if (typeof v === 'boolean') return v ? '1' : '0'; - if (v instanceof Date) return v.toISOString(); - if (typeof v === 'object') return JSON.stringify(v); - return String(v); - } - - /** - * Recover a runtime value from its cube-stringified form for in-memory - * comparison. Booleans, integers, floats and ISO-date-like strings are - * coerced; everything else stays as a string. + * [#5373] The comparands of one lowered entry, in the storage form of the + * field they are compared against — the ONE place either exit converts a + * value, so the two exits cannot drift apart. + * + * The only conversion left is the temporal one (#4047): a `datetime` column + * holds canonical UTC ISO text, so a `Date` comparand has to become that text + * or mingo's cross-type comparison drops every row. That rule is keyed on the + * DECLARED field kind and belongs to the driver, so it is borrowed from the + * driver rather than re-derived here — a second derivation of it is the + * in-package divergence #5240 ruled against. + * + * Everything else passes through untouched. That is the point of #5373: a + * boolean stays a boolean, `null` stays `null`, and a text column's `'100'` + * stays the string `'100'` instead of becoming the number `100`. */ - private coerceFilterValue(s: string): unknown { - if (s === 'true') return true; - if (s === 'false') return false; - if (s === 'null') return null; - // Numeric strings: integer or float (no leading zeros except '0') - if (/^-?\d+$/.test(s)) { - const n = Number(s); - if (Number.isFinite(n)) return n; - } - if (/^-?\d+\.\d+$/.test(s)) { - const n = Number(s); - if (Number.isFinite(n)) return n; - } - return s; + private comparandsFor(cube: Cube, member: string, values: unknown[]): unknown[] { + const table = this.extractTableName(cube.sql); + const fieldPath = this.resolveFieldPath(cube, member); + return values.map(v => this.driver.filterComparandStorageForm(table, fieldPath, v)); } /** - * Type-aware SQL literal formatter. Booleans and numbers are emitted - * unquoted; everything else is single-quoted with embedded quotes - * escaped. + * [#5373] A JS comparand as a SQL literal — the one point where a value is + * stringified, and the reason it may be. + * + * This used to take the cube-stringified `string`, which meant it could only + * guess the original type back out of the text: `'100'` from a TEXT column + * looked exactly like `100` from a numeric one, and it emitted both unquoted + * (`WHERE code = 100`). Given the real value there is nothing to guess. + * + * Booleans keep the SQLite-style `1`/`0` spelling the old encoding chose — + * that justification was always sound for THIS half, and only wrong because + * the in-memory half was forced to share it. + * + * A `null` comparand never reaches here from `equals`/`notEquals`; the WHERE + * builder emits `IS NULL` / `IS NOT NULL` for those. `NULL` is the honest + * literal for the remaining operators, which cannot be satisfied by it. */ - private toSqlLiteral(s: string): string { - if (s === 'true') return '1'; - if (s === 'false') return '0'; - if (s === 'null') return 'NULL'; - if (/^-?\d+(\.\d+)?$/.test(s)) return s; - return `'${s.replace(/'/g, "''")}'`; + private toSqlLiteral(v: unknown): string { + if (v == null) return 'NULL'; + if (typeof v === 'boolean') return v ? '1' : '0'; + if (typeof v === 'number') return Number.isFinite(v) ? String(v) : 'NULL'; + if (typeof v === 'bigint') return String(v); + const text = v instanceof Date + ? v.toISOString() + : typeof v === 'object' ? JSON.stringify(v) : String(v); + return `'${text.replace(/'/g, "''")}'`; } private resolveFieldPath(cube: Cube, member: string): string { diff --git a/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts b/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts index 064f6d7de4..38fb819d7e 100644 --- a/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts +++ b/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts @@ -55,6 +55,23 @@ * and ADR-0078 / #4286 each ruled on, and it stays true if the cube pipeline * later learns `$or` — the case simply moves from the refused column to the * agreeing one without this file changing. + * + * # The COMPARAND axis (#5373) + * + * `FILTER_LOGIC_CASES` is a table of operator and combinator SHAPES, and every + * column in its fixture is a string — deliberately, so that nothing in it is + * about coercion. That is a real hole on a face whose defect was coercion: all + * three cases above passed while `{is_active: true}` returned zero rows and + * `{closed_at: null}` returned the whole table, because the cube lowering + * round-tripped every comparand through `string[]` and that round trip is lossy + * for anything that is not already a string. + * + * So the second half of this file runs the same invariant over a fixture built + * to vary the comparand's TYPE instead of the filter's shape. It belongs beside + * the shape table rather than in a file of its own for the reason the shape + * table exists at all: the thing being defended is "this package's filter faces + * agree", and a divergence introduced on either axis has to fail in the place + * someone looks when they change a lowering. */ import { describe, it, expect, beforeAll } from 'vitest'; @@ -243,3 +260,215 @@ describe('[#5345] MemoryAnalyticsService — the same table, through the THIRD f } }); }); + +/** + * [#5373] The same invariant, over comparand TYPES rather than filter shapes. + * + * The fixture is the one measured in the issue, plus the columns that separate + * the third symptom: `code` is TEXT holding `'100'`, `qty` is NUMBER holding + * `100`, so a comparand that silently became a number is visible as a wrong row + * set on one and invisible on the other. `made_at` is a declared `datetime`, + * which is the one comparand that legitimately still converts (#4047) and so is + * the one a "stop converting" fix is most likely to break. + */ +const COMPARAND_TABLE = 'comparand_deal'; + +const COMPARAND_FIELDS = { + id: { type: 'text', name: 'id' }, + is_active: { type: 'boolean', name: 'is_active' }, + name: { type: 'text', name: 'name' }, + closed_at: { type: 'text', name: 'closed_at' }, + code: { type: 'text', name: 'code' }, + qty: { type: 'number', name: 'qty' }, + made_at: { type: 'datetime', name: 'made_at' }, +} as const; + +const COMPARAND_ROWS: Array> = [ + { id: '1', is_active: true, name: 'alpha', closed_at: null, code: '100', qty: 100, made_at: new Date('2026-01-01T00:00:00Z') }, + { id: '2', is_active: false, name: 'beta', closed_at: '2026-01-01', code: '200', qty: 200, made_at: new Date('2026-06-01T00:00:00Z') }, + { id: '3', is_active: true, name: 'gamma', closed_at: null, code: '100', qty: 100, made_at: new Date('2026-01-01T00:00:00Z') }, +]; + +const COMPARAND_CUBE: Cube = { + name: COMPARAND_TABLE, + title: 'Comparand round-trip', + sql: COMPARAND_TABLE, + measures: { count: { name: 'count', label: 'Rows', type: 'count', sql: 'id' } }, + dimensions: Object.fromEntries( + Object.keys(COMPARAND_FIELDS).map((f) => [f, { name: f, label: f, type: 'string' as const, sql: f }]), + ), + public: true, +}; + +/** + * Each case is `[name, where, expected ids]`. `expected` is asserted against the + * live path too, so a case whose expectation is simply wrong fails loudly rather + * than certifying whatever the two faces happen to agree on. + */ +const COMPARAND_CASES: Array<[name: string, where: FilterCondition, expected: string[]]> = [ + // The issue's measured table. Booleans round-tripped `true` → `'1'` → the + // NUMBER 1, and mingo never equates 1 with true, so both of these were 0 rows. + ['boolean true selects the true rows', { is_active: true } as FilterCondition, ['1', '3']], + ['boolean false selects the false row', { is_active: false } as FilterCondition, ['2']], + // The widening one: `{closed_at: null}` produced no cube entry at all, so the + // predicate vanished and the query answered with the whole table (#3948). + ['a null comparand is a predicate, not an absent one', { closed_at: null } as FilterCondition, ['1', '3']], + ['negated null selects the complement', { closed_at: { $ne: null } } as FilterCondition, ['2']], + // The issue's UNVERIFIED third symptom, measured and real: `'100'` from a TEXT + // column round-tripped to the number 100 and matched nothing. + ['a numeric-looking STRING stays a string', { code: '100' } as FilterCondition, ['1', '3']], + ['a numeric-looking string inside $in stays a string', { code: { $in: ['100', '200'] } } as FilterCondition, ['1', '2', '3']], + ['a real number comparand still matches a numeric column', { qty: 100 } as FilterCondition, ['1', '3']], + ['a real number inside $in still matches', { qty: { $in: [100, 200] } } as FilterCondition, ['1', '2', '3']], + // Same root cause, opposite direction: `$ne` against the number 1 excluded + // nothing, so a negated boolean answered with every row. + ['negated boolean excludes only the matching rows', { is_active: { $ne: true } } as FilterCondition, ['2']], + // The comparand that must STILL convert: a `Date` against a declared + // `datetime` column, which stores canonical UTC ISO text (#4047). + ['a Date comparand still meets a datetime column', { made_at: new Date('2026-01-01T00:00:00Z') } as FilterCondition, ['1', '3']], + ['a Date bound still orders against a datetime column', { made_at: { $lte: new Date('2026-03-01T00:00:00Z') } } as FilterCondition, ['1', '3']], + // Ordinary strings were never broken; pinned so a fix aimed at the others + // cannot quietly cost the common case. + ['a plain string comparand is unchanged', { name: 'alpha' } as FilterCondition, ['1']], + ['$and folds comparands of mixed type', { $and: [{ is_active: true }, { code: '100' }] } as FilterCondition, ['1', '3']], +]; + +describe('[#5373] comparand types — the analytics face against the live query path', () => { + let driver: InMemoryDriver; + let service: MemoryAnalyticsService; + + beforeAll(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + await driver.syncSchema(COMPARAND_TABLE, { fields: { ...COMPARAND_FIELDS } } as never); + for (const row of COMPARAND_ROWS) await driver.create(COMPARAND_TABLE, { ...row }); + service = new MemoryAnalyticsService({ driver, cubes: [COMPARAND_CUBE] }); + }); + + const sorted = (ids: string[]): string[] => [...ids].sort((x, y) => x.localeCompare(y)); + + const findIds = async (where: FilterCondition): Promise => { + const rows = await driver.find(COMPARAND_TABLE, { object: COMPARAND_TABLE, fields: ['id'], where }); + return sorted((rows as Array>).map((r) => String(r.id))); + }; + + const analyticsIds = async (where: FilterCondition): Promise => { + const result = await service.query({ + cube: COMPARAND_TABLE, + measures: [`${COMPARAND_TABLE}.count`], + dimensions: [`${COMPARAND_TABLE}.id`], + where, + }); + return sorted((result.rows as Array>).map((r) => String(r[`${COMPARAND_TABLE}.id`]))); + }; + + it('the fixture really is all three rows', async () => { + expect(await findIds({})).toEqual(['1', '2', '3']); + expect(await analyticsIds({})).toEqual(['1', '2', '3']); + }); + + for (const [name, where, expected] of COMPARAND_CASES) { + it(name, async () => { + // The live path first: it is the reference, so a wrong `expected` is + // reported as such instead of being blamed on the analytics face. + expect(await findIds(where), `${name}: the LIVE path disagrees with the expectation`).toEqual(sorted(expected)); + expect( + await analyticsIds(where), + `${name}: the analytics face answered a different row set than find() — the #5240 divergence`, + ).toEqual(sorted(expected)); + }); + } + + /** + * The whole point, stated once as a predicate over the whole table: no case + * may be answered differently by the two faces. Case-by-case assertions above + * already imply it, but stated here it survives the expectations being edited. + */ + it('no comparand case is answered differently by the two faces', async () => { + for (const [name, where] of COMPARAND_CASES) { + expect(await analyticsIds(where), `${name}: the two faces disagree`).toEqual(await findIds(where)); + } + }); + + /** + * The counter-test for the fixture itself. If `is_active` were stored as + * `1`/`0` rather than `true`/`false`, the boolean cases above would pass + * without the bug ever having been fixed. + */ + it('the fixture stores real booleans and real nulls, not their stringified forms', async () => { + const rows = (await driver.find(COMPARAND_TABLE, { + object: COMPARAND_TABLE, + fields: ['id', 'is_active', 'closed_at', 'code'], + })) as Array>; + const one = rows.find((r) => r.id === '1')!; + expect(one.is_active).toBe(true); + expect(one.closed_at).toBeNull(); + expect(one.code).toBe('100'); + expect(rows.find((r) => r.id === '2')!.is_active).toBe(false); + }); +}); + +/** + * [#5373] The OTHER exit. + * + * `query()` and `generateSql()` compile the same lowered entries into different + * targets, and the defect existed precisely because one encoding served both. A + * fix that satisfies mingo while emitting SQL that means something else has + * moved the bug rather than closed it, so the SQL exit is pinned on the same + * cases — including the two where the correct SQL is not a comparison at all. + */ +describe('[#5373] the generateSql exit emits literals that mean the same thing', () => { + let service: MemoryAnalyticsService; + + beforeAll(async () => { + const driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + await driver.syncSchema(COMPARAND_TABLE, { fields: { ...COMPARAND_FIELDS } } as never); + service = new MemoryAnalyticsService({ driver, cubes: [COMPARAND_CUBE] }); + }); + + const whereClause = async (where: FilterCondition): Promise => { + const { sql } = await service.generateSql({ + cube: COMPARAND_TABLE, + measures: [`${COMPARAND_TABLE}.count`], + where, + }); + const m = /WHERE (.*?)(?: GROUP BY | ORDER BY | LIMIT | OFFSET |$)/.exec(sql); + return m ? m[1].trim() : ''; + }; + + it('a boolean keeps the SQLite-style numeric spelling', async () => { + expect(await whereClause({ is_active: true } as FilterCondition)).toBe('is_active = 1'); + expect(await whereClause({ is_active: false } as FilterCondition)).toBe('is_active = 0'); + }); + + /** + * `= NULL` is never true in SQL, so emitting it would make this exit select + * nothing while `query()` selects the null rows — the same divergence one + * layer over. Before #5373 the clause was absent entirely, which is the + * widening direction instead. + */ + it('a null comparand becomes a nullness test, not a comparison', async () => { + expect(await whereClause({ closed_at: null } as FilterCondition)).toBe('closed_at IS NULL'); + expect(await whereClause({ closed_at: { $ne: null } } as FilterCondition)).toBe('closed_at IS NOT NULL'); + }); + + /** + * The third symptom at this exit: with only the stringified form to read, + * a TEXT `'100'` and a numeric `100` were indistinguishable and both were + * emitted unquoted. + */ + it('a numeric-looking string is quoted and a real number is not', async () => { + expect(await whereClause({ code: '100' } as FilterCondition)).toBe("code = '100'"); + expect(await whereClause({ qty: 100 } as FilterCondition)).toBe('qty = 100'); + }); + + it('a string comparand escapes its embedded quotes', async () => { + expect(await whereClause({ name: "O'Brien" } as FilterCondition)).toBe("name = 'O''Brien'"); + }); + + it('a Date comparand is emitted as quoted canonical UTC text', async () => { + expect(await whereClause({ made_at: new Date('2026-01-01T00:00:00Z') } as FilterCondition)) + .toBe("made_at = '2026-01-01T00:00:00.000Z'"); + }); +}); diff --git a/packages/plugins/driver-memory/src/memory-driver.ts b/packages/plugins/driver-memory/src/memory-driver.ts index d3a08a05e3..2bdfbd5048 100644 --- a/packages/plugins/driver-memory/src/memory-driver.ts +++ b/packages/plugins/driver-memory/src/memory-driver.ts @@ -1213,6 +1213,26 @@ export class InMemoryDriver implements IDataDriver { return coerceTemporalValue(value, this.temporalKind(object, field)); } + /** + * [#5373] {@link toStorageForm}, for the analytics (cube) face. + * + * That face compiles its own `where` (`memory-analytics.ts`) and must compare + * against the same stored bytes this driver wrote, so it needs the same + * comparand rule — and the rule is keyed on the DECLARED field kind + * (`temporalFields`, populated by `syncSchema`), which only the driver holds. + * The alternative was for the analytics face to re-derive a temporal form from + * the value's shape, and a second implementation of this rule is precisely the + * in-package divergence #5240 ruled against: mingo compares cross-type as + * never-equal, so the two faces would answer one `where` with different rows + * the moment the two derivations disagreed. + * + * Deliberately narrow — one comparand, no filter semantics — so it exposes the + * convention without exposing the filter pipeline. + */ + filterComparandStorageForm(object: string | undefined, field: string, value: unknown): unknown { + return this.toStorageForm(object, field, value); + } + /** * Put every declared temporal field of a record into its storage form — the * write half of the convention the filter path reads against. Returns the