diff --git a/.changeset/analytics-filter-value-type-fidelity.md b/.changeset/analytics-filter-value-type-fidelity.md new file mode 100644 index 0000000000..52d43c197e --- /dev/null +++ b/.changeset/analytics-filter-value-type-fidelity.md @@ -0,0 +1,56 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): 过滤值不再被降级成字符串 —— `{code: {$eq: '007'}}` / `'null'` / `'true'` 按作者写的字面值绑定 (#5526) + +analytics 的 `filter-normalizer` 内部把每个比较数(comparand)压成 `values: string[]` +再由消费方**猜**回类型:出口是 `stringifyForCube`,入口是 `recoverNumber` 与 +`coerceFilterValueForSql` / `coerceFilterValueForObjectQL`。字母表是"全体字符串"、 +解码规则是"这串看起来像不像数字/布尔/null"的编码没有任何转义机制,于是作者写的字符串 +和编码器为其他类型写下的 token 撞车。`{code: {$eq: v}}` 在 `main` 上实测: + +| 作者的 `v` | SQL 绑定 | 引擎绑定 | +|---|---|---| +| `'007'` | `7`(#5528 已修) | `7`(#5528 已修) | +| `'1.50'` | `1.5`(#5528 已修) | `1.5`(#5528 已修) | +| `'null'` | 真 NULL | 真 `null` | +| `'true'` | `1` | `true` | + +每一行都是一个缺陷:存着作者那种写法的 TEXT 列不再匹配。`'007'` 在 SQLite 上是 +整数与 TEXT 列的跨类型比较、恒不相等,在 Postgres 上 `text = integer` 直接报类型错; +`'null'` 那一行比"空"更糟 —— 与真 NULL 的比较对任何行都是 UNKNOWN,图表永远画不出东西。 +零填充串、当枚举码用的 `'true'`/`'false'`、当字面标签用的 `'null'` 都是真实业务形状 +(订单号、SKU、邮编、国际长途区号)。 + +**修法**:`NormalizedFilterNode` 的 leaf `values` 由 `string[]` 改为 `unknown[]`, +作者写的值原样穿过整棵树,不再有任何东西去解码它。仅在边界真正要求时才转换: + +- `toSqlBindValue`(唯一留下的转换,且是**单向**的:值 → 它的 SQL 绑定形态,不是解码器) + ——只处理驱动绑不了的 JS 类型:`boolean` → `1`/`0`(better-sqlite3 拒绝 JS 布尔)、 + `Date` → ISO 文本、其他对象 → JSON 文本。它不检查任何字符串。 +- LIKE 族的比较数被 `filter.zod.ts` 声明为 `z.string()`,所以在发射点字符串化 —— + 与 `driver-sql` 的 `applyLike` 同一个 `String(value)`,两个面上 `$contains` 仍是一件事。 + +ObjectQL 引擎路径现在不需要任何转换:引擎按**存储**的运行时类型比较,而它拿到的就是 +作者写的值。`stringifyForCube` / `recoverNumber` / `coerceFilterValueForSql` / +`coerceFilterValueForObjectQL` 一并删除。 + +两处读法作为直接后果改变了,方向都是 fail-closed: + +- `{name: {$contains: null}}` 原先编译成 `LIKE '%%'` —— 匹配**每一个**非 NULL 行, + 因为 `stringifyForCube(null)` 是 `''`;现在是 `LIKE '%null%'`,与 `driver-sql` + 一直以来的编译结果一致。 +- `{amount: {$gt: null}}` 原先编译成 `amount > ''`(一次针对空字符串的真实比较); + 现在绑定 NULL,谓词为 UNKNOWN、图表画不出行 —— 无序比较数的诚实答案,也是 + `driver-memory` / `formula` 给出的答案。(#5332 明确指出这个比较数位置没有任何裁决 + 覆盖、`''` 只是占位符;删掉编码器就按构造把它定了。) + +`timeDimensions[].dateRange` 的两个边界现在按 spec 声明的类型(`string[]`)原样传递: +原先它们也过 `coerceFilterValueForObjectQL`,其文档宣称"epoch-ms 边界会还原成数字"—— +那是消费方在宽容地兜一个契约并未声明的形状,和把 `'007'` 读成 `7` 是同一个猜测 +(Prime Directive #12:epoch-ms 窗口要么在生产者、要么在 spec 里声明,不在这里猜)。 + +`{stage: null}` / `{$eq: null}` / `{$ne: null}` / `{$null:}` / `{$exists:}` 的空值 +谓词语义(#5332 / #5525)不变:真 `null` 比较数编译成 `notSet` / `set`,从不进入 +`values`。#5567 的 LIKE 转义契约不变。 diff --git a/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts b/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts index 8d52fb6c24..84a7416f06 100644 --- a/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts +++ b/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts @@ -146,7 +146,9 @@ const ACCEPTED: Array<{ name: string; where: unknown; tree: unknown }> = [ { name: 'an explicit operator', where: { amount: { $gte: 10 } }, - tree: { kind: 'leaf', member: 'amount', operator: 'gte', values: ['10'] }, + // [#5526] `values` is `unknown[]`, so the number the author wrote stays a + // number instead of being encoded to `'10'` and guessed back. + tree: { kind: 'leaf', member: 'amount', operator: 'gte', values: [10] }, }, { name: '$between lowered to its two bounds', @@ -154,8 +156,8 @@ const ACCEPTED: Array<{ name: string; where: unknown; tree: unknown }> = [ tree: { kind: 'and', children: [ - { kind: 'leaf', member: 'amount', operator: 'gte', values: ['10'] }, - { kind: 'leaf', member: 'amount', operator: 'lte', values: ['20'] }, + { kind: 'leaf', member: 'amount', operator: 'gte', values: [10] }, + { kind: 'leaf', member: 'amount', operator: 'lte', values: [20] }, ], }, }, diff --git a/packages/services/service-analytics/src/__tests__/filter-value-canonical-number.test.ts b/packages/services/service-analytics/src/__tests__/filter-value-canonical-number.test.ts deleted file mode 100644 index c16280d20e..0000000000 --- a/packages/services/service-analytics/src/__tests__/filter-value-canonical-number.test.ts +++ /dev/null @@ -1,374 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * A STRING comparand keeps its spelling — #5528 (route C of #5526). - * - * `filter-normalizer` round-trips every comparand through `values: string[]`: - * `stringifyForCube` on the way out, `coerceFilterValueForSql` / - * `coerceFilterValueForObjectQL` on the way back. The decoder used to decide - * "this is a number" from the string's SHAPE alone (`/^-?\d+(\.\d+)?$/`), which - * cannot tell a stringified number from a string the author wrote — so `'007'` - * came back as `7` and `'1.50'` as `1.5`, on BOTH consumers. - * - * Measured on `main` before the fix (the #5526 table, reproduced): - * - * | author's `v` | leaf values | SQL bind | engine bind | - * |---|---|---|---| - * | `'007'` | `["007"]` | `7` | `7` | - * | `'0912'` | `["0912"]` | `912` | `912` | - * | `'1.50'` | `["1.50"]` | `1.5` | `1.5` | - * - * Zero-padded and trailing-zero strings are ordinary business shapes — order - * numbers, SKUs, dialling codes, postcodes, prices — so the row-set case below - * is the symptom an author actually reports: a widget filtered on order number - * `'007'` returning the row that stores `'7'`, or nothing at all. - * - * Recovery is now limited to a number's own canonical spelling - * (`String(Number(s)) === s`). The two directions this file has to hold apart: - * - * - a comparand that REALLY was a number is `String(n)` by construction, so it - * still round-trips (`7` → `'7'` → `7`) — the guard against over-correcting; - * - a string `Number()` would rewrite cannot have come from a number, so it - * stays the author's string. - * - * ⛔ Scope: `'null'` / `'true'` / `'false'` still collide with the tokens the - * ENCODER writes for the real values — that is the `string[]` encoding having no - * escape, i.e. #5526's root cause, deliberately not touched here. The collision - * is pinned below as UNCHANGED so no future reader mistakes it for fixed. - */ - -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import type { Cube, AnalyticsQuery, FilterCondition } from '@objectstack/spec/data'; -import type { StrategyContext } from '@objectstack/spec/contracts'; -import { DatasetSchema } from '@objectstack/spec/ui'; - -import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; -import { - normalizeAnalyticsFilterTree, - coerceFilterValueForSql, - coerceFilterValueForObjectQL, -} from '../strategies/filter-normalizer.js'; -import { AnalyticsService } from '../analytics-service.js'; - -// ── 1. The decoder, value by value ─────────────────────────────────────────── - -/** - * `sql` / `objectql` are what each consumer must bind. They differ only on the - * boolean tokens (the SQL path cannot bind a JS boolean); which strings count as - * NUMBERS is one rule shared by both, which is why every numeric row here - * carries the same expectation twice. - */ -const DECODE: Array<{ input: string; sql: unknown; objectql: unknown; why: string }> = [ - // The #5526 table's regression rows: information `Number()` would destroy. - { input: '007', sql: '007', objectql: '007', why: 'leading zeros — order number / SKU (was 7)' }, - { input: '0912', sql: '0912', objectql: '0912', why: 'leading zero — dialling code (was 912)' }, - { input: '1.50', sql: '1.50', objectql: '1.50', why: 'trailing zero — price string (was 1.5)' }, - { input: '1.0', sql: '1.0', objectql: '1.0', why: 'trailing zero (was 1)' }, - { input: '-0', sql: '-0', objectql: '-0', why: 'the sign is lost: String(Number("-0")) is "0" (was 0)' }, - { - input: '12345678901234567890', - sql: '12345678901234567890', - objectql: '12345678901234567890', - why: 'more digits than a double holds (was 12345678901234567000)', - }, - { - input: '1000000000000000000000', - sql: '1000000000000000000000', - objectql: '1000000000000000000000', - why: 'canonical form of 1e21 is exponential, so the digits round-trip lossily (was 1e21)', - }, - - // Canonical numeric spellings — what a real number comparand encodes to. - { input: '7', sql: 7, objectql: 7, why: 'String(7) — recovered' }, - { input: '1.5', sql: 1.5, objectql: 1.5, why: 'String(1.5) — recovered' }, - { input: '-3', sql: -3, objectql: -3, why: 'negative canonical — recovered' }, - { input: '0', sql: 0, objectql: 0, why: 'String(0) — recovered' }, - { input: '1000', sql: 1000, objectql: 1000, why: 'trailing zeros are canonical HERE — recovered' }, - - // Shapes the leading regex rejected before this change and still rejects: the - // narrowing can only ever REMOVE recoveries, never add one. - { input: '1e3', sql: '1e3', objectql: '1e3', why: 'exponent — string before #5528, string after' }, - { input: '1e+21', sql: '1e+21', objectql: '1e+21', why: 'canonical String(1e21), but the regex still refuses it' }, - { input: '+7', sql: '+7', objectql: '+7', why: 'leading plus — never recovered' }, - { input: ' 7', sql: ' 7', objectql: ' 7', why: 'whitespace — never recovered' }, - { input: '0x10', sql: '0x10', objectql: '0x10', why: 'hex — never recovered' }, - { input: 'Infinity', sql: 'Infinity', objectql: 'Infinity', why: 'not finite — never recovered' }, - { input: 'NaN', sql: 'NaN', objectql: 'NaN', why: 'not finite — never recovered' }, - { input: '', sql: '', objectql: '', why: 'the empty string is a value, not a number' }, - { input: 'won', sql: 'won', objectql: 'won', why: 'plain text' }, -]; - -describe('analytics filter decode — only a canonical numeric spelling is a number (#5528)', () => { - for (const c of DECODE) { - it(`${JSON.stringify(c.input)} → SQL ${JSON.stringify(c.sql)} / engine ${JSON.stringify(c.objectql)}`, () => { - expect(coerceFilterValueForSql(c.input), c.why).toEqual(c.sql); - expect(coerceFilterValueForObjectQL(c.input), c.why).toEqual(c.objectql); - }); - } - - it('keeps the two consumers in step on WHICH strings are numbers', () => { - // The table deliberately holds no boolean/null token — those are the one - // place the two consumers are MEANT to differ, and they are pinned in their - // own case below. Everywhere else a divergence means one coercer grew its own - // idea of a number, which is how one `where` starts meaning two things. - for (const c of DECODE) { - expect(coerceFilterValueForSql(c.input)).toEqual(coerceFilterValueForObjectQL(c.input)); - } - }); -}); - -describe('a real number comparand still round-trips (the over-correction guard)', () => { - // Encode with the real encoder rather than a hand-written string: this is the - // path a `{$eq: 7}` actually takes, so it fails if the narrowing went too far. - for (const n of [7, 1.5, -3, 0, 1000, -12.25]) { - it(`${n} → values → ${n}`, () => { - const node = normalizeAnalyticsFilterTree({ where: { score: { $eq: n } } }); - expect(node?.kind).toBe('leaf'); - const encoded = node?.kind === 'leaf' ? node.values[0] : undefined; - expect(encoded).toBe(String(n)); - expect(coerceFilterValueForSql(encoded!)).toBe(n); - expect(coerceFilterValueForObjectQL(encoded!)).toBe(n); - }); - } -}); - -describe('#5526 root cause NOT fixed here — the token collision is unchanged', () => { - // Pinned, not endorsed. `stringifyForCube` writes these same three tokens for - // the real `null` / `true` / `false`, and `values: string[]` has no escape, so - // an author filtering for the literal text still loses. Route A/B in #5526. - it('the author strings null / true / false still decode to non-strings', () => { - expect(coerceFilterValueForSql('null')).toBeNull(); - expect(coerceFilterValueForObjectQL('null')).toBeNull(); - expect(coerceFilterValueForSql('true')).toBe(1); - expect(coerceFilterValueForObjectQL('true')).toBe(true); - expect(coerceFilterValueForSql('false')).toBe(0); - expect(coerceFilterValueForObjectQL('false')).toBe(false); - }); -}); - -// ── 2. The SQL consumer: bound params AND row sets ─────────────────────────── - -interface Row { - id: string; - code: string; - score: number; -} - -/** - * `r_7` is the row that makes the bug visible rather than merely empty: with the - * comparand downgraded to the integer `7`, SQLite applies the TEXT column's - * affinity to it, compares `'7'`, and hands the author asking for order `'007'` - * a DIFFERENT row. `r_15` does the same for `'1.50'`. - */ -const ROWS: Row[] = [ - { id: 'r_007', code: '007', score: 7 }, - { id: 'r_7', code: '7', score: 7 }, - { id: 'r_150', code: '1.50', score: 1 }, - { id: 'r_15', code: '1.5', score: 1 }, -]; - -const CUBE: Cube = { - name: 'orders', - title: 'Orders', - sql: 'orders', - measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, - dimensions: { - id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, - code: { name: 'code', label: 'Code', type: 'string', sql: 'code' }, - score: { name: 'score', label: 'Score', type: 'number', sql: 'score' }, - }, - public: false, -} as unknown as Cube; - -const ROW_CASES: Array<{ name: string; filter: FilterCondition; expected: string[]; note: string }> = [ - { - name: "{code: {$eq: '007'}} finds the row storing '007'", - filter: { code: { $eq: '007' } }, - expected: ['r_007'], - note: "Before #5528 the bind was the integer 7 and this returned ['r_7'] — the wrong row, silently.", - }, - { - name: "{code: {$eq: '1.50'}} finds the row storing '1.50'", - filter: { code: { $eq: '1.50' } }, - expected: ['r_150'], - note: "Before #5528 the bind was 1.5 and this returned ['r_15'].", - }, - { - name: "{code: {$eq: '7'}} still finds the row storing '7'", - filter: { code: { $eq: '7' } }, - expected: ['r_7'], - note: 'A canonical spelling is still recovered as a number; on a TEXT column SQLite compares it as text, so the right row comes back either way.', - }, - { - name: "{code: {$in: ['007', '1.50']}} keeps both spellings", - filter: { code: { $in: ['007', '1.50'] } }, - expected: ['r_007', 'r_150'], - note: 'The coercers serve every operator, not just $eq.', - }, - { - name: '{score: {$eq: 7}} still matches on the numeric column', - filter: { score: { $eq: 7 } }, - expected: ['r_007', 'r_7'], - note: 'A genuine number comparand is unaffected by the narrowing.', - }, -]; - -/** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ -async function locateWasm(): Promise<((file: string) => string) | undefined> { - try { - const { createRequire } = await import('node:module'); - const require = createRequire(import.meta.url); - const pkgJsonPath = require.resolve('sql.js/package.json'); - const { dirname, join } = await import('node:path'); - const dir = dirname(pkgJsonPath); - return (file: string) => join(dir, 'dist', file); - } catch { - return undefined; - } -} - -describe("analytics SQL path — a text column's own spelling is what gets bound (#5528)", () => { - let db: any; - let ctx: StrategyContext; - let bound: unknown[][]; - - beforeAll(async () => { - const mod: any = await import('sql.js'); - const initSqlJs = mod.default ?? mod; - const locateFile = await locateWasm(); - const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); - - db = new SQL.Database(); - // `code` is TEXT on purpose: the whole point is a column that STORES the - // author's spelling, which is where a numeric bind stops matching. - db.run(`CREATE TABLE "orders" ("id" TEXT PRIMARY KEY, "code" TEXT, "score" INTEGER);`); - const insert = db.prepare(`INSERT INTO "orders" ("id","code","score") VALUES (?,?,?)`); - for (const r of ROWS) insert.run([r.id, r.code, r.score]); - insert.free(); - - bound = []; - ctx = { - getCube: (name: string) => (name === 'orders' ? CUBE : undefined), - queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), - executeRawSql: async (_object: string, sql: string, params: unknown[]) => { - bound.push(params); - const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); - stmt.bind(params as any[]); - const out: Record[] = []; - while (stmt.step()) out.push(stmt.getAsObject()); - stmt.free(); - return out; - }, - } as StrategyContext; - }); - - afterAll(() => { - db?.close(); - }); - - for (const c of ROW_CASES) { - it(c.name, async () => { - bound.length = 0; - const result = await new NativeSQLStrategy().execute( - { cube: 'orders', measures: ['total'], dimensions: ['id'], where: c.filter } as AnalyticsQuery, - ctx, - ); - const got = result.rows.map((r) => String(r.id)).sort(); - expect(got, c.note).toEqual(c.expected); - }); - } - - it("binds '007' as TEXT, not as the integer 7", async () => { - bound.length = 0; - await new NativeSQLStrategy().execute( - { cube: 'orders', measures: ['total'], dimensions: ['id'], where: { code: { $eq: '007' } } } as AnalyticsQuery, - ctx, - ); - expect(bound[0]).toContain('007'); - expect(bound[0].map((p) => typeof p)).toContain('string'); - expect(bound[0]).not.toContain(7); - }); - - it('binds a genuine number comparand as a NUMBER', async () => { - bound.length = 0; - await new NativeSQLStrategy().execute( - { cube: 'orders', measures: ['total'], dimensions: ['id'], where: { score: { $eq: 7 } } } as AnalyticsQuery, - ctx, - ); - // Not merely "the rows came back": SQLite's numeric affinity would rescue a - // '7' bound as text on this column, so the row set alone cannot see the - // difference. The bound TYPE can. - expect(bound[0]).toContain(7); - expect(bound[0]).not.toContain('7'); - }); -}); - -// ── 3. The ObjectQL consumer: the comparand handed to the engine ───────────── - -const dataset = DatasetSchema.parse({ - name: 'orders', - label: 'Orders', - object: 'order', - dimensions: [{ name: 'code', field: 'code', type: 'string' }], - measures: [{ name: 'order_count', aggregate: 'count' }], -}); - -/** Stored rows carry the author's STRINGS, exactly as the engine would hold them. */ -const ENGINE_ROWS: Array<{ code: string }> = [{ code: '007' }, { code: '7' }, { code: '1.50' }]; - -/** - * A stand-in for `engine.aggregate` that filters with STRICT equality, the way - * the real engine compares against a stored value: a comparand downgraded to `7` - * matches none of the rows above, so the count genuinely goes to zero. - */ -function makeEngine(captured: Array | undefined>) { - return async ( - _object: string, - options: { groupBy?: string[]; filter?: Record }, - ): Promise>> => { - captured.push(options.filter); - const filtered = ENGINE_ROWS.filter((row) => - Object.entries(options.filter ?? {}).every( - ([field, cond]) => (row as Record)[field] === cond, - ), - ); - return [{ order_count: filtered.length }]; - }; -} - -describe("analytics engine path — the comparand reaches engine.aggregate as '007' (#5528)", () => { - for (const v of ['007', '1.50']) { - it(`passes the string ${JSON.stringify(v)} through, and counts the row that stores it`, async () => { - const captured: Array | undefined> = []; - const svc = new AnalyticsService({ - queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), - executeAggregate: makeEngine(captured), - }); - - const result = await svc.queryDataset!(dataset, { - measures: ['order_count'], - runtimeFilter: { code: { $eq: v } }, - }); - - expect(captured[0]?.code).toBe(v); - // Was 0 before #5528: the engine compared the number 7 / 1.5 against a - // stored string and nothing matched. - expect(result.rows).toEqual([{ order_count: 1 }]); - }); - } - - it('still hands the engine a real number for a number comparand', async () => { - const captured: Array | undefined> = []; - const svc = new AnalyticsService({ - queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), - executeAggregate: makeEngine(captured), - }); - - await svc.queryDataset!(dataset, { - measures: ['order_count'], - runtimeFilter: { code: { $eq: 7 } }, - }); - - expect(captured[0]?.code).toBe(7); - }); -}); diff --git a/packages/services/service-analytics/src/__tests__/filter-value-type-fidelity.test.ts b/packages/services/service-analytics/src/__tests__/filter-value-type-fidelity.test.ts new file mode 100644 index 0000000000..c2adc5e3a5 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/filter-value-type-fidelity.test.ts @@ -0,0 +1,654 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A comparand keeps its own TYPE, end to end — #5526. + * + * `filter-normalizer` used to carry `values: string[]`, so every comparand was + * flattened to a string on the way in (`stringifyForCube`) and GUESSED back into + * a type on the way out (`recoverNumber`, behind `coerceFilterValueForSql` / + * `coerceFilterValueForObjectQL`). An all-strings encoding has no escape, so + * author strings collided with the tokens the encoder wrote for other types. + * Measured on `main` for `{code: {$eq: v}}` — #5526's table: + * + * | author's `v` | leaf values | SQL bind | engine bind | + * |---|---|---|---| + * | `'007'` | `["007"]` | `7` → fixed by #5528 | `7` → fixed by #5528 | + * | `'1.50'` | `["1.50"]` | `1.5` → fixed by #5528 | `1.5` → fixed by #5528 | + * | `'null'` | `["null"]` | real `NULL` | real `null` | + * | `'true'` | `["true"]` | `1` | `true` | + * + * `values` is now `unknown[]`: the author's value travels untouched and NOTHING + * decodes it. So this file no longer tests a decoder — there is none. It tests + * the property that replaced it, on every consumer that reads a leaf: + * + * 1. the leaf carries `v` ITSELF; + * 2. the SQL path binds `v`, converted only where a driver cannot bind the JS + * type (`boolean` → `1`/`0`, `Date` → ISO) — never by re-reading a string; + * 3. the engine path binds `v` with no conversion at all; + * 4. the row sets that follow, against a TEXT column with DECOY rows. + * + * # Provenance — this file is #5528's asset, carried forward + * + * It was `filter-value-canonical-number.test.ts`, which pinned the narrowed + * decoder #5528 shipped as an explicit stopgap. Every case of that file survives + * here, upgraded from "what does `coerceFilterValueForSql('007')` return" to the + * end-to-end question, because the function it named is gone and the scenario is + * not. Two blocks changed VERDICT rather than form, and both are the point of + * #5526: + * + * - the `'null'` / `'true'` / `'false'` collision, pinned there as UNCHANGED + * ("route A/B in #5526"), is now pinned as FIXED; + * - `{score: {$gte: '80'}}` bound the number `80` (`native-sql-datetime-filter` + * pinned that too); it now binds the string the author wrote. The DB's own + * type resolution decides the comparison, which is the whole reason a + * consumer must not guess: SQLite applies the column's numeric affinity, and + * Postgres infers the parameter's type from the column. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; +import { DatasetSchema } from '@objectstack/spec/ui'; + +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; +import { + normalizeAnalyticsFilterTree, + toSqlBindValue, +} from '../strategies/filter-normalizer.js'; +import { AnalyticsService } from '../analytics-service.js'; + +// ── 1. Encode + bind, comparand by comparand ───────────────────────────────── + +/** + * `leaf` is what the tree must carry, `sql` what the SQL path must bind, `engine` + * what `engine.aggregate` must receive. + * + * The two consumers differ in exactly ONE place — a boolean, which SQL spells + * `1`/`0` because better-sqlite3 refuses a JS boolean and the engine needs the + * real thing to match a stored boolean. Every other row carries the same value + * three times over, and that identity IS the invariant: a divergence means + * something re-typed the author's value on one path. + */ +const CASES: Array<{ + name: string; + value: unknown; + leaf: unknown; + sql: unknown; + engine: unknown; + why: string; +}> = [ + // ── #5526's headline: the tokens the old encoder owned ────────────────────── + { + name: "the author string 'null'", + value: 'null', + leaf: 'null', + sql: 'null', + engine: 'null', + why: 'was real NULL on both paths — every comparison UNKNOWN, so the widget could never draw a row', + }, + { + name: "the author string 'true'", + value: 'true', + leaf: 'true', + sql: 'true', + engine: 'true', + why: 'was 1 (SQL) / true (engine) — a text column storing "true" stopped matching', + }, + { + name: "the author string 'false'", + value: 'false', + leaf: 'false', + sql: 'false', + engine: 'false', + why: 'was 0 (SQL) / false (engine)', + }, + + // ── #5528's rows: information `Number()` would have destroyed ────────────── + { name: "'007'", value: '007', leaf: '007', sql: '007', engine: '007', why: 'leading zeros — order number / SKU (was 7 before #5528)' }, + { name: "'0912'", value: '0912', leaf: '0912', sql: '0912', engine: '0912', why: 'leading zero — dialling code (was 912)' }, + { name: "'1.50'", value: '1.50', leaf: '1.50', sql: '1.50', engine: '1.50', why: 'trailing zero — price string (was 1.5)' }, + { name: "'1.0'", value: '1.0', leaf: '1.0', sql: '1.0', engine: '1.0', why: 'trailing zero (was 1)' }, + { name: "'-0'", value: '-0', leaf: '-0', sql: '-0', engine: '-0', why: 'the sign is lost by Number() (was 0)' }, + { + name: "'12345678901234567890'", + value: '12345678901234567890', + leaf: '12345678901234567890', + sql: '12345678901234567890', + engine: '12345678901234567890', + why: 'more digits than a double holds (was 12345678901234567000)', + }, + { + name: "'1000000000000000000000'", + value: '1000000000000000000000', + leaf: '1000000000000000000000', + sql: '1000000000000000000000', + engine: '1000000000000000000000', + why: 'canonical form of 1e21 is exponential, so the digits round-tripped lossily (was 1e21)', + }, + // Strings #5528's regex already refused. They were strings then and are + // strings now — by construction rather than by a regex, which is the change. + { name: "'1e3'", value: '1e3', leaf: '1e3', sql: '1e3', engine: '1e3', why: 'exponent' }, + { name: "'1e+21'", value: '1e+21', leaf: '1e+21', sql: '1e+21', engine: '1e+21', why: 'canonical String(1e21), and still a string' }, + { name: "'+7'", value: '+7', leaf: '+7', sql: '+7', engine: '+7', why: 'leading plus' }, + { name: "' 7'", value: ' 7', leaf: ' 7', sql: ' 7', engine: ' 7', why: 'leading whitespace' }, + { name: "'0x10'", value: '0x10', leaf: '0x10', sql: '0x10', engine: '0x10', why: 'hex' }, + { name: "'Infinity'", value: 'Infinity', leaf: 'Infinity', sql: 'Infinity', engine: 'Infinity', why: 'not finite' }, + { name: "'NaN'", value: 'NaN', leaf: 'NaN', sql: 'NaN', engine: 'NaN', why: 'not finite' }, + { name: "''", value: '', leaf: '', sql: '', engine: '', why: 'the empty string is a value, not a null and not a number' }, + { name: "'won'", value: 'won', leaf: 'won', sql: 'won', engine: 'won', why: 'plain text' }, + // The strings that DID round-trip through #5528's decoder. They are the + // over-correction guard in the other direction: a string spelled like a number + // is still a STRING now, because the author typed a string. + { name: "'7' (canonical numeric spelling)", value: '7', leaf: '7', sql: '7', engine: '7', why: "#5528 recovered this as 7; the author's string is a string" }, + { name: "'80' (canonical numeric spelling)", value: '80', leaf: '80', sql: '80', engine: '80', why: 'the native-sql-datetime-filter case: was 80, now the string' }, + + // ── Real non-string types: unchanged behaviour, now by construction ───────── + { name: 'the number 7', value: 7, leaf: 7, sql: 7, engine: 7, why: 'a number never becomes text' }, + { name: 'the number 1.5', value: 1.5, leaf: 1.5, sql: 1.5, engine: 1.5, why: 'no formatting round trip to lose precision in' }, + { name: 'the number -3', value: -3, leaf: -3, sql: -3, engine: -3, why: 'sign preserved' }, + { name: 'the number 0', value: 0, leaf: 0, sql: 0, engine: 0, why: 'zero is a value, not an absence' }, + { name: 'the number -12.25', value: -12.25, leaf: -12.25, sql: -12.25, engine: -12.25, why: 'no String()/Number() lap at all' }, + { + name: 'the boolean true', + value: true, + leaf: true, + sql: 1, + engine: true, + why: 'the ONE divergence: better-sqlite3 cannot bind a JS boolean; the engine compares against the stored boolean', + }, + { name: 'the boolean false', value: false, leaf: false, sql: 0, engine: false, why: 'mirror of true' }, +]; + +describe('[#5526] a leaf carries the author comparand ITSELF', () => { + for (const c of CASES) { + it(`${c.name} → leaf ${JSON.stringify(c.leaf)}`, () => { + const node = normalizeAnalyticsFilterTree({ where: { code: { $eq: c.value } } }); + expect(node?.kind).toBe('leaf'); + // Exact set, not "contains": the leaf carries one comparand and it is the + // author's. + expect((node as { values: unknown[] }).values, c.why).toEqual([c.leaf]); + }); + } + + it('the leaf value is the SAME REFERENCE for a non-primitive, i.e. nothing re-encoded it', () => { + const when = new Date('2026-03-04T05:06:07.000Z'); + const node = normalizeAnalyticsFilterTree({ where: { closed: { $gte: when } } }) as + | { kind: 'leaf'; values: unknown[] } + | null; + expect(node?.values[0]).toBe(when); + }); +}); + +describe('[#5526] the SQL bind form converts only what a driver cannot bind', () => { + for (const c of CASES) { + it(`${c.name} → binds ${JSON.stringify(c.sql)}`, () => { + expect(toSqlBindValue(c.value), c.why).toEqual(c.sql); + }); + } + + it('the two consumers agree on every comparand EXCEPT a boolean', () => { + // The engine path applies no conversion, so `toSqlBindValue` is the entire + // difference between the two. Anything but a boolean that differs means the + // SQL boundary grew an opinion about a type it should have passed through. + for (const c of CASES) { + if (typeof c.value === 'boolean') { + expect(toSqlBindValue(c.value)).not.toEqual(c.value); + continue; + } + expect(toSqlBindValue(c.value), c.name).toEqual(c.value); + } + }); + + it('a Date binds as canonical ISO text (unbindable object → the one SQL form it has)', () => { + expect(toSqlBindValue(new Date('2026-03-04T05:06:07.000Z'))).toBe('2026-03-04T05:06:07.000Z'); + }); + + it('null binds as NULL, not as the empty string', () => { + // The old encoder wrote `''` for a `null` comparand in an ordering position, + // which is a REAL comparison against the empty string — on a text column it + // matched rows. NULL makes the predicate UNKNOWN, i.e. no rows: the answer + // `driver-memory` and `formula` give, and the fail-closed one. + expect(toSqlBindValue(null)).toBeNull(); + }); + + it('an object comparand binds as JSON rather than failing at the driver', () => { + expect(toSqlBindValue({ a: 1 })).toBe('{"a":1}'); + expect(toSqlBindValue([1, 'x'])).toBe('[1,"x"]'); + }); + + it('`undefined` in a comparand position is normalised to null at the leaf', () => { + // JSON has no `undefined`, so this is not an authorable shape (#5332's + // reading). It must not reach a driver as `undefined` — that is a bind error, + // not a predicate — and `null` is the fail-closed reading. Note the operator + // is still `equals`: only `=== null` is the null PREDICATE. + const node = normalizeAnalyticsFilterTree({ where: { code: { $eq: undefined } } }); + expect(node).toEqual({ kind: 'leaf', member: 'code', operator: 'equals', values: [null] }); + }); +}); + +// ── 2. The SQL consumer: bound params AND row sets, with decoys ────────────── + +interface Row { + id: string; + code: string | null; + score: number; +} + +/** + * Every row here is a DECOY for one of the collisions above: + * + * - `r_7` is what a `'007'` filter used to return once the comparand became the + * integer `7` and SQLite applied the TEXT column's affinity — the wrong row, + * silently. `r_15` does the same for `'1.50'`. + * - `r_nulltext` STORES the text `'null'` while `r_realnull` stores real NULL: + * the old encoding could not tell the author's two intents apart, and + * `{code: {$eq: 'null'}}` reached SQL as `code = NULL`, which is UNKNOWN for + * BOTH of these rows and returned neither. + * - `r_truetext` does the same for `'true'` against `r_1`, which stores `'1'`. + */ +const ROWS: Row[] = [ + { id: 'r_007', code: '007', score: 7 }, + { id: 'r_7', code: '7', score: 7 }, + { id: 'r_150', code: '1.50', score: 1 }, + { id: 'r_15', code: '1.5', score: 1 }, + { id: 'r_nulltext', code: 'null', score: 2 }, + { id: 'r_realnull', code: null, score: 2 }, + { id: 'r_truetext', code: 'true', score: 3 }, + { id: 'r_1', code: '1', score: 3 }, + { id: 'r_80', code: '80', score: 80 }, +]; + +const CUBE: Cube = { + name: 'orders', + title: 'Orders', + sql: 'orders', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: { + id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, + code: { name: 'code', label: 'Code', type: 'string', sql: 'code' }, + score: { name: 'score', label: 'Score', type: 'number', sql: 'score' }, + }, + public: false, +} as unknown as Cube; + +const ROW_CASES: Array<{ name: string; filter: FilterCondition; expected: string[]; note: string }> = [ + { + name: "{code: {$eq: 'null'}} finds the row storing the TEXT 'null' — and only it", + filter: { code: { $eq: 'null' } }, + expected: ['r_nulltext'], + note: "#5526's headline. The bind was real NULL, so `code = NULL` was UNKNOWN for every row and this returned [].", + }, + { + name: "{code: {$eq: 'true'}} finds the row storing the TEXT 'true', not the row storing '1'", + filter: { code: { $eq: 'true' } }, + expected: ['r_truetext'], + note: "The bind was the integer 1, which SQLite compares as '1' against this TEXT column → ['r_1'], the wrong row.", + }, + { + name: "{code: {$eq: 'false'}} finds nothing rather than the row storing '0'", + filter: { code: { $eq: 'false' } }, + expected: [], + note: 'No row stores the text "false". The bind was 0, which matched nothing here either — but for the wrong reason.', + }, + { + name: "{code: {$eq: '007'}} finds the row storing '007'", + filter: { code: { $eq: '007' } }, + expected: ['r_007'], + note: "Before #5528 the bind was the integer 7 and this returned ['r_7'].", + }, + { + name: "{code: {$eq: '1.50'}} finds the row storing '1.50'", + filter: { code: { $eq: '1.50' } }, + expected: ['r_150'], + note: "Before #5528 the bind was 1.5 and this returned ['r_15'].", + }, + { + name: "{code: {$eq: '7'}} still finds the row storing '7'", + filter: { code: { $eq: '7' } }, + expected: ['r_7'], + note: 'A numeric-looking string is now bound as text; on a TEXT column that is the same row it always was.', + }, + { + name: "{code: {$in: ['007', '1.50', 'null', 'true']}} keeps all four spellings", + filter: { code: { $in: ['007', '1.50', 'null', 'true'] } }, + expected: ['r_007', 'r_150', 'r_nulltext', 'r_truetext'], + note: 'Every operator reads the same leaf, so $in gains the fix with $eq.', + }, + { + name: '{score: {$eq: 7}} still matches on the numeric column', + filter: { score: { $eq: 7 } }, + expected: ['r_007', 'r_7'], + note: 'A genuine number comparand is untouched.', + }, + { + name: "{score: {$gte: '80'}} — a STRING comparand on a numeric column still matches", + filter: { score: { $gte: '80' } }, + expected: ['r_80'], + note: "Was bound as 80; now bound as '80'. SQLite applies the INTEGER column's numeric affinity to a TEXT comparand, so the DB decides the comparison — which is why the consumer must not guess.", + }, + { + name: '{code: null} is still the null PREDICATE, not a value comparison', + filter: { code: null }, + expected: ['r_realnull'], + note: "#5332 / #5525's ruling is untouched: a real `null` comparand compiles to IS NULL (notSet) and never enters `values`.", + }, + { + name: '{code: {$eq: null}} is the same predicate as {code: null}', + filter: { code: { $eq: null } }, + expected: ['r_realnull'], + note: 'The #5332 / #5525 pair. Deleting the encoder must not disturb it.', + }, + { + name: '{code: {$ne: null}} is IS NOT NULL, and the TEXT "null" row is one of the rows it keeps', + filter: { code: { $ne: null } }, + expected: ['r_007', 'r_1', 'r_15', 'r_150', 'r_7', 'r_80', 'r_nulltext', 'r_truetext'], + note: 'The two facts the old encoding conflated, from the other side.', + }, +]; + +/** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ +async function locateWasm(): Promise<((file: string) => string) | undefined> { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + const dir = dirname(pkgJsonPath); + return (file: string) => join(dir, 'dist', file); + } catch { + return undefined; + } +} + +describe("[#5526] analytics SQL path — a text column's own spelling is what gets bound", () => { + let db: any; + let ctx: StrategyContext; + let bound: unknown[][]; + + beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + + db = new SQL.Database(); + // `code` is TEXT on purpose: the whole point is a column that STORES the + // author's spelling, which is where a re-typed bind stops matching. + db.run(`CREATE TABLE "orders" ("id" TEXT PRIMARY KEY, "code" TEXT, "score" INTEGER);`); + const insert = db.prepare(`INSERT INTO "orders" ("id","code","score") VALUES (?,?,?)`); + for (const r of ROWS) insert.run([r.id, r.code, r.score]); + insert.free(); + + bound = []; + ctx = { + getCube: (name: string) => (name === 'orders' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object: string, sql: string, params: unknown[]) => { + bound.push(params); + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const out: Record[] = []; + while (stmt.step()) out.push(stmt.getAsObject()); + stmt.free(); + return out; + }, + } as StrategyContext; + }); + + afterAll(() => { + db?.close(); + }); + + for (const c of ROW_CASES) { + it(c.name, async () => { + bound.length = 0; + const result = await new NativeSQLStrategy().execute( + { cube: 'orders', measures: ['total'], dimensions: ['id'], where: c.filter } as AnalyticsQuery, + ctx, + ); + const got = result.rows.map((r) => String(r.id)).sort(); + expect(got, c.note).toEqual(c.expected); + }); + } + + it("binds 'null' / 'true' / '007' as TEXT, and a real boolean as 1", async () => { + const bindsFor = async (where: FilterCondition): Promise => { + bound.length = 0; + await new NativeSQLStrategy().execute( + { cube: 'orders', measures: ['total'], dimensions: ['id'], where } as AnalyticsQuery, + ctx, + ); + return bound[0]; + }; + // Exact param sets — the statement binds one comparand per case, so a stray + // conversion shows up as a different array rather than a missing `toContain`. + expect(await bindsFor({ code: { $eq: 'null' } })).toEqual(['null']); + expect(await bindsFor({ code: { $eq: 'true' } })).toEqual(['true']); + expect(await bindsFor({ code: { $eq: 'false' } })).toEqual(['false']); + expect(await bindsFor({ code: { $eq: '007' } })).toEqual(['007']); + expect(await bindsFor({ code: { $eq: '80' } })).toEqual(['80']); + // Not merely "the rows came back": SQLite's affinity would rescue a '7' bound + // as text on the numeric column, so only the bound TYPE can see this. + expect(await bindsFor({ score: { $eq: 7 } })).toEqual([7]); + expect(await bindsFor({ code: { $eq: true } as never })).toEqual([1]); + expect(await bindsFor({ code: { $eq: false } as never })).toEqual([0]); + }); + + it('a null comparand in an ORDERING position binds NULL, so the widget draws nothing', async () => { + // Uncovered by any ruling before this (#5332 said so explicitly): the encoder + // wrote `''`, i.e. `code > ''`, a real comparison that on a text column + // returned rows. NULL is UNKNOWN for every row — no rows, no accident. + bound.length = 0; + const result = await new NativeSQLStrategy().execute( + { cube: 'orders', measures: ['total'], dimensions: ['id'], where: { code: { $gt: null } } } as AnalyticsQuery, + ctx, + ); + expect(bound[0]).toEqual([null]); + expect(result.rows).toEqual([]); + }); +}); + +// ── 3. The LIKE family: its comparand is declared a `string` ───────────────── + +describe('[#5526] the LIKE family stringifies at the emitter, on all three emitters', () => { + const nativeParams = async (where: FilterCondition): Promise => { + const ctx = { + getCube: (name: string) => (name === 'orders' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + } as unknown as StrategyContext; + return (await new NativeSQLStrategy().generateSql( + { cube: 'orders', measures: ['total'], where } as AnalyticsQuery, + ctx, + )).params; + }; + const echoParams = async (where: FilterCondition): Promise => { + const ctx = { + getCube: (name: string) => (name === 'orders' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => [], + } as unknown as StrategyContext; + return (await new ObjectQLStrategy().generateSql( + { cube: 'orders', measures: ['total'], where } as AnalyticsQuery, + ctx, + )).params; + }; + /** The operand the engine receives, via the private converter. */ + const engineOperand = (where: FilterCondition): unknown => { + const node = normalizeAnalyticsFilterTree({ where }) as { + operator: string; + values: unknown[]; + }; + const strategy = new ObjectQLStrategy() as unknown as { + convertFilter(operator: string, values?: unknown[]): unknown; + }; + return strategy.convertFilter(node.operator, node.values); + }; + + it('a string comparand is unchanged — the escaping contract (#5567) is untouched', async () => { + expect(await nativeParams({ code: { $contains: 'a_b' } })).toEqual(['%a\\_b%', '\\']); + expect(await echoParams({ code: { $contains: 'a_b' } })).toEqual(['%a\\_b%', '\\']); + expect(engineOperand({ code: { $contains: 'a_b' } })).toEqual({ $contains: 'a_b' }); + }); + + it('a NUMBER comparand becomes its String() form — the same one driver-sql applyLike uses', async () => { + // `filter.zod.ts` declares `$contains: z.string()`, so a number here is + // off-contract input. It is stringified rather than dropped (a dropped + // predicate WIDENS — #3948 / #4128) and rather than refused, because + // `driver-sql`'s `applyLike` does `String(value)` too: refusing on one face + // only would fork what `{$contains: 5}` means by which face answered. The + // shared leniency is filed separately, not decided here. + expect(await nativeParams({ code: { $contains: 5 } } as unknown as FilterCondition)).toEqual(['%5%', '\\']); + expect(engineOperand({ code: { $contains: 5 } } as unknown as FilterCondition)).toEqual({ $contains: '5' }); + }); + + it("a null comparand becomes '%null%' — narrower than the '%%' it used to be", async () => { + // The old encoder wrote `''` for null, so this compiled to `LIKE '%%'` and + // matched EVERY non-NULL row: a silent widening, in a file whose whole + // subject is silent widening. `String(null)` is `'null'`, which is what + // `driver-sql` has always compiled — so this converges as it narrows. + expect(await nativeParams({ code: { $contains: null } } as unknown as FilterCondition)).toEqual(['%null%', '\\']); + expect(engineOperand({ code: { $contains: null } } as unknown as FilterCondition)).toEqual({ $contains: 'null' }); + }); + + it('the echo binds what execution binds, for every LIKE shape', async () => { + for (const where of [ + { code: { $contains: 'a_b' } }, + { code: { $startsWith: '50%' } }, + { code: { $endsWith: 'x\\y' } }, + { code: { $notContains: '_admin' } }, + ] as FilterCondition[]) { + expect(await echoParams(where), JSON.stringify(where)).toEqual(await nativeParams(where)); + } + }); +}); + +// ── 4. The ObjectQL consumer: the comparand handed to the engine ───────────── + +const dataset = DatasetSchema.parse({ + name: 'orders', + label: 'Orders', + object: 'order', + dimensions: [{ name: 'code', field: 'code', type: 'string' }], + measures: [{ name: 'order_count', aggregate: 'count' }], +}); + +/** Stored rows carry the author's STRINGS, exactly as the engine would hold them. */ +const ENGINE_ROWS: Array<{ code: unknown }> = [ + { code: '007' }, + { code: '7' }, + { code: '1.50' }, + { code: 'null' }, + { code: null }, + { code: 'true' }, + { code: true }, +]; + +/** + * A stand-in for `engine.aggregate` that filters with STRICT equality, the way + * the real engine compares against a stored value: a comparand re-typed to `7` / + * `null` / `true` matches a DIFFERENT row than the author asked for, or none. + */ +function makeEngine(captured: Array | undefined>) { + return async ( + _object: string, + options: { groupBy?: string[]; filter?: Record }, + ): Promise>> => { + captured.push(options.filter); + const filtered = ENGINE_ROWS.filter((row) => + Object.entries(options.filter ?? {}).every( + ([field, cond]) => (row as Record)[field] === cond, + ), + ); + return [{ order_count: filtered.length }]; + }; +} + +describe('[#5526] analytics engine path — the comparand reaches engine.aggregate verbatim', () => { + const run = async (comparandValue: unknown) => { + const captured: Array | undefined> = []; + const svc = new AnalyticsService({ + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: makeEngine(captured), + }); + const result = await svc.queryDataset!(dataset, { + measures: ['order_count'], + runtimeFilter: { code: { $eq: comparandValue } } as FilterCondition, + }); + return { captured, result }; + }; + + for (const v of ['007', '1.50', 'null', 'true', '7']) { + it(`passes the string ${JSON.stringify(v)} through, and counts exactly the row that stores it`, async () => { + const { captured, result } = await run(v); + expect(captured[0]?.code).toBe(v); + // Exactly one stored row carries each of these spellings, so `1` here is an + // exact-set assertion over a fixture that also holds the decoys (real + // `null`, real `true`, the numeric-looking `'7'`). + expect(result.rows).toEqual([{ order_count: 1 }]); + }); + } + + it('still hands the engine a real number for a number comparand', async () => { + const { captured } = await run(7); + expect(captured[0]?.code).toBe(7); + }); + + it('still hands the engine a real boolean for a boolean comparand', async () => { + const { captured, result } = await run(true); + expect(captured[0]?.code).toBe(true); + // The stored real `true` row, NOT the row storing the text 'true'. + expect(result.rows).toEqual([{ order_count: 1 }]); + }); + + it('a real null comparand is still the null predicate, not a value', async () => { + const { captured, result } = await run(null); + // `convertFilter` maps `notSet` to a bare `null` — the spelling every driver + // reads as IS NULL (#5332 / #5525), reached without entering `values`. + expect(captured[0]).toEqual({ code: null }); + expect(result.rows).toEqual([{ order_count: 1 }]); + }); +}); + +// ── 5. `dateRange` binds at its DECLARED type ──────────────────────────────── + +describe('[#5526] a timeDimension dateRange binds at the type the spec declares (string)', () => { + const engineFilterFor = async (dateRange: string[]): Promise | undefined> => { + const captured: Array | undefined> = []; + const ctx = { + getCube: (name: string) => (name === 'orders' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async (_o: string, options: { filter?: Record }) => { + captured.push(options.filter); + return []; + }, + } as unknown as StrategyContext; + await new ObjectQLStrategy().execute( + { + cube: 'orders', + measures: ['total'], + timeDimensions: [{ dimension: 'code', dateRange }], + } as AnalyticsQuery, + ctx, + ); + return captured[0]; + }; + + it('forwards ISO bounds as the strings they are', async () => { + expect(await engineFilterFor(['2026-01-01', '2026-01-31'])).toEqual({ + code: { $gte: '2026-01-01', $lte: '2026-01-31' }, + }); + }); + + it('a numeric-looking bound stays a string too', async () => { + // `coerceFilterValueForObjectQL` used to recover this as the number + // 1750000000000 — a lenient consumer rescuing a shape + // `AnalyticsQuerySchema` does not declare (`dateRange: string[]`). An + // epoch-ms window would have to be declared at the producer or in the spec, + // not guessed here (Prime Directive #12). + expect(await engineFilterFor(['1750000000000', '1750000000001'])).toEqual({ + code: { $gte: '1750000000000', $lte: '1750000000001' }, + }); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter.test.ts b/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter.test.ts index e7589abbaf..e067ca555b 100644 --- a/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter.test.ts +++ b/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter.test.ts @@ -203,7 +203,20 @@ describe('NativeSQLStrategy — datetime filter storage coercion', () => { where: { score: { $gte: '80' } }, }; const { params } = await strategy.generateSql(query, ctx); - // hook returns the string unchanged → falls back to numeric recovery - expect(params).toEqual([80]); + // The hook returns the value unchanged for a non-temporal column, and the + // fallback binds it as the author wrote it. + // + // [#5526] This assertion used to read `[80]`. It was pinning the DECODER + // half of the deleted `values: string[]` round trip: the fallback was + // `coerceFilterValueForSql`, which re-read a numeric-LOOKING string as a + // number — the same guess that bound `'007'` as `7` against a TEXT column + // and drew "no data". A string comparand now binds as a string, and the + // comparison is decided by the database's own type resolution (SQLite + // applies this INTEGER column's numeric affinity to it; Postgres infers the + // parameter type from the column). What this test still proves is unchanged + // and is why it stays here: the temporal hook does not touch a non-temporal + // column. The full comparand-type table lives in + // `filter-value-type-fidelity.test.ts`. + expect(params).toEqual(['80']); }); }); diff --git a/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts b/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts index 23a629a098..5b70d9c0f1 100644 --- a/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts +++ b/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts @@ -352,7 +352,9 @@ describe('[#5333] `/analytics/sql` echo — every authorable operator renders a buildFilterClauseSql( col: string, operator: string, - values: string[] | undefined, + // [#5526] `unknown[]`: a leaf carries the author's comparand at its + // own type, so this local mirror of the private signature does too. + values: unknown[] | undefined, params: unknown[], ): string | null; }; @@ -388,7 +390,9 @@ describe('[#5333] `/analytics/sql` echo — every authorable operator renders a buildFilterClauseSql( col: string, operator: string, - values: string[] | undefined, + // [#5526] `unknown[]`: a leaf carries the author's comparand at its + // own type, so this local mirror of the private signature does too. + values: unknown[] | undefined, params: unknown[], ): string | null; }; diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index 9cb5591932..0189d2d99b 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -113,25 +113,60 @@ * still described the old emitter would have negated an always-false conjunction * and answered `{$not: {stage: {$eq: null}}}` with every row. * - * # A STRING comparand keeps its spelling (#5528, partial) - * - * #5332 is about the ENCODER ({@link stringifyForCube}); the same round trip has - * a DECODER — {@link recoverNumber}, behind {@link coerceFilterValueForSql} and - * {@link coerceFilterValueForObjectQL} — and it was guessing "this is a number" - * from the string's shape alone. So `{code: {$eq: '007'}}` bound the integer `7` - * and `{price: {$eq: '1.50'}}` bound `1.5`, on both consumers: against a TEXT - * column that is zero rows on SQLite and a type error on Postgres, reported to - * the author as "no data" (#5526's measured table). - * - * Recovery is now limited to a number's own canonical spelling - * (`String(Number(s)) === s`), which is exactly what a real number comparand - * produces on the way out — so `7` → `'7'` → `7` still works, while a string - * that `Number()` would rewrite is kept as the author wrote it. - * - * This is a STOPGAP, and named as one: `values: string[]` still has no escape, - * so the author strings `'null'` / `'true'` / `'false'` still collide with the - * tokens the encoder writes for the real values. Fixing the round trip itself — - * tagged values, or an `unknown[]` internal representation — is #5526. + * # A comparand keeps its own TYPE — there is no round trip any more (#5526) + * + * A leaf's `values` used to be `string[]`, so every comparand was encoded to a + * string on the way in (`stringifyForCube`) and GUESSED back into a type on the + * way out (`recoverNumber`, behind `coerceFilterValueForSql` / + * `coerceFilterValueForObjectQL`). An encoding whose alphabet is "all strings" + * and whose decoder is "does this string look like a number/boolean/null" has no + * escape, so author strings COLLIDED with the tokens the encoder wrote for other + * types. Measured on `main` (#5526's table), for `{code: {$eq: v}}`: + * + * | author's `v` | bound (SQL) | bound (engine) | + * |---|---|---| + * | `'007'` | `7` (#5528: fixed) | `7` (#5528: fixed) | + * | `'1.50'` | `1.5` (#5528: fixed)| `1.5` (#5528: fixed)| + * | `'null'` | real `NULL` | real `null` | + * | `'true'` | `1` | `true` | + * + * Every row of that table is one defect: a TEXT column storing the author's + * spelling stops matching. `'007'` on SQLite compares an integer against a TEXT + * column and is never equal; on Postgres `text = integer` is a type error. The + * `'null'` row is worse than empty — a comparison against real NULL is UNKNOWN + * for every row, so the widget can never draw anything. Zero-padded strings, + * `'true'`/`'false'` as enum-ish codes and `'null'` as a literal label are all + * ordinary business shapes (order numbers, SKUs, postcodes, dialling codes). + * + * #5528 narrowed the number half of the decoder (canonical spelling only) as a + * STOPGAP and said so; this is the ruled fix. `values` is now `unknown[]`: the + * comparand the author wrote travels through the tree untouched, and no + * stringification happens at all except where a boundary genuinely demands it: + * + * - {@link toSqlBindValue} — the ONLY survivor, and it is one-way (a value → + * its SQL bind form), never a decoder. It exists because a SQL driver cannot + * bind every JS type: better-sqlite3 refuses a `boolean`, a `Date` and a + * plain object. Nothing about it inspects a string. + * - the LIKE family, whose comparand `filter.zod.ts` declares a `string` + * (`$contains: z.string()`), so `like-pattern.ts` stringifies at the emitter + * — the same `String(value)` `driver-sql`'s `applyLike` applies, which is + * what keeps one `$contains` meaning one thing on both faces. + * + * The ObjectQL path needs NO conversion at all now: the engine compares against + * the stored runtime type, and the value it receives is the author's own. + * + * Two shapes changed reading as a consequence, both toward fail-closed and both + * pinned in `filter-value-type-fidelity.test.ts`: + * + * - `{name: {$contains: null}}` compiled to `LIKE '%%'` — matching EVERY + * non-NULL row — because `stringifyForCube(null)` was `''`. It is now + * `LIKE '%null%'`, which is what `driver-sql` has always compiled it to. + * - `{amount: {$gt: null}}` compiled to `amount > ''`, a real comparison + * against the empty string. It now binds NULL, so the predicate is UNKNOWN + * and the widget draws nothing — the honest answer for an unordered + * comparand, and the one `driver-memory` / `formula` give. (#5332 named this + * comparand position as covered by no ruling and left the `''` placeholder + * alone; deleting the encoder decides it by construction.) * * # A `where` ARRAY is lowered here, not dropped (#5334) * @@ -174,8 +209,10 @@ * SHARED combinator table (`FILTER_LOGIC_CASES`, #3774) that the SQL compiler, * the in-memory matcher, `formula` and `read-scope-sql` are already held to, * `filter-normalizer-not-null-safe.test.ts` for the two squares that table - * deliberately does not carry (NULL handling, boolean identities), and - * `filter-array-lowering.test.ts` for the array door (#5334). + * deliberately does not carry (NULL handling, boolean identities), + * `filter-array-lowering.test.ts` for the array door (#5334), and + * `filter-value-type-fidelity.test.ts` for what each comparand TYPE binds on both + * consumers (#5526, carrying #5528's cases forward as end-to-end assertions). */ import { isFilterAST, parseFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data'; @@ -184,7 +221,8 @@ import { StandardErrorCode } from '@objectstack/spec/api'; export interface NormalizedAnalyticsFilter { member: string; operator: string; - values: string[]; + /** The author's comparands, at their own types — see {@link NormalizedFilterNode}. */ + values: unknown[]; } // ── [#5334 / #5352] The refusal envelope ───────────────────────────────────── @@ -240,32 +278,27 @@ const MONGO_TO_CUBE_OP: Record = { }; /** - * Stringify a filter value as the internal pipeline requires `values: string[]`. - * - * Booleans serialize as the tokens `'true'`/`'false'` (NOT `'1'`/`'0'`) so the - * boolean identity survives the string roundtrip: the consuming strategies can - * recover a real boolean for the ObjectQL engine (which compares against the - * stored boolean type) while still binding `1`/`0` for SQL. Stringifying to - * `'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. + * The comparand a leaf carries: the author's value, at the author's type. + * + * [#5526] This function is what used to be `stringifyForCube`, and the whole of + * its former body is gone: `values` is `unknown[]`, so a comparand needs no + * encoding and there is nothing for a decoder downstream to guess at. What + * remains is one normalisation, and it is not a type conversion: + * + * `undefined` becomes `null`. JSON has no `undefined`, so no authored + * `FilterCondition` can carry one — `{$eq: undefined}` is a key the author did + * not mean to write (#5332's reading, unchanged here) — while a `values` entry + * that IS `undefined` is a bind error on better-sqlite3 rather than a predicate. + * `null` is the fail-closed reading: the comparison is UNKNOWN, so the widget + * draws nothing instead of drawing rows chosen by an accident. + * + * Note what this does NOT do: `{$eq: undefined}` still compiles to an `equals` + * leaf, not to `notSet`. Only `=== null` is the null PREDICATE (#5332's identity + * test, which this module reads at the operator branch, above this function), + * and widening it to `== null` here would re-decide that ruling sideways. */ -function stringifyForCube(v: unknown): string { - if (v == null) return ''; - if (typeof v === 'boolean') return v ? 'true' : 'false'; - if (v instanceof Date) return v.toISOString(); - if (typeof v === 'object') return JSON.stringify(v); - return String(v); +function comparand(v: unknown): unknown { + return v === undefined ? null : v; } /** @@ -286,9 +319,19 @@ function stringifyForCube(v: unknown): string { * emitting nothing, which every compiler reads as "no constraint", i.e. the * opposite. TRUE keeps its existing spelling (`null` = no constraint, the AND * identity); FALSE needs a node because it must survive into the WHERE clause. + * + * [#5526] A leaf's `values` is `unknown[]`, not `string[]`. The author's + * comparand travels at its own type: a number stays a number, a boolean a + * boolean, and — the defect this fixed — a STRING stays the string the author + * typed, so `'007'` is never the integer `7` and `'null'` is never real NULL. + * The compilers of this tree convert only where their own boundary forces it + * ({@link toSqlBindValue} for a SQL parameter, `like-pattern.ts` for the LIKE + * family, whose comparand the spec declares a `string`); the ObjectQL engine path + * converts nothing, because the engine compares against the stored runtime type + * and the value it is handed is the author's own. See the module header. */ export type NormalizedFilterNode = - | { kind: 'leaf'; member: string; operator: string; values: string[] } + | { kind: 'leaf'; member: string; operator: string; values: unknown[] } | { kind: 'const'; value: boolean } | { kind: 'and'; children: NormalizedFilterNode[] } | { kind: 'or'; children: NormalizedFilterNode[] } @@ -347,7 +390,7 @@ function andOf(children: NormalizedFilterNode[]): NormalizedFilterNode | null { */ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { const out: NormalizedFilterNode[] = []; - const leaf = (operator: string, values: string[]): void => { + const leaf = (operator: string, values: unknown[]): void => { out.push({ kind: 'leaf', member: key, operator, values }); }; @@ -403,8 +446,8 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { `${JSON.stringify(v)}. Dropping the predicate would silently widen the query to every row.`, ); } - leaf('gte', [stringifyForCube(v[0])]); - leaf('lte', [stringifyForCube(v[1])]); + leaf('gte', [comparand(v[0])]); + leaf('lte', [comparand(v[1])]); continue; } @@ -442,8 +485,10 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { // 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. + // write). [#5526] `comparand`'s `undefined` → `null` normalisation + // deliberately does NOT widen this test to `== null`: it makes the VALUE + // bindable, while this branch decides what the operator MEANS, and the + // meaning is #5332's to change, not a side effect of deleting an encoder. if ((opKey === '$eq' || opKey === '$ne') && wrapper[opKey] === null) { leaf(opKey === '$eq' ? 'notSet' : 'set', []); continue; @@ -479,7 +524,7 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { ); } const v = wrapper[opKey]; - leaf(cubeOp, Array.isArray(v) ? v.map(stringifyForCube) : [stringifyForCube(v)]); + leaf(cubeOp, Array.isArray(v) ? v.map(comparand) : [comparand(v)]); } return out; } @@ -495,8 +540,8 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { // explicit `{$in: []}` spelling is — see the note at that branch. if (Array.isArray(raw)) { if (raw.length === 0) out.push({ kind: 'const', value: false }); - else leaf('in', raw.map(stringifyForCube)); - } else leaf('equals', [stringifyForCube(raw)]); + else leaf('in', raw.map(comparand)); + } else leaf('equals', [comparand(raw)]); return out; } @@ -935,95 +980,42 @@ export function collectFilterLeaves( } /** - * Recover a finite number from a token that is a number's OWN canonical - * spelling — `String(Number(s)) === s` — else undefined. - * - * This is the decoder half of the `values: string[]` round trip - * {@link stringifyForCube} encodes into, and it is a LAST RESORT by design: - * ADR-0053 D-A2 demoted textual type re-derivation behind the driver-backed - * `coerceTemporalFilterValue` hook, leaving this function only the - * boolean/number recovery for non-temporal columns. Guessing a type from a - * string's shape can only ever be a guess, so the narrower the guess, the fewer - * author values it can overwrite. - * - * # Why canonical form, not "looks numeric" (#5528, route C of #5526) - * - * The test used to be the SHAPE alone (`/^-?\d+(\.\d+)?$/`), which cannot tell - * a number that was stringified on the way out from a string the author wrote. - * `'007'` came back as `7` and `'1.50'` as `1.5` — on BOTH consumers (the SQL - * bind in `native-sql-strategy` and the engine comparand in - * `objectql-strategy`) — so a widget filtered `{code: {$eq: '007'}}` compared an - * INTEGER against a TEXT column: zero rows on SQLite (cross-type compare is - * never equal), a type error on Postgres. Silent, and drawn as "no data". - * - * Zero-padded and trailing-zero strings are ordinary business shapes — order - * numbers, work orders, SKUs, dialling codes, postcodes, `'1.50'` prices — not - * constructed edge cases (#5526's measured table). - * - * The canonical-form test separates the two cases without needing a type: - * - * - a comparand that REALLY was a number arrives as `String(n)` by - * construction, so it round-trips exactly and is still recovered - * (`7` → `'7'` → `7`, `1.5` → `'1.5'` → `1.5`, `-3` → `'-3'` → `-3`); - * - a string that survived `Number()` with information LOST — a leading zero, - * a trailing zero, `'-0'`, more digits than a double can hold — cannot have - * come from a number, so it is the author's string and stays one. - * - * The shape regex is kept AHEAD of the round-trip test so this change can only - * ever NARROW what is recovered: `'1e3'`, `'1e+21'`, `'+7'`, `' 7'`, `'0x10'`, - * `'Infinity'` and `'NaN'` were strings before and are strings still, even - * though some of them are canonical `String(Number(…))` output. - * - * ⛔ **Not fixed here, on purpose.** `'null'` / `'true'` / `'false'` still - * collide with the tokens {@link stringifyForCube} writes for the real `null` - * and booleans, so those three author strings still decode to non-strings. That - * collision is not a bad `if` — it is the `string[]` encoding having no escape, - * i.e. the root cause #5526 exists to rule on (tagged encoding, or an - * `unknown[]` internal representation). This function only stops the shapes that - * are unambiguously lossy from being downgraded; it does not make the round trip - * lossless. - */ -function recoverNumber(s: string): number | undefined { - if (!/^-?\d+(\.\d+)?$/.test(s)) return undefined; - const n = Number(s); - if (!Number.isFinite(n)) return undefined; - // The round-trip test: only a string that IS `n`'s canonical spelling carries - // no writing `Number()` threw away, so only that string can be read as `n`. - if (String(n) !== s) return undefined; - return n; -} - -/** - * Coerce a stringified filter value back into a runtime type for SQL - * parameter binding. Better-sqlite3 (and most drivers) cannot bind a JS - * boolean, so booleans are recovered as `1`/`0` integers; numbers are - * recovered as numbers — avoiding string-vs-number mismatches against typed - * columns. - * - * "Numbers" means only a number's own canonical spelling — see - * {@link recoverNumber} for why `'007'` and `'1.50'` bind as TEXT (#5528). + * [#5526] Put one comparand into a form a SQL driver can BIND. One-way, and the + * only stringification left in this module's value path. + * + * This replaces `coerceFilterValueForSql` / `coerceFilterValueForObjectQL`, and + * the difference is the whole of #5526: those two were DECODERS — they received a + * string and guessed which type it had been before `stringifyForCube` flattened + * it, so `'007'` became `7`, `'null'` became real NULL and `'true'` became `1`, + * whatever the author meant. Nothing here inspects a string. A `string` comparand + * is returned untouched, always, because a `string` is already bindable; only the + * JS types a driver CANNOT bind are converted, each to the one form SQL has for + * it: + * + * - `boolean` → `1` / `0`. better-sqlite3 refuses a JS boolean outright + * ("can only bind numbers, strings, bigints, buffers, and null"), and `1`/`0` + * is how every dialect these strategies target spells a bit. The ObjectQL + * path deliberately does NOT do this — the engine compares against the + * STORED boolean, where `1` never matches `true` (the regression + * `objectql-strategy-boolean-filter.test.ts` guards). + * - `Date` → canonical UTC ISO text. A comparand on a temporal column has + * normally been through `StrategyContext.coerceTemporalFilterValue` (the + * driver's own storage convention, ADR-0053 D-A2) before it gets here; this + * arm is the fallback for the hookless / non-temporal case, where an + * unbindable object would otherwise reach the driver. + * - any other object / array → JSON text. Not a meaningful comparison on any + * column, but the shape `filter.zod.ts` cannot exclude, and a driver-level + * bind error tells the author nothing about their filter. + * + * `number`, `bigint`, `null` and `string` pass through — `null` included, and + * that is deliberate: `col > NULL` is UNKNOWN, so the widget draws nothing. It is + * the honest answer for an unordered comparand and the one the JS backends give; + * the `''` this used to bind was a real comparison against the empty string, + * which on a text column silently matched rows (see the module header). */ -export function coerceFilterValueForSql(s: string): unknown { - if (s === 'true') return 1; - if (s === 'false') return 0; - if (s === 'null') return null; - return recoverNumber(s) ?? s; -} - -/** - * Coerce a stringified filter value back into a runtime type for the ObjectQL - * aggregate engine. Unlike the SQL path, the engine compares against the - * *stored* runtime type, so a boolean field holds a real `true`/`false` — bind - * the boolean itself, NOT `1`/`0`, or the equality never matches. - * - * The number recovery is the SAME canonical-form test the SQL path uses - * ({@link recoverNumber}): the two consumers differ in how they spell a boolean, - * never in which strings they consider numbers, so `{code: {$eq: '007'}}` binds - * the string `'007'` on both paths (#5528). - */ -export function coerceFilterValueForObjectQL(s: string): unknown { - if (s === 'true') return true; - if (s === 'false') return false; - if (s === 'null') return null; - return recoverNumber(s) ?? s; +export function toSqlBindValue(v: unknown): unknown { + if (typeof v === 'boolean') return v ? 1 : 0; + if (v instanceof Date) return v.toISOString(); + if (v !== null && typeof v === 'object') return JSON.stringify(v); + return v; } diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 2817ec964a..f5d131d7aa 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -5,7 +5,7 @@ import type { Cube } from '@objectstack/spec/data'; import type { AnalyticsStrategy, StrategyContext } from './types.js'; import { normalizeAnalyticsFilterTree, - coerceFilterValueForSql, + toSqlBindValue, SQL_CONST_FALSE, SQL_CONST_TRUE, type NormalizedFilterNode, @@ -525,14 +525,22 @@ export class NativeSQLStrategy implements AnalyticsStrategy { * driver-backed `coerceTemporalFilterValue` hook (single source of truth for * the date/datetime storage convention — see StrategyContext); when the hook * is absent, or returns the value unchanged (the field is not a temporal - * column, or the dialect stores it as a native timestamp), falls back to the - * generic boolean/number recovery so non-temporal typed columns still bind - * correctly. + * column, or the dialect stores it as a native timestamp), falls back to + * {@link toSqlBindValue} so an unbindable JS type still reaches the driver as + * something it can bind. + * + * [#5526] `value` is `unknown`, not `string`, because a leaf now carries the + * author's comparand at its own type. Both halves of this method were already + * `unknown`-typed for it: the hook's contract is + * `coerceTemporalFilterValue(object, field, value: unknown)` and the fallback + * converts only what a driver cannot bind. What CHANGED is that a string is no + * longer re-typed on the way out — the fallback used to be + * `coerceFilterValueForSql`, which read `'007'` as the integer `7`. */ private coerceTemporal( ctx: StrategyContext, target: { object: string; field: string }, - value: string, + value: unknown, ): unknown { if (typeof ctx.coerceTemporalFilterValue === 'function') { const coerced = ctx.coerceTemporalFilterValue(target.object, target.field, value); @@ -540,7 +548,7 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // columns; only short-circuit when it actually changed the value. if (coerced !== value) return coerced; } - return coerceFilterValueForSql(value); + return toSqlBindValue(value); } /** @@ -653,7 +661,12 @@ export class NativeSQLStrategy implements AnalyticsStrategy { private buildFilterClause( rawCol: string, operator: string, - values: string[] | undefined, + // [#5526] `unknown[]`: the author's comparands, at their own types. Every + // conversion below is one a BOUNDARY demands — `likePattern` because + // `filter.zod.ts` declares the LIKE comparand a `string`, `coerceTemporal` + // because a driver cannot bind every JS type — never a guess about which + // type a string "really" was. + values: unknown[] | undefined, params: unknown[], ctx: StrategyContext, target: { object: string; field: string }, diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index f412dd9799..22af5b80e3 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -6,7 +6,6 @@ import type { AnalyticsStrategy, StrategyContext } from './types.js'; import { normalizeAnalyticsFilterTree, collectFilterLeaves, - coerceFilterValueForObjectQL, SQL_CONST_FALSE, SQL_CONST_TRUE, type NormalizedFilterNode, @@ -631,9 +630,16 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * Render one normalized filter as a display SQL predicate for `generateSql`. * * Mirrors `NativeSQLStrategy.buildFilterClause`'s operator vocabulary so the - * two previews read alike, but binds through `coerceFilterValueForObjectQL`: - * the comparand shown is the one THIS path actually hands the engine (a real - * boolean, not SQL's 1/0). + * two previews read alike, but binds the comparand VERBATIM: the value shown is + * the one THIS path actually hands the engine (a real boolean, not SQL's 1/0). + * + * [#5526] "Verbatim" is now literal. This used to bind through + * `coerceFilterValueForObjectQL`, which decoded the string a `string[]` leaf + * carried back into a type — so an echo could show `7` for a filter the author + * wrote as `'007'`. A leaf carries the author's value at its own type, so the + * echo needs no conversion at all to stay honest about execution. The LIKE + * family is still the one exception, for the reason `filter.zod.ts` gives: its + * comparand is declared a `string`, and what binds is the PATTERN. * * `null` means "this leaf carries no predicate" — a value-less scalar leaf, * which `execute()` and `NativeSQLStrategy` drop too. It does NOT mean "I could @@ -643,7 +649,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { private buildFilterClauseSql( col: string, operator: string, - values: string[] | undefined, + values: unknown[] | undefined, params: unknown[], ): string | null { if (operator === 'set') return `${col} IS NOT NULL`; @@ -653,15 +659,14 @@ export class ObjectQLStrategy implements AnalyticsStrategy { if (operator === 'in' || operator === 'notIn') { const placeholders = values - .map((v) => { params.push(coerceFilterValueForObjectQL(v)); return `$${params.length}`; }) + .map((v) => { params.push(v); return `$${params.length}`; }) .join(', '); return `${col} ${operator === 'in' ? 'IN' : 'NOT IN'} (${placeholders})`; } - // The LIKE family binds its PATTERN, which is text by construction, so it - // skips `coerceFilterValueForObjectQL` — same reason `NativeSQLStrategy` - // keeps the un-normalised column reference for these: a prefix/suffix/ - // substring match reads the column as stored. + // The LIKE family binds its PATTERN, which is text by construction — same + // reason `NativeSQLStrategy` keeps the un-normalised column reference for + // these: a prefix/suffix/substring match reads the column as stored. const like = LIKE_SQL_OPS[operator]; if (like) { // [#5567] Escaped pattern + an explicit `ESCAPE`, matching what @@ -702,7 +707,7 @@ export class ObjectQLStrategy implements AnalyticsStrategy { `one that ran (#5333).`, ); } - params.push(coerceFilterValueForObjectQL(values[0])); + params.push(values[0]); return `${col} ${op} $${params.length}`; } @@ -974,9 +979,16 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * performs the same half-open translation itself because it binds into raw * SQL, so one dashboard reads the same on every driver. * - * Comparands are coerced by the SAME helper the `where` path uses, so an - * epoch-ms bound recovers as a number and an ISO string stays a string. No - * STORAGE coercion happens here, deliberately: `NativeSQLStrategy` needs + * [#5526] Bounds are forwarded at the type `dateRange` is DECLARED with — + * `string` (`AnalyticsQuerySchema`'s `timeDimensions[].dateRange: string[]`) — + * and nothing re-types them. They used to pass through + * `coerceFilterValueForObjectQL`, whose TSDoc advertised that "an epoch-ms + * bound recovers as a number"; that was a lenient CONSUMER rescuing a shape the + * contract does not declare, and the same guess is what read a `'007'` filter + * comparand as `7` (Prime Directive #12 — the producer or the spec is where an + * epoch-ms window would have to be declared, not here). An author who wants an + * instant window writes it as one; a declared `string` binds as a string. No + * STORAGE coercion happens here either, deliberately: `NativeSQLStrategy` needs * `coerceTemporal` because it binds into raw SQL and had to learn that a * SQLite `Field.datetime` is an INTEGER epoch (#2034); this path goes through * `engine.aggregate()`, where the driver's own CRUD filter coercion applies — @@ -1006,22 +1018,40 @@ export class ObjectQLStrategy implements AnalyticsStrategy { if (start == null) continue; out.push({ field: this.resolveFieldName(cube, td.dimension, 'dimension'), - bounds: { - $gte: coerceFilterValueForObjectQL(String(start)), - $lte: coerceFilterValueForObjectQL(String(end)), - }, + bounds: { $gte: start, $lte: end }, }); } return out; } - private convertFilter(operator: string, values?: string[]): unknown { + /** + * One leaf as the operand the engine's `FilterCondition` expects. + * + * [#5526] The comparand is passed through UNCONVERTED. That is the whole of + * this path's share of the fix: the engine compares against the value as + * STORED, and a leaf now carries the value the author wrote, so `'007'` stays + * `'007'`, `true` stays `true` and `7` stays `7` with nothing in between to + * re-type them. The two `coerceFilterValueForObjectQL` calls this replaced + * existed only to undo `stringifyForCube`, and undoing it required guessing. + * + * The four LIKE-family arms are the exception, and a contract one: + * `filter.zod.ts` declares `$contains` / `$notContains` / `$startsWith` / + * `$endsWith` as `z.string()`, so this PRODUCER must hand the engine a real + * string — `String(…)`, the same normalisation `like-pattern.ts` applies at the + * two SQL emitters and `driver-sql`'s `applyLike` applies at the driver, so one + * `$contains` means one thing on every face (#5567's invariant). + */ + private convertFilter(operator: string, values?: unknown[]): unknown { if (operator === 'set') return { $ne: null }; if (operator === 'notSet') return null; if (!values || values.length === 0) return undefined; - const v0 = coerceFilterValueForObjectQL(values[0]); - const all = values.map(coerceFilterValueForObjectQL); + const v0 = values[0]; + // A COPY, not the leaf's own array: the `$in` / `$nin` operand below travels + // into the filter object the engine receives, and a node of this tree is + // never shared (see `falseNode`). The old `values.map(coerce…)` copied as a + // side effect of converting; dropping the conversion must not drop the copy. + const all = [...values]; switch (operator) { case 'equals': return v0; case 'notEquals': return { $ne: v0 }; @@ -1052,15 +1082,15 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // // `MONGO_TO_CUBE_OP` maps `$contains` → `contains` and nothing else does, // so returning `$contains` here is the round trip of the author's own key. - case 'contains': return { $contains: values[0] }; + case 'contains': return { $contains: String(v0) }; // `notContains` had no arm and fell to the `default` below, which returns // a BARE VALUE — i.e. `{field: 'x'}`, an equality. "does not contain x" // was compiled as "equals x". These three pass through as the canonical // spec operators every driver implements directly, so an anchored match // stays anchored rather than depending on regex dialect (#4128). - case 'notContains': return { $notContains: values[0] }; - case 'startsWith': return { $startsWith: values[0] }; - case 'endsWith': return { $endsWith: values[0] }; + case 'notContains': return { $notContains: String(v0) }; + case 'startsWith': return { $startsWith: String(v0) }; + case 'endsWith': return { $endsWith: String(v0) }; case 'in': return { $in: all }; case 'notIn': return { $nin: all }; default: