From b1c2bb7005f677d937e48ec043384e3d66ddb195 Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Wed, 5 Aug 2026 17:54:15 +0000 Subject: [PATCH] fix(service-analytics): render $startsWith / $endsWith in the /analytics/sql echo (#5333) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectQLStrategy.buildFilterClauseSql` is the third compiler of the analytics filter tree — the one whose output is echoed to the browser. It handled `set`/`notSet`/`in`/`notIn`/`contains`/`notContains` explicitly and sent everything else to `SCALAR_SQL_OPS`, whose six entries do not include `startsWith`/`endsWith`; the unmapped exit was `return null`, which every compiler of this tree reads as "this node carries no constraint". So `{stage: {$startsWith: 'w'}}` echoed a statement with no WHERE at all while the query it documents ran `stage LIKE 'w%'`. The echo was strictly WIDER than execution, and its only reason to exist is reproducing execution: an author debugging "why does this chart show fewer rows" ran it, got more rows, and concluded the filter never applied. Same class as #3601 / #3602 / #3650, reached through the operator table. Two changes: 1. `LIKE_SQL_OPS` collects the four LIKE-family operators with their SQL spelling and pattern, one row each, mirroring `NativeSQLStrategy`'s `opMap`/`likePattern` pair so the two tables sit side by side and drift is visible. `contains`/`notContains` output is byte-identical. 2. The unmapped exit THROWS instead of returning null. It can, because the upstream vocabulary is closed: `fieldLeaves` in `filter-normalizer.ts` is the only leaf producer and refuses an operator outside `MONGO_TO_CUBE_OP` with INVALID_FILTER/400 before a leaf exists. An arrival therefore means our own two tables drifted — same call `convertFilter`'s `default:` arm made in #4128 — and a silently wider query is the one answer that must not be given. Deliberately not the 400 envelope: this is not a caller mistake. Measured: this exit is unreachable through the public door today, so reverting it to `return null` turns exactly one assertion red and leaves the enumeration green. It is a drift tripwire, not the behaviour fix. `objectql-echo-operator-coverage.test.ts` pins the issue's table by ROW IDS — the echoed statement is executed against the same fixture and its rows compared to what the query returns — then enumerates all 15 of `filter.zod.ts`'s `FILTER_OPERATORS`, asserting each renders a predicate with placeholders and `params` aligned. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BWS4heBoAitLmzCLhcYdbK --- .../analytics-echo-startswith-endswith.md | 54 +++ .../objectql-echo-operator-coverage.test.ts | 398 ++++++++++++++++++ .../src/strategies/objectql-strategy.ts | 65 ++- 3 files changed, 511 insertions(+), 6 deletions(-) create mode 100644 .changeset/analytics-echo-startswith-endswith.md create mode 100644 packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts diff --git a/.changeset/analytics-echo-startswith-endswith.md b/.changeset/analytics-echo-startswith-endswith.md new file mode 100644 index 0000000000..a46f42b253 --- /dev/null +++ b/.changeset/analytics-echo-startswith-endswith.md @@ -0,0 +1,54 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): `/analytics/sql` 回显补上 `$startsWith` / `$endsWith` 谓词(#5333) + +`ObjectQLStrategy.generateSql` 是同一棵过滤树的**第三个**编译器 —— 输出给浏览器的 +展示 SQL。它的 `buildFilterClauseSql` 显式处理 `set`/`notSet`/`in`/`notIn`/ +`contains`/`notContains`,其余落到只有六个条目的 `SCALAR_SQL_OPS` 查表; +`startsWith` / `endsWith` 两处都不在,于是走到 `return null`,而**这棵树的每个编译器 +都把 `null` 读成「本节点没有约束」**。结果: + +| `where` | 实际执行(`NativeSQLStrategy`) | 修复前的回显 | 修复后的回显 | +|---|---|---|---| +| `{stage: {$startsWith: 'w'}}` | `WHERE stage LIKE $1` / `['w%']` | **没有 WHERE**,`params` 为空 | `WHERE stage LIKE $1` / `['w%']` | +| `{stage: {$endsWith: 'n'}}` | `WHERE stage LIKE $1` / `['%n']` | **没有 WHERE**,`params` 为空 | `WHERE stage LIKE $1` / `['%n']` | +| `{stage: {$contains: 'w'}}` | `WHERE stage LIKE $1` | `WHERE stage LIKE $1`(本来就对) | 不变 | + +回显比实际执行的查询**更宽**。这个字符串存在的唯一理由就是复现执行 —— 文件自己在渲染 +块顶上写着 “a rendering that contradicts execution is worse than no rendering” —— +所以一个带着「为什么这张图少了几行」来看回显的作者,拿到的是一条**没有该筛选条件**的 +语句:跑一遍返回更多行,于是结论是「筛选器没生效」,而实际执行是生效的。与 +#3601 / #3602 / #3650 同一类「回显与执行不一致」,只是这次是从**算子表**这一侧到达的。 + +不涉及越权或错行:该字符串从不执行(`execute()` 的 echo 会丢弃 `params`),损害限于 +可调试性。 + +**两处修改:** + +1. **LIKE 家族收进一张表。** 新增 `LIKE_SQL_OPS`,四个算子(`contains` / + `notContains` / `startsWith` / `endsWith`)的 SQL 拼写与 pattern 并排放在一起, + 与 `NativeSQLStrategy.buildFilterClause` 的 `opMap` / `likePattern` 逐条对应 —— + 回显描述的正是那个编译器产出的语句,两张表并列摆着,漂移才看得见。 + `contains` / `notContains` 的产物一字未变。 + +2. **「渲染不了就静默丢」的出口改为 THROW。** `return null` 在这里与「无约束」同形, + 所以下一个新增算子会以同样的方式再丢一次。之所以**可以**抛错:上游算子词汇表是 + **封闭**的 —— `filter-normalizer.ts` 的 `fieldLeaves` 是叶节点的唯一生产者,它对 + `MONGO_TO_CUBE_OP` 之外的算子在建叶之前就以 `INVALID_FILTER` / 400 拒绝。因此任何 + 调用方写出的过滤器都到不了这个出口;真到了,只能意味着 normalizer 的表新增了这里 + 没有分支的算子,那是我们自己两张表漂移,而对此**唯一不能给的答案就是悄悄放宽作者的 + 查询**。与 `convertFilter` 的 `default:` 分支在 #4128 做出的是同一个选择;刻意**不**用 + `invalidFilterError` 的 400 信封 —— 这不是调用方形状的错误。 + +**该 throw 出口今天从公共入口不可达,这一点是测过的、也是刻意报告的**:把它改回 +`return null`(保留第 1 项修改)只会让它自己那一条断言变红,枚举断言和回显对照表 +全部保持绿色。它是一个漂移探针,不是行为修复 —— 行为修复是第 1 项。 + +新增 `objectql-echo-operator-coverage.test.ts`:issue 那张对照表按**行结果**钉住 +(回显语句在同一份 fixture 上真的被执行,行 id 与查询实际返回的行 id 比对 —— 丢掉的 +谓词藏不住,它返回的正是筛选器排除掉的行),再按 `filter.zod.ts` 的 +`FILTER_OPERATORS` 枚举全部 15 个可编写算子,逐个断言回显渲染出谓词、且 +placeholder 与 `params` 对齐。只断言 SQL 字符串会放过下一个未映射的算子 —— #4128 里 +`$between` 就藏在 `$startsWith` 后面。 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 new file mode 100644 index 0000000000..8834b5b989 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts @@ -0,0 +1,398 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5333] The echoed display SQL covers the WHOLE operator vocabulary. + * + * `ObjectQLStrategy.generateSql` is the third compiler of the same filter tree + * — the one whose output goes to the browser as `/analytics/sql` — and its only + * reason to exist is REPRODUCING execution. `buildFilterClauseSql` handled + * `set` / `notSet` / `in` / `notIn` / `contains` / `notContains` explicitly and + * sent everything else to `SCALAR_SQL_OPS`, whose six entries do not include + * `startsWith` / `endsWith`; an unmapped operator hit `return null`, which every + * compiler of this tree reads as "this node carries no constraint". So + * `{stage: {$startsWith: 'w'}}` echoed a statement with NO `WHERE` while the + * query it describes ran `stage LIKE 'w%'` — the echo was strictly WIDER than + * execution, and an author debugging "why does this chart have fewer rows than I + * expect" ran it, got MORE rows back, and concluded the filter was not applied. + * + * That is the #3601 / #3602 / #3650 class ("a rendering that contradicts + * execution is worse than no rendering", this file's subject's own words) reached + * through the operator table rather than through the read scope or a time window. + * + * Two things are pinned here, because the fix has two halves: + * + * 1. **Row-result cover for the issue's measured table.** The echoed statement + * is EXECUTED against the same fixture and its row ids compared to what the + * query actually returns. A dropped predicate cannot hide from that: the echo + * returns rows the filter excludes, which is the exact complaint. Asserting + * only the SQL string would have let the next unmapped operator through, the + * way `$between` sat behind `$startsWith` in #4128. + * 2. **An enumeration over the CLOSED vocabulary.** `filter-normalizer.ts` + * refuses an operator it cannot map (`Unsupported filter operator …`), so the + * leaf operators that can ever reach this compiler are exactly what + * {@link fieldLeaves} emits for the spec's `FILTER_OPERATORS` — a finite, + * enumerable set. Driving all fifteen authorable spellings through the echo + * turns "the two tables drifted" from something a reader has to notice into a + * failing test, which is what #4128 asked for and did not get for this third + * compiler. + * + * The same closure is what lets the unmapped-operator exit THROW instead of + * returning `null` (last block): a caller cannot reach it — the normalizer + * refuses first, with a 400 — so an arrival means our own two tables disagree, + * and silently widening the author's query is the one answer that must not be + * given. Same call `convertFilter`'s `default:` arm made in #4128. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; +import { FILTER_OPERATORS } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; + +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; + +/** + * `stage` is the string column the LIKE family reads. `'won'` is the only row + * that both starts with `w` and ends with `n`, and `'lost'` contains an `o` + * without doing either — so a prefix/suffix anchor compiled as a substring + * shows up as a wrong id rather than passing by luck. Rows 3/4 carry a NULL + * `stage`, the rows a dropped predicate hands back. + */ +const FIXTURE = [ + { id: '1', stage: 'won', amount: 10 }, + { id: '2', stage: 'lost', amount: 20 }, + { id: '3', stage: null, amount: 30 }, + { id: '4', stage: null, amount: 40 }, +]; + +const ALL_IDS = ['1', '2', '3', '4']; + +const CUBE: Cube = { + name: 'deals', + title: 'Deals', + sql: 'deal', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: Object.fromEntries( + ['id', 'stage', 'amount'].map((n) => [ + n, + { name: n, label: n, type: n === 'amount' ? 'number' : 'string', sql: n }, + ]), + ), + public: false, +} as unknown as Cube; + +/** 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; + } +} + +/** + * The stand-in engine's one accommodation, and it is NOT about #5333. + * + * `ObjectQLStrategy.convertFilter` sends the `contains` leaf to the engine as + * `{$regex: }` while its three siblings (`notContains`, + * `startsWith`, `endsWith`) pass as the canonical spec operators #4128 gave + * them. `$regex` is not in `filter.zod.ts`'s `FILTER_OPERATORS`, so + * `compileScopedFilterToSql` — a `FilterCondition` consumer, not a driver — + * fails closed on it. Filed as #5557: it is a defect in what this path EXECUTES, + * one function away from the echo this file is about, so it is translated here + * rather than fixed or hidden. + */ +function asCondition(filter: Record): FilterCondition { + const out: Record = {}; + for (const [field, cond] of Object.entries(filter)) { + if (cond && typeof cond === 'object' && !Array.isArray(cond) && '$regex' in cond) { + const { $regex, ...rest } = cond as Record; + out[field] = { ...rest, $contains: $regex }; + } else { + out[field] = cond; + } + } + return out as FilterCondition; +} + +/** One authorable `$op` spelling, with a comparand this fixture can answer. */ +const OPERATOR_CASES: Record = { + $eq: { stage: { $eq: 'won' } }, + $ne: { stage: { $ne: 'won' } }, + $gt: { amount: { $gt: 10 } }, + $gte: { amount: { $gte: 20 } }, + $lt: { amount: { $lt: 40 } }, + $lte: { amount: { $lte: 20 } }, + $in: { stage: { $in: ['won', 'lost'] } }, + $nin: { stage: { $nin: ['won'] } }, + $between: { amount: { $between: [10, 20] } }, + $contains: { stage: { $contains: 'o' } }, + $notContains: { stage: { $notContains: 'o' } }, + $startsWith: { stage: { $startsWith: 'w' } }, + $endsWith: { stage: { $endsWith: 'n' } }, + $null: { stage: { $null: true } }, + $exists: { stage: { $exists: false } }, +}; + +describe('[#5333] `/analytics/sql` echo — every authorable operator renders a predicate', () => { + let db: any; + let nativeCtx: StrategyContext; + let objectqlCtx: StrategyContext; + + 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(); + db.run(`CREATE TABLE "deal" ("id" TEXT PRIMARY KEY, "stage" TEXT, "amount" REAL);`); + const insert = db.prepare(`INSERT INTO "deal" ("id","stage","amount") VALUES (?,?,?)`); + for (const r of FIXTURE) insert.run([r.id, r.stage, r.amount]); + insert.free(); + + nativeCtx = { + getCube: (name: string) => (name === 'deals' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object: string, sql: string, params: unknown[]) => { + 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; + + // The ObjectQL path hands a `FilterCondition` to the engine, not SQL, so + // `compileScopedFilterToSql` — this package's other FilterCondition consumer + // — stands in for it, the way `filter-normalizer-not-null-safe.test.ts` does. + // That makes "what the query returns" a real row set rather than a + // re-reading of the same compiler the echo uses. + objectqlCtx = { + getCube: (name: string) => (name === 'deals' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async ( + _object: string, + options: { groupBy?: string[]; filter?: Record }, + ) => { + const { sql, params } = compileScopedFilterToSql( + asCondition(options.filter ?? {}), + 'deal', + ); + const stmt = db.prepare( + `SELECT "id" FROM "deal" AS "deal" WHERE ${sql.length > 0 ? sql : '1 = 1'}`, + ); + stmt.bind(params as any[]); + const out: Record[] = []; + while (stmt.step()) out.push({ ...stmt.getAsObject(), total: 1 }); + stmt.free(); + return out; + }, + } as StrategyContext; + }); + + afterAll(() => { + db?.close(); + }); + + const query = (where: unknown): AnalyticsQuery => ({ + cube: 'deals', + measures: ['total'], + dimensions: ['id'], + timezone: 'UTC', + where, + } as AnalyticsQuery); + + /** The echoed display SQL for a `where`, as `/analytics/sql` returns it. */ + const echo = (where: unknown): Promise<{ sql: string; params: unknown[] }> => + new ObjectQLStrategy().generateSql(query(where), objectqlCtx); + + /** The statement `NativeSQLStrategy` actually EXECUTES for the same `where`. */ + const nativeSql = (where: unknown): Promise<{ sql: string; params: unknown[] }> => + new NativeSQLStrategy().generateSql(query(where), nativeCtx); + + /** Run a `$n`-dialect statement and return its `id` column, sorted. */ + const run = (sql: string, params: unknown[]): string[] => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const out: string[] = []; + while (stmt.step()) out.push(String(stmt.getAsObject().id)); + stmt.free(); + return out.sort((x, y) => x.localeCompare(y)); + }; + + /** The rows the echoed statement returns — the author's "let me run this". */ + const echoIds = async (where: unknown): Promise => { + const { sql, params } = await echo(where); + return run(sql, params); + }; + + /** The rows the ObjectQL path actually returns for the same `where`. */ + const executedIds = async (where: unknown): Promise => { + const result = await new ObjectQLStrategy().execute(query(where), objectqlCtx); + return result.rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + }; + + // ── The issue's measured table, one case per row ──────────────────────────── + + describe("the issue's measured table", () => { + it('`$startsWith` echoes `LIKE` with the prefix pattern — was no WHERE at all', async () => { + const { sql, params } = await echo({ stage: { $startsWith: 'w' } }); + expect(sql).toContain('WHERE'); + expect(sql).toContain('stage LIKE $1'); + expect(params).toEqual(['w%']); + }); + + it('`$endsWith` echoes `LIKE` with the suffix pattern — was no WHERE at all', async () => { + const { sql, params } = await echo({ stage: { $endsWith: 'n' } }); + expect(sql).toContain('WHERE'); + expect(sql).toContain('stage LIKE $1'); + expect(params).toEqual(['%n']); + }); + + it('`$contains` still echoes the substring pattern — the row that already worked', async () => { + const { sql, params } = await echo({ stage: { $contains: 'o' } }); + expect(sql).toContain('stage LIKE $1'); + expect(params).toEqual(['%o%']); + }); + + it('the LIKE patterns are the ones the EXECUTED statement binds', async () => { + // `NativeSQLStrategy.buildFilterClause`'s `likePattern` table is the + // reference: the echo exists to describe that query, so the same `where` + // must bind the same pattern on both compilers. + for (const where of [ + { stage: { $startsWith: 'w' } }, + { stage: { $endsWith: 'n' } }, + { stage: { $contains: 'o' } }, + { stage: { $notContains: 'o' } }, + ]) { + const echoed = await echo(where); + const executed = await nativeSql(where); + expect(echoed.params, JSON.stringify(where)).toEqual(executed.params); + } + }); + + it('running the echo returns the rows the query returns, not more', async () => { + // The complaint itself: the author runs the echoed statement to reproduce + // a result and gets a WIDER row set, so the filter reads as not applied. + for (const [where, expected] of [ + [{ stage: { $startsWith: 'w' } }, ['1']], + [{ stage: { $endsWith: 'n' } }, ['1']], + [{ stage: { $contains: 'o' } }, ['1', '2']], + ] as Array<[FilterCondition, string[]]>) { + expect(await echoIds(where), `echo ${JSON.stringify(where)}`).toEqual(expected); + expect(await executedIds(where), `executed ${JSON.stringify(where)}`).toEqual(expected); + // Belt and braces: the whole fixture back is the one wrong answer that + // looks like a working statement. + expect( + (await echoIds(where)).length, + `${JSON.stringify(where)} echoed every row — the predicate was dropped`, + ).toBeLessThan(ALL_IDS.length); + } + }); + }); + + // ── The vocabulary, enumerated so the two tables cannot drift apart ───────── + + describe('the closed vocabulary, enumerated', () => { + it('covers every operator the spec declares — no case is missing from the table', () => { + expect(Object.keys(OPERATOR_CASES).sort()).toEqual([...FILTER_OPERATORS].sort()); + }); + + for (const op of FILTER_OPERATORS) { + it(`${op} renders a predicate in the echo`, async () => { + const where = OPERATOR_CASES[op]; + const { sql, params } = await echo(where); + // A missing predicate is not a missing feature: the statement stays + // valid and simply describes a wider query than the one it documents. + expect(sql, `${op} echoed no WHERE — the predicate was dropped`).toContain('WHERE'); + expect(sql).toMatch(/WHERE\s+\S/); + // The placeholder/`params` alignment `renderFilterNodeSql` promises: a + // clause dropped after its comparand was pushed leaves a binding with + // no placeholder to consume it, which silently shifts every later one. + const placeholders = new Set(sql.match(/\$\d+/g) ?? []); + expect(placeholders.size, `${op} placeholder/params mismatch`).toBe(params.length); + }); + } + + it('renders a predicate wherever the EXECUTED statement has one', async () => { + for (const op of FILTER_OPERATORS) { + const where = OPERATOR_CASES[op]; + const executed = await nativeSql(where); + if (!executed.sql.includes('WHERE')) continue; + const echoed = await echo(where); + expect(echoed.sql, `${op}: executed has a WHERE, echo does not`).toContain('WHERE'); + } + }); + }); + + // ── The exit that used to drop a predicate in silence ────────────────────── + + describe('an operator this compiler cannot render THROWS rather than vanishing', () => { + /** + * Reached through the private compiler on purpose. The public door CANNOT + * produce this call — `filter-normalizer.ts` refuses an operator outside + * `MONGO_TO_CUBE_OP` with a 400 before a leaf is ever built, which is + * exactly why the exit may throw: an arrival means the normalizer gained an + * operator this renderer has no arm for, i.e. our own two tables drifted. + * The alternative, `return null`, is read as "no constraint" by + * `renderFilterNodeSql` and hands the author a wider statement than the + * query — the #5333 defect, reproduced by the next operator to be added. + */ + const renderLeaf = (operator: string): string | null => { + const strategy = new ObjectQLStrategy() as unknown as { + buildFilterClauseSql( + col: string, + operator: string, + values: string[] | undefined, + params: unknown[], + ): string | null; + }; + return strategy.buildFilterClauseSql('stage', operator, ['w'], []); + }; + + it('throws for an operator with no SQL spelling', () => { + expect(() => renderLeaf('sortOf')).toThrow(/cannot render/i); + expect(() => renderLeaf('sortOf')).toThrow(/sortOf/); + }); + + it('does not throw for anything the normalizer can actually emit', () => { + // The leaf operators `fieldLeaves` produces: `MONGO_TO_CUBE_OP`'s twelve, + // plus `set` / `notSet` (the null predicates, `$eq: null` and a bare + // `null`) — `$between` lowers to `gte` / `lte`, already in the twelve. + const EMITTABLE = [ + 'equals', 'notEquals', 'gt', 'gte', 'lt', 'lte', + 'in', 'notIn', 'contains', 'notContains', 'startsWith', 'endsWith', + 'set', 'notSet', + ]; + for (const operator of EMITTABLE) { + expect(() => renderLeaf(operator), operator).not.toThrow(); + expect(renderLeaf(operator), `${operator} rendered nothing`).not.toBeNull(); + } + }); + + it('a value-less scalar leaf is still "no predicate", not a throw', () => { + // Unchanged: the empty-`values` exit mirrors `NativeSQLStrategy` and + // `execute()`, which drop such a leaf too. #5134 made the one shape that + // used to arrive here — an empty `$in` — a boolean CONSTANT upstream, so + // this arm is about a value-less leaf, not about an unknown operator. + const strategy = new ObjectQLStrategy() as unknown as { + buildFilterClauseSql( + col: string, + operator: string, + values: string[] | undefined, + params: unknown[], + ): string | null; + }; + expect(strategy.buildFilterClauseSql('stage', 'equals', [], [])).toBeNull(); + expect(strategy.buildFilterClauseSql('stage', 'equals', undefined, [])).toBeNull(); + }); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 7a5a7bc9b0..4191bce543 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -26,6 +26,23 @@ const SCALAR_SQL_OPS: Record = { equals: '=', notEquals: '!=', gt: '>', gte: '>=', lt: '<', lte: '<=', }; +/** + * The LIKE family: SQL spelling + the pattern each wraps its comparand in. + * + * Deliberately the same pair of tables `NativeSQLStrategy.buildFilterClause` + * carries (`opMap` / `likePattern`), because this file renders a description of + * the statement THAT compiler produces. Keeping them as one table here is the + * point of #5333: `startsWith` / `endsWith` were in neither the branch above nor + * `SCALAR_SQL_OPS`, so they fell to the unmapped exit and the predicate vanished + * from the echo while the query it documents ran `LIKE 'w%'`. + */ +const LIKE_SQL_OPS: Record string }> = { + contains: { sql: 'LIKE', pattern: (v) => `%${v}%` }, + notContains: { sql: 'NOT LIKE', pattern: (v) => `%${v}%` }, + startsWith: { sql: 'LIKE', pattern: (v) => `${v}%` }, + endsWith: { sql: 'LIKE', pattern: (v) => `%${v}` }, +}; + /** One cross-object grouping dimension planned for FK-expand (#3654). */ interface CrossObjectPlanDim { /** The caller's dimension name (output key), e.g. `region`. */ @@ -607,8 +624,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * 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). Returns null for an operator/value combination - * that carries no predicate, matching `execute()`, which drops it too. + * boolean, not SQL's 1/0). + * + * `null` means "this leaf carries no predicate" — a value-less scalar leaf, + * which `execute()` and `NativeSQLStrategy` drop too. It does NOT mean "I could + * not render that operator": #5333 was exactly that conflation, and an + * unrenderable operator now THROWS (see the exit below). */ private buildFilterClauseSql( col: string, @@ -628,13 +649,45 @@ export class ObjectQLStrategy implements AnalyticsStrategy { return `${col} ${operator === 'in' ? 'IN' : 'NOT IN'} (${placeholders})`; } - if (operator === 'contains' || operator === 'notContains') { - params.push(`%${values[0]}%`); - return `${col} ${operator === 'contains' ? 'LIKE' : 'NOT LIKE'} $${params.length}`; + // 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. + const like = LIKE_SQL_OPS[operator]; + if (like) { + params.push(like.pattern(values[0])); + return `${col} ${like.sql} $${params.length}`; } const op = SCALAR_SQL_OPS[operator]; - if (!op) return null; + if (!op) { + // [#5333] THROW rather than `return null`. `renderFilterNodeSql` reads a + // `null` as "this node constrains nothing", so the old exit deleted the + // predicate from the echoed statement — and a rendering WIDER than + // execution is the failure this whole render block exists to prevent + // (#3601 / #3602 / #3650): the author runs it to reproduce a result, gets + // more rows, and concludes the filter never applied. + // + // It can throw because the vocabulary upstream is CLOSED: `fieldLeaves` + // in `filter-normalizer.ts` is the only producer of leaf nodes, and it + // refuses an operator outside `MONGO_TO_CUBE_OP` with `INVALID_FILTER` / + // 400 before a leaf exists. So no caller-authored filter can land here — + // an arrival means the normalizer's table gained an entry this renderer + // has no arm for, which is our bug, not the caller's, and the one answer + // that must never be given for it is a silently wider query. Same call + // `convertFilter`'s `default:` arm made when it stopped reading an + // unmapped operator as equality (#4128). Deliberately NOT + // `invalidFilterError`'s 400 envelope: this is drift between two of our + // own tables, not a caller-shaped mistake. + throw new Error( + `[analytics] ObjectQLStrategy cannot render display SQL for filter operator ` + + `"${operator}" (on "${col}"). The analytics operator vocabulary is closed — ` + + `filter-normalizer.ts refuses anything it cannot map — so this means a new ` + + `operator reached the normalizer without an arm here. Add one rather than ` + + `dropping the predicate: an echo without it describes a WIDER query than the ` + + `one that ran (#5333).`, + ); + } params.push(coerceFilterValueForObjectQL(values[0])); return `${col} ${op} $${params.length}`; }