diff --git a/.changeset/kv-items-literal-escaping.md b/.changeset/kv-items-literal-escaping.md new file mode 100644 index 0000000000..286154f96c --- /dev/null +++ b/.changeset/kv-items-literal-escaping.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/common-utils': patch +--- + +fix: Keep string literals as written when rewriting `Map['key'] = 'value'` filters to the key-value items index, so values containing quotes or backslashes match instead of returning no rows diff --git a/packages/common-utils/src/__tests__/kvItemsRewrite.int.test.ts b/packages/common-utils/src/__tests__/kvItemsRewrite.int.test.ts new file mode 100644 index 0000000000..3748c8bd24 --- /dev/null +++ b/packages/common-utils/src/__tests__/kvItemsRewrite.int.test.ts @@ -0,0 +1,147 @@ +import { ClickHouseClient, createClient } from '@clickhouse/client'; +import SqlString from 'sqlstring'; + +import { parameterizedQueryToSql } from '@/clickhouse'; +import { ClickhouseClient as HdxClickhouseClient } from '@/clickhouse/node'; +import { Metadata, MetadataCache } from '@/core/metadata'; +import { renderChartConfig } from '@/core/renderChartConfig'; +import { ChartConfigWithOptDateRange } from '@/types'; + +// Runs the Map['k'] = 'v' -> has(items, concat(...)) rewrite against a real +// ClickHouse table, so a literal that is escaped differently from the source +// SQL shows up as a missing row rather than a string mismatch in a unit test. +describe('KV items rewrite integration', () => { + let client: ClickHouseClient; + let hdxClient: HdxClickhouseClient; + let metadata: Metadata; + + const DATABASE = 'default'; + const TABLE_NAME = 'kv_items_rewrite_int_test'; + + const ROWS: Array<{ Id: number; LogAttributes: Record }> = [ + { Id: 1, LogAttributes: { msg: 'plain' } }, + { Id: 2, LogAttributes: { msg: 'say "hi"' } }, + { Id: 3, LogAttributes: { msg: "it's" } }, + { Id: 4, LogAttributes: { msg: 'C:\\temp\\x' } }, + { Id: 5, LogAttributes: { msg: 'line1\nline2' } }, + { + Id: 6, + LogAttributes: { + msg: '["useMetadata.useTableMetadata",{"connectionId":"6a0c3937cc01bdc90179f030"}] data is undefined', + }, + }, + { Id: 7, LogAttributes: { "k'y": 'v' } }, + ]; + + const msgOf = (id: number) => ROWS.find(r => r.Id === id)!.LogAttributes.msg; + + const queryIds = async (where: string) => { + const config: ChartConfigWithOptDateRange = { + connection: 'test-connection', + from: { databaseName: DATABASE, tableName: TABLE_NAME }, + select: 'Id', + where, + whereLanguage: 'sql', + timestampValueExpression: 'Timestamp', + dateRange: [new Date('2025-01-01'), new Date('2025-01-02')], + orderBy: 'Id', + }; + const sql = parameterizedQueryToSql( + await renderChartConfig(config, metadata, undefined), + ); + const result = await client.query({ query: sql, format: 'JSONEachRow' }); + const rows = await result.json<{ Id: number }>(); + return { sql, ids: rows.map(r => Number(r.Id)) }; + }; + + beforeAll(async () => { + const host = process.env.CLICKHOUSE_HOST || 'http://localhost:8123'; + const username = process.env.CLICKHOUSE_USER || 'default'; + const password = process.env.CLICKHOUSE_PASSWORD || ''; + + client = createClient({ url: host, username, password }); + hdxClient = new HdxClickhouseClient({ host, username, password }); + + await client.command({ + query: `CREATE OR REPLACE TABLE ${DATABASE}.${TABLE_NAME} ( + Timestamp DateTime64(9) CODEC(Delta(8), ZSTD(1)), + Id UInt32, + LogAttributes Map(LowCardinality(String), String) CODEC(ZSTD(1)), + LogAttributeItems Array(String) MATERIALIZED + arrayMap((x) -> concat(x.1, '=', x.2), LogAttributes::Array(Tuple(String, String))) + CODEC(ZSTD(1)), + INDEX idx_log_attr_items LogAttributeItems TYPE text(tokenizer = 'array') GRANULARITY 1 + ) + ENGINE = MergeTree + ORDER BY (Timestamp)`, + }); + + await client.insert({ + table: `${DATABASE}.${TABLE_NAME}`, + values: ROWS.map(r => ({ ...r, Timestamp: '2025-01-01 12:00:00' })), + format: 'JSONEachRow', + }); + }); + + beforeEach(() => { + metadata = new Metadata(hdxClient, new MetadataCache()); + }); + + afterAll(async () => { + await client.command({ + query: `DROP TABLE IF EXISTS ${DATABASE}.${TABLE_NAME}`, + }); + await hdxClient.close(); + await client.close(); + }); + + it.each([ + ['plain value', 1], + ['double quotes', 2], + ['single quote', 3], + ['backslashes', 4], + ['newline', 5], + ['JSON with escaped quotes', 6], + ])('finds the row for Map[key] = value with %s', async (_, id) => { + const literal = SqlString.escape(msgOf(id)); + const rewritten = await queryIds(`LogAttributes['msg'] = ${literal}`); + // arrayElement() is not rewritten, so it shows what the filter should match. + const baseline = await queryIds( + `arrayElement(LogAttributes, 'msg') = ${literal}`, + ); + + expect(rewritten.sql).toContain('has(`LogAttributeItems`'); + expect(baseline.sql).not.toContain('LogAttributeItems'); + expect(rewritten.ids).toEqual([id]); + expect(baseline.ids).toEqual([id]); + }); + + it('finds the row for a doubled single quote in the value', async () => { + const { sql, ids } = await queryIds(`LogAttributes['msg'] = 'it''s'`); + expect(sql).toContain('has(`LogAttributeItems`'); + expect(ids).toEqual([3]); + }); + + it('finds the rows for Map[key] IN (...) with escaped values', async () => { + const values = [2, 4, 6].map(id => SqlString.escape(msgOf(id))); + const { sql, ids } = await queryIds( + `LogAttributes['msg'] IN (${values.join(', ')})`, + ); + expect(sql).toContain('hasAny(`LogAttributeItems`'); + expect(ids).toEqual([2, 4, 6]); + }); + + it('finds the row when the map key contains an escaped quote', async () => { + const { sql, ids } = await queryIds(`LogAttributes['k\\'y'] = 'v'`); + expect(sql).toContain('has(`LogAttributeItems`'); + expect(ids).toEqual([7]); + }); + + it('finds the row for a side panel rowWhere with JSON in an attribute', async () => { + const { sql, ids } = await queryIds( + String.raw`Timestamp = '2025-01-01 12:00:00' AND LogAttributes['msg']='[\"useMetadata.useTableMetadata\",{\"connectionId\":\"6a0c3937cc01bdc90179f030\"}] data is undefined'`, + ); + expect(sql).toContain('has(`LogAttributeItems`'); + expect(ids).toEqual([6]); + }); +}); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 41aea39ed9..0a6761b2c0 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -1878,6 +1878,21 @@ describe('renderChartConfig', () => { expect(sql).not.toContain("LogAttributes['service.name'] = 'api'"); }); + it('keeps escaped quotes in a SQL `where` value as written', async () => { + stubKvItemsMetadata(); + const literal = String.raw`'[\"x\"] it\'s'`; + const sql = parameterizedQueryToSql( + await renderChartConfig( + buildWhereConfig(`LogAttributes['error.message'] = ${literal}`), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain( + `has(\`LogAttributeItems\`, concat('error.message', '=', ${literal}))`, + ); + }); + it('rewrites `Map[key] IN (many)` in a SQL `where` to hasAny() on ClickHouse >= 26.5', async () => { stubKvItemsMetadata(); const sql = parameterizedQueryToSql( diff --git a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts index ae7624f26a..61215e8f05 100644 --- a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts +++ b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts @@ -426,6 +426,88 @@ describe('rewriteSqlFilterWithKvItems', () => { }); }); + describe('string literal escaping', () => { + // node-sql-parser keeps escape sequences undecoded in `.value`, so the + // rewrite must emit the original literal rather than re-escape it. + it.each([ + ['backslash-escaped double quotes', String.raw`'[\"x\"]'`], + ['backslash-escaped single quote', String.raw`'it\'s'`], + ['doubled single quote', `'it''s'`], + ['escaped backslash', String.raw`'a\\b'`], + ])('keeps %s in the value as written', (_, literal) => { + expect( + rewriteSqlFilterWithKvItems( + `LogAttributes['k'] = ${literal}`, + defaultLookup, + ), + ).toBe(`has(\`LogAttributeItems\`, concat('k', '=', ${literal}))`); + }); + + it('keeps escaped characters in the map key as written', () => { + expect( + rewriteSqlFilterWithKvItems( + String.raw`LogAttributes['k\'y'] = 'v'`, + defaultLookup, + ), + ).toBe( + String.raw`has(` + + '`LogAttributeItems`' + + String.raw`, concat('k\'y', '=', 'v'))`, + ); + }); + + it('keeps escaped values in a hasAny IN rewrite', () => { + expect( + rewriteSqlFilterWithKvItems( + String.raw`LogAttributes['k'] IN ('a\"b', 'c''d')`, + defaultLookup, + ), + ).toBe( + 'hasAny(`LogAttributeItems`, array(' + + String.raw`concat('k', '=', 'a\"b'), concat('k', '=', 'c''d')))`, + ); + }); + + it('keeps escaped values in the legacy has OR has IN rewrite', () => { + expect( + rewriteSqlFilterWithKvItems( + String.raw`LogAttributes['k'] IN ('a\"b', 'c\\d')`, + legacyLookup, + ), + ).toBe( + '(has(`LogAttributeItems`, ' + + String.raw`concat('k', '=', 'a\"b')) OR ` + + 'has(`LogAttributeItems`, ' + + String.raw`concat('k', '=', 'c\\d')))`, + ); + }); + + it('rewrites a side panel rowWhere with JSON in an attribute value', () => { + const lookup = makeLookup([ + [ + 'SpanAttributes', + { columnName: 'SpanAttributeItems', separator: '=' }, + ], + ]); + const message = String.raw`'[\"useMetadata.useTableMetadata\",{\"connectionId\":\"6a0c3937cc01bdc90179f030\"}] data is undefined'`; + const result = rewriteSqlFilterWithKvItems( + "Timestamp=parseDateTime64BestEffort('2026-10-28T13:50:49.762000000Z', 9) " + + "AND ServiceName='chc-hdx-app-prod' " + + "AND SpanAttributes['error.object']='Error' " + + `AND SpanAttributes['error.message']=${message} ` + + "AND SpanName='console.error'", + lookup, + ); + expect(result).toContain( + `has(\`SpanAttributeItems\`, concat('error.message', '=', ${message}))`, + ); + expect(result).toContain( + "has(`SpanAttributeItems`, concat('error.object', '=', 'Error'))", + ); + expect(result).not.toContain('\\\\'); + }); + }); + describe('indexHint', () => { // The LLM dashboard gates pair a value term with indexHint(a OR b), which // is the first caller to put a bare boolean OR in an argument position. diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 8de7551337..f0c5af8fbd 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -376,18 +376,15 @@ const fastifySQL = ({ } }; -function generateHasSqlForKvItemsColumn( - column: string, - key: string, +// keySql/valueSql are SQL literals taken verbatim from the parsed condition. +// The parser's `.value` keeps escape sequences undecoded, so re-escaping it +// with SqlString would double-escape quotes and backslashes. +function kvItemsConcatSql( + keySql: string, separator: string, - value: string, + valueSql: string, ): string { - return SqlString.format('has(??, concat(?, ?, ?))', [ - column, - key, - separator, - value, - ]); + return `concat(${keySql}, ${SqlString.format('?', [separator])}, ${valueSql})`; } export const rewriteSqlFilterWithKvItems = ( @@ -432,11 +429,11 @@ export const rewriteSqlFilterWithKvItems = ( ) { return; } - const mapKey: string = idxNode.value; + const keySql = parser.exprToSQL(idxNode); const info = textIndexInfoLookup.get(mapColumn)?.kv; if (!info) return; - let values: string[]; + const valueNodes: { value: string }[] = []; if (op === '=') { const right = node.right; if ( @@ -445,11 +442,10 @@ export const rewriteSqlFilterWithKvItems = ( ) { return; } - values = [right.value]; + valueNodes.push(right); } else { const right = node.right; if (right?.type !== 'expr_list' || !Array.isArray(right.value)) return; - const collected: string[] = []; for (const item of right.value) { if ( item?.type !== 'single_quote_string' || @@ -457,45 +453,35 @@ export const rewriteSqlFilterWithKvItems = ( ) { return; } - collected.push(item.value); + valueNodes.push(item); } - values = collected; } // Bail on empty values: `Map['k']='' ` also matches absent keys because // Map(String, String)'s subscript default is '', which `has(items, 'k=')` // alone does not preserve. Same rationale for empty entries in IN lists. - if (values.length === 0 || values.some(v => v === '')) return; + if (valueNodes.length === 0 || valueNodes.some(v => v.value === '')) { + return; + } + const valueSqls = valueNodes.map(v => parser.exprToSQL(v)); + + const columnSql = SqlString.format('??', [info.columnName]); + const concatSql = (valueSql: string) => + kvItemsConcatSql(keySql, info.separator, valueSql); let replacement: string; - if (values.length === 1) { - replacement = generateHasSqlForKvItemsColumn( - info.columnName, - mapKey, - info.separator, - values[0], - ); + if (valueSqls.length === 1) { + replacement = `has(${columnSql}, ${concatSql(valueSqls[0])})`; } else if (info.useHasAny) { // ClickHouse >= 26.5 supports `hasAny` over the direct_read map items // column in a single call. - replacement = `hasAny(${SqlString.format('??', [ - info.columnName, - ])}, array(${values - .map(v => - SqlString.format('concat(?, ?, ?)', [mapKey, info.separator, v]), - ) + replacement = `hasAny(${columnSql}, array(${valueSqls + .map(concatSql) .join(', ')}))`; } else { // Backport branches (26.2/26.3/26.4) support `has` but not `hasAny` over // the items column, so we fall back to a chain of `has(...) OR ...`. - replacement = `(${values - .map(v => - generateHasSqlForKvItemsColumn( - info.columnName, - mapKey, - info.separator, - v, - ), - ) + replacement = `(${valueSqls + .map(v => `has(${columnSql}, ${concatSql(v)})`) .join(' OR ')})`; }