diff --git a/.changeset/fix-json-add-to-filters-sql.md b/.changeset/fix-json-add-to-filters-sql.md new file mode 100644 index 0000000000..ccfc4c2b51 --- /dev/null +++ b/.changeset/fix-json-add-to-filters-sql.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +Fix "Add to Filters" on a value inside parsed JSON from a String column (for example `Body`) building invalid SQL. The `JSONExtractString(...)` expression the JSON viewer produces is now passed through unchanged instead of being mis-parsed as a dot-form Map sub-key and mangled into a query ClickHouse rejects. diff --git a/packages/app/src/components/DBRowJsonViewer.test.tsx b/packages/app/src/components/DBRowJsonViewer.test.tsx index dfa5ee2b17..75a97f4d97 100644 --- a/packages/app/src/components/DBRowJsonViewer.test.tsx +++ b/packages/app/src/components/DBRowJsonViewer.test.tsx @@ -164,6 +164,21 @@ describe('DBRowJsonViewer', () => { ); }); + // HDX-4427: "Add to Filters" on a value inside parsed JSON from a String + // column must hand searchFilters the JSONExtract* expression, which is what + // gets serialized into the WHERE clause. Body here is a String column holding + // a JSON string with a dotted key, mirroring the play-clickstack repro. + it('adds a JSONExtractString filter for a value inside a parsed-JSON string column', () => { + renderComponent({ Body: JSON.stringify({ 'app.user.currency': 'USD' }) }); + + expandAndClickButton('Body', 'app.user.currency', 'Add to Filters'); + + expect(mockOnPropertyAddClick).toHaveBeenCalledWith( + "JSONExtractString(Body, 'app.user.currency')", + 'USD', + ); + }); + it('toggles columns with correct path formatting', () => { renderComponent(logData); clickLineButton('field1', 'Column'); diff --git a/packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts b/packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts new file mode 100644 index 0000000000..f298bb04bf --- /dev/null +++ b/packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts @@ -0,0 +1,167 @@ +import { + type FilterState, + filtersToQuery, + isValidFilterCondition, +} from '@hyperdx/common-utils/dist/filters'; + +import { buildJSONExtractQuery } from '@/components/DBRowJsonViewer'; +import { escapeFilterStateKeys, parseQuery } from '@/searchFilters'; + +import { cleanClickHouseExpression } from './utils'; + +// End-to-end coverage for the "Add to Filters" -> WHERE-clause path, driven +// through the real producer (buildJSONExtractQuery) and the real serialization +// (escapeFilterStateKeys -> filtersToQuery). This is the regression net for +// HDX-4427: a filter key that is a raw ClickHouse expression must serialize to +// valid SQL and survive a parseQuery round-trip. Asserting validity (rather +// than an exact string) catches this bug class for any JSON/Map key shape, not +// just the one the user happened to hit. + +// Real top-level columns on a typical OTel logs table. +const knownColumns = new Set(['Body', 'LogAttributes', 'ServiceName']); + +// buildJSONExtractQuery returns string | null; in every case here a nested path +// exists, so null is a test-setup error rather than an expected branch. +const jsonKey = ( + keyPath: string[], + parsedJsonRootPath: string[], + jsonColumns: string[] = [], + jsonExtractFn: + | 'JSONExtractString' + | 'JSONExtractFloat' + | 'JSONExtractBool' = 'JSONExtractString', + mapColumns: string[] = [], +): string => { + const out = buildJSONExtractQuery( + keyPath, + parsedJsonRootPath, + jsonColumns, + jsonExtractFn, + mapColumns, + ); + if (out == null) { + throw new Error('buildJSONExtractQuery returned null for a nested path'); + } + return out; +}; + +const runAddToFilter = ( + key: string, + values: { included?: string[]; excluded?: string[] }, +) => { + const state: FilterState = { + [key]: { + included: new Set(values.included ?? []), + excluded: new Set(values.excluded ?? []), + }, + }; + // Mirror updateFilterQuery: escape keys, then serialize to the query. + return filtersToQuery(escapeFilterStateKeys(state, knownColumns)); +}; + +describe('parsed-JSON "Add to Filters" -> valid SQL pipeline (HDX-4427)', () => { + const cases: { + desc: string; + key: string; + values: { included?: string[]; excluded?: string[] }; + }[] = [ + { + desc: 'flat dotted key in a String column (the reported play-clickstack case)', + key: jsonKey(['Body', 'app.user.currency'], ['Body']), + values: { included: ['USD'] }, + }, + { + desc: 'nested path in a String column', + key: jsonKey(['Body', 'app', 'user', 'id'], ['Body']), + values: { included: ['u-1'] }, + }, + { + desc: 'simple top-level key in a String column', + key: jsonKey(['Body', 'level'], ['Body']), + values: { included: ['error'], excluded: ['debug'] }, + }, + { + desc: 'numeric metric via JSONExtractFloat with a dotted path', + key: jsonKey( + ['Body', 'metrics.latency'], + ['Body'], + [], + 'JSONExtractFloat', + ), + values: { included: ['200'] }, + }, + { + desc: 'boolean flag via JSONExtractBool with a dotted path', + key: jsonKey(['Body', 'flags.enabled'], ['Body'], [], 'JSONExtractBool'), + values: { included: ['true'] }, + }, + { + desc: 'Map sub-value holding JSON (bracketed base column)', + key: jsonKey( + ['LogAttributes', 'config', 'db.host'], + ['LogAttributes', 'config'], + [], + 'JSONExtractString', + ['LogAttributes'], + ), + values: { included: ['localhost'] }, + }, + { + desc: 'value containing a single quote (SQL escaping)', + key: jsonKey(['Body', 'user.name'], ['Body']), + values: { included: ["O'Brien"] }, + }, + ]; + + it.each(cases)( + 'emits only valid ClickHouse SQL for $desc', + ({ key, values }) => { + const query = runAddToFilter(key, values); + + expect(query.length).toBeGreaterThan(0); + for (const filter of query) { + expect(filter.type).toBe('sql'); + if (filter.type === 'sql') { + expect(isValidFilterCondition(filter.condition, 'sql')).toBe(true); + } + } + }, + ); + + it('emits the corrected, valid condition for the reported case', () => { + const key = jsonKey(['Body', 'app.user.currency'], ['Body']); + expect(runAddToFilter(key, { included: ['USD'] })).toEqual([ + { + type: 'sql', + condition: "JSONExtractString(Body, 'app.user.currency') IN ('USD')", + }, + ]); + }); + + it('round-trips included and excluded values through parseQuery', () => { + const key = jsonKey(['Body', 'app.user.currency'], ['Body']); + const query = runAddToFilter(key, { + included: ['USD', 'EUR'], + excluded: ['JPY'], + }); + + const back = parseQuery(query).filters; + // parseQuery keys are the canonical (escaped) form; clean them the same way + // unescapeFilterStateKeys does before comparing to the in-memory key. + const restoredKeys = Object.keys(back).map(cleanClickHouseExpression); + expect(restoredKeys).toEqual([key]); + + const entry = back[Object.keys(back)[0]]; + expect(entry.included).toEqual(new Set(['USD', 'EUR'])); + expect(entry.excluded).toEqual(new Set(['JPY'])); + }); + + it('round-trips a value containing a single quote', () => { + const key = jsonKey(['Body', 'user.name'], ['Body']); + const back = parseQuery( + runAddToFilter(key, { included: ["O'Brien"] }), + ).filters; + const entry = back[Object.keys(back)[0]]; + expect(entry.included).toEqual(new Set(["O'Brien"])); + }); +}); diff --git a/packages/app/src/components/DBSearchPageFilters/utils.test.ts b/packages/app/src/components/DBSearchPageFilters/utils.test.ts index 986a86f0bd..10fa53c521 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.test.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.test.ts @@ -180,6 +180,34 @@ describe('toClickHouseKeyExpression', () => { "LogAttributes['42.foo']", ); }); + + // HDX-4427: "Add to Filters" on a value inside parsed JSON from a String + // column builds a JSONExtract* function call as the filter key. These are + // already valid ClickHouse expressions and must pass through untouched. + // Previously the dot inside the quoted JSON path argument made + // parseMapFieldName treat the whole expression as a dot-form Map sub-key, and + // mergePath mangled it into invalid SQL like + // `JSONExtractString(Body, 'app['user.currency')']`. + describe('raw SQL function-call expression keys (parsed-JSON "Add to Filters")', () => { + it.each([ + "JSONExtractString(Body, 'app.user.currency')", + "JSONExtractString(Body, 'app', 'user.currency')", + "JSONExtractString(Body, 'level')", + "JSONExtractFloat(Body, 'metrics.latency')", + "JSONExtractBool(Body, 'flags.enabled')", + "JSONExtractString(LogAttributes['weird.key.payload'], 'abc.def.jqk/abcd')", + ])('leaves the JSON-extract expression %s unchanged', key => { + expect(toClickHouseKeyExpression(key)).toBe(key); + }); + + // The guard generalizes the previous `startsWith('toString(')` special case, + // so a toString() wrapper with no bracket access still passes through. + it('leaves a toString() wrapper without bracket access unchanged', () => { + expect(toClickHouseKeyExpression('toString(Body)')).toBe( + 'toString(Body)', + ); + }); + }); }); describe('toQuotedClickHouseKeyExpression', () => { @@ -241,6 +269,18 @@ describe('toQuotedClickHouseKeyExpression', () => { ).toBe('`service-name`'); }); + // HDX-4427: the JSONExtract* key from a parsed-JSON "Add to Filters" reaches + // toQuotedClickHouseKeyExpression via escapeFilterStateKeys. It is not a known + // column and is already valid SQL, so it must pass through unquoted/unmangled. + it('leaves a JSON-extract function-call key unchanged', () => { + expect( + toQuotedClickHouseKeyExpression( + "JSONExtractString(Body, 'app.user.currency')", + new Set(['Body']), + ), + ).toBe("JSONExtractString(Body, 'app.user.currency')"); + }); + describe('with knownColumns (schema-aware)', () => { it('quotes a flat column whose name contains dots as a single identifier', () => { const cols = new Set(['__hdx_materialized_k8s.cluster.name']); diff --git a/packages/app/src/components/DBSearchPageFilters/utils.ts b/packages/app/src/components/DBSearchPageFilters/utils.ts index b2297a6cc4..86f0465426 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.ts @@ -142,12 +142,19 @@ export function getFilterStateEntry( ); } +// A key that begins with `identifier(` is a raw SQL function call (e.g. +// `toString(...)`, `JSONExtractString(...)`), not a column name or a dot-form +// Map sub-key, so it is already a valid ClickHouse expression. +const isSqlFunctionCallExpression = (key: string): boolean => + /^[A-Za-z_]\w*\(/.test(key); + // Coerce a filterState key into a ClickHouse expression suitable for raw SQL. // A dot-form Map sub-key like `LogAttributes.host.name` is rewritten to bracket // form `LogAttributes['host.name']` via `mergePath` so the conversion stays // consistent with the keys produced by the facet-discovery path. Bracket form, -// backtick-quoted JSON paths, `toString(...)` wrappers, and plain column names -// are returned unchanged. Use this when handing a filterState key off to a SQL +// backtick-quoted JSON paths, raw SQL function-call expressions +// (`toString(...)`, `JSONExtractString(...)`), and plain column names are +// returned unchanged. Use this when handing a filterState key off to a SQL // caller (e.g. "Load more" via metadata.getKeyValues), since `setFilterValue` // normalizes Map sub-keys to dot form which ClickHouse cannot resolve as map // access. @@ -162,7 +169,12 @@ export function toClickHouseKeyExpression(key: string): string { key.includes("['") || key.includes('["') || key.includes('`') || - key.startsWith('toString(') + // "Add to Filters" on a value inside parsed JSON from a String column builds + // a function-call key (e.g. JSONExtractString(Body, 'app.user.currency')); + // it must pass through untouched. Without this, parseMapFieldName splits on + // the dot inside the quoted argument and mergePath mangles it into the + // invalid `JSONExtractString(Body, 'app['user.currency')']`. HDX-4427. + isSqlFunctionCallExpression(key) ) { return key; } diff --git a/packages/app/src/components/HyperJson.tsx b/packages/app/src/components/HyperJson.tsx index c047983097..8a6a51aaf7 100644 --- a/packages/app/src/components/HyperJson.tsx +++ b/packages/app/src/components/HyperJson.tsx @@ -272,6 +272,7 @@ const Line = React.memo( <>
0, diff --git a/packages/app/tests/e2e/components/SidePanelComponent.ts b/packages/app/tests/e2e/components/SidePanelComponent.ts index 50559ce0b2..7153dc58b4 100644 --- a/packages/app/tests/e2e/components/SidePanelComponent.ts +++ b/packages/app/tests/e2e/components/SidePanelComponent.ts @@ -62,6 +62,47 @@ export class SidePanelComponent { } } + /** + * In the parsed JSON view, expand a field whose value is a JSON string, then + * click the "Add to Filters" line action on a nested key. Each row in the JSON + * viewer carries `data-testid="json-viewer-line"`, and the action is a + * `