From 7d1b838fe65573e44bd17d5d287bc8164e024baa Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 2 Nov 2026 15:39:06 -0500 Subject: [PATCH] fix: keep JSON path segments with special characters quoted in row detail lookups --- .../fix-at-prefixed-json-key-row-details.md | 5 ++ .../e2e/components/SidePanelComponent.ts | 21 ++++++ .../json-at-prefixed-key-row-details.spec.ts | 73 +++++++++++++++++++ packages/app/tests/e2e/seed-clickhouse.ts | 12 ++- .../src/__tests__/metadata.test.ts | 60 +++++++++++++++ packages/common-utils/src/core/metadata.ts | 47 ++++++++++-- 6 files changed, 212 insertions(+), 6 deletions(-) create mode 100644 .changeset/fix-at-prefixed-json-key-row-details.md create mode 100644 packages/app/tests/e2e/features/search/json-at-prefixed-key-row-details.spec.ts diff --git a/.changeset/fix-at-prefixed-json-key-row-details.md b/.changeset/fix-at-prefixed-json-key-row-details.md new file mode 100644 index 0000000000..4f9ea6a094 --- /dev/null +++ b/.changeset/fix-at-prefixed-json-key-row-details.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/common-utils': patch +--- + +fix: keep JSON path segments with special characters quoted in row detail lookups diff --git a/packages/app/tests/e2e/components/SidePanelComponent.ts b/packages/app/tests/e2e/components/SidePanelComponent.ts index 753513db3e..d32912bb7c 100644 --- a/packages/app/tests/e2e/components/SidePanelComponent.ts +++ b/packages/app/tests/e2e/components/SidePanelComponent.ts @@ -329,6 +329,27 @@ export class SidePanelComponent { .click({ timeout: this.defaultTimeout }); } + /** + * The "Column Values" (parsed) tab's content panel, which renders the full + * fetched row via the JSON viewer. + */ + get columnValuesContent() { + return this.panelContainer.getByTestId('side-panel-tab-parsed'); + } + + /** Open the "Column Values" (parsed) tab. */ + async openColumnValues() { + await this.clickTab('parsed'); + } + + /** + * A line in the "Column Values" tab matching `text` (substring). Used to + * assert a fetched row's value is shown. + */ + columnValue(text: string) { + return this.columnValuesContent.getByText(text, { exact: false }); + } + /** * Close the side panel (if it has a close button) */ diff --git a/packages/app/tests/e2e/features/search/json-at-prefixed-key-row-details.spec.ts b/packages/app/tests/e2e/features/search/json-at-prefixed-key-row-details.spec.ts new file mode 100644 index 0000000000..10d42d4869 --- /dev/null +++ b/packages/app/tests/e2e/features/search/json-at-prefixed-key-row-details.spec.ts @@ -0,0 +1,73 @@ +/** + * Selecting an at-prefixed JSON key breaks row details. + * + * When a displayed SELECT column is a JSON subcolumn whose key is not a bare + * identifier (e.g. `ResourceAttributesJSON.`@request_id``), ClickHouse reports + * the result column name without backticks (`ResourceAttributesJSON.@request_id`). + * The row side panel rebuilds a WHERE clause from the displayed columns to fetch + * the full row; before the fix it used that bare name verbatim, producing + * invalid SQL that matched no rows, so the panel came up empty. + * + * This drives the real UI path against a JSON-column schema: select the + * at-prefixed key, open the row, and confirm the detail panel is populated with + * the selected event's values. + */ +import { SearchPage } from '../../page-objects/SearchPage'; +import { + INTERESTING_FILTER_KEYS_JSON_AT_KEY, + INTERESTING_FILTER_KEYS_ROWS, +} from '../../seed-clickhouse'; +import { expect, test } from '../../utils/base-test'; +import { INTERESTING_FILTER_KEYS_SOURCE_NAME } from '../../utils/constants'; + +const QUERY_TIMEOUT = 20_000; + +const [ROW1] = INTERESTING_FILTER_KEYS_ROWS; + +test.describe( + 'Search: at-prefixed JSON key row details', + { tag: ['@search', '@full-stack'] }, + () => { + test('opening a row with an at-prefixed JSON key in SELECT shows the event', async ({ + page, + }) => { + const searchPage = new SearchPage(page); + await searchPage.goto(); + await searchPage.selectSource(INTERESTING_FILTER_KEYS_SOURCE_NAME); + await searchPage.timePicker.selectRelativeTime('Last 1 hour'); + + await test.step('isolate a single row', async () => { + await searchPage.performSearch(`ServiceName:"${ROW1.serviceName}"`); + await expect(searchPage.getTableError()).toHaveCount(0); + await expect(searchPage.table.getRows()).toHaveCount(1, { + timeout: QUERY_TIMEOUT, + }); + }); + + await test.step('select the at-prefixed JSON key as a column', async () => { + await searchPage.setCustomSELECT( + `Timestamp, ServiceName, ResourceAttributesJSON.\`${INTERESTING_FILTER_KEYS_JSON_AT_KEY}\``, + ); + await searchPage.submitEmptySearch(); + await expect(searchPage.getTableError()).toHaveCount(0); + await expect(searchPage.table.getRows()).toHaveCount(1, { + timeout: QUERY_TIMEOUT, + }); + }); + + await test.step('row details load the selected event', async () => { + await searchPage.table.clickFirstRow(); + await expect(searchPage.sidePanel.container).toBeVisible(); + // Before the fix the detail WHERE dropped the key's backticks and + // matched no rows, so the panel rendered empty with no error. + await expect(searchPage.sidePanel.errorState).toHaveCount(0); + + await searchPage.sidePanel.openColumnValues(); + await expect( + searchPage.sidePanel.columnValue(ROW1.jsonAtKeyValue), + ).toBeVisible({ timeout: QUERY_TIMEOUT }); + await expect(searchPage.sidePanel.columnValue(ROW1.body)).toBeVisible(); + }); + }); + }, +); diff --git a/packages/app/tests/e2e/seed-clickhouse.ts b/packages/app/tests/e2e/seed-clickhouse.ts index c101df91c6..f96e4fe916 100644 --- a/packages/app/tests/e2e/seed-clickhouse.ts +++ b/packages/app/tests/e2e/seed-clickhouse.ts @@ -98,6 +98,10 @@ export const INTERESTING_FILTER_KEYS_ROWS = [ serviceName: 'service1', resourceAttrValue: 'value1', jsonValue: 'value1', + // Top-level JSON key that is NOT a bare identifier (`@` prefix). Selecting + // it (`ResourceAttributesJSON.`@request_id``) and opening the row must keep + // the key backtick-quoted in the detail WHERE clause (HDX-5867). + jsonAtKeyValue: 'request-A', clusterName: 'cluster1', serviceNameHyphen: 'svc-one', mapHyphenValue: 'mapval1', @@ -108,6 +112,7 @@ export const INTERESTING_FILTER_KEYS_ROWS = [ serviceName: 'service2', resourceAttrValue: 'value2', jsonValue: 'value2', + jsonAtKeyValue: 'request-B', clusterName: 'cluster2', serviceNameHyphen: 'svc-two', mapHyphenValue: 'mapval2', @@ -118,6 +123,7 @@ export const INTERESTING_FILTER_KEYS_ROWS = [ serviceName: 'service3', resourceAttrValue: 'value3', jsonValue: 'value3', + jsonAtKeyValue: 'request-C', clusterName: 'cluster3', serviceNameHyphen: 'svc-three', mapHyphenValue: 'mapval3', @@ -126,6 +132,10 @@ export const INTERESTING_FILTER_KEYS_ROWS = [ }, ] as const; +// The at-prefixed JSON key seeded into `ResourceAttributesJSON`. Exported so the +// row-details spec selects and asserts against the same key/values. +export const INTERESTING_FILTER_KEYS_JSON_AT_KEY = '@request_id'; + // A single log whose Body is a JSON string with a flat, dotted key. It lives in // the default E2E Logs table (so it shares the E2E Logs source); the unique // ServiceName isolates it from other default-logs tests, which all scope by @@ -793,7 +803,7 @@ function generateK8sEventLogs( function generateInterestingFilterKeyLogData(seedRef: number): string { return INTERESTING_FILTER_KEYS_ROWS.map((row, i) => { const timestampNs = (seedRef - (i + 1) * 1000) * 1000000; - const json = `{"key": {"subKey": {"subSubKey": "${row.jsonValue}"}}}`; + const json = `{"key": {"subKey": {"subSubKey": "${row.jsonValue}"}}, "${INTERESTING_FILTER_KEYS_JSON_AT_KEY}": "${row.jsonAtKeyValue}"}`; // JSON column with a hyphenated name AND hyphenated nested keys. const jsonHyphen = `{"key-1": {"key-2": "${row.jsonHyphenValue}"}}`; return ( diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index dab2349b0b..a4ea74e0a8 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -4072,6 +4072,66 @@ describe('quoteResultColumnNameIfNeeded', () => { '`x-host-header`', ); }); + + it('quotes non-identifier segments of a dotted JSON path', () => { + expect(quoteResultColumnNameIfNeeded('event.@request_id')).toBe( + 'event.`@request_id`', + ); + expect(quoteResultColumnNameIfNeeded('event.my key')).toBe( + 'event.`my key`', + ); + expect(quoteResultColumnNameIfNeeded('a.b.@c')).toBe('a.b.`@c`'); + }); + + it('quotes the path but preserves a JSON type cast', () => { + // ClickHouse reports `event.foo.:String` as event.foo.:`String`. + expect(quoteResultColumnNameIfNeeded('event.foo.:`String`')).toBe( + 'event.foo.:`String`', + ); + expect(quoteResultColumnNameIfNeeded('event.@request_id.:`String`')).toBe( + 'event.`@request_id`.:`String`', + ); + expect(quoteResultColumnNameIfNeeded('event.@id.:`Decimal(10, 2)`')).toBe( + 'event.`@id`.:`Decimal(10, 2)`', + ); + expect(quoteResultColumnNameIfNeeded('event.@id.:`Int64`.null')).toBe( + 'event.`@id`.:`Int64`.null', + ); + }); + + it('quotes a key that starts with `:`', () => { + expect(quoteResultColumnNameIfNeeded('event.:k')).toBe('event.`:k`'); + }); + + it('quotes a dotted-path segment that itself contains a colon', () => { + expect(quoteResultColumnNameIfNeeded('event.k8s:pod')).toBe( + 'event.`k8s:pod`', + ); + }); + + it('quotes an all-digit JSON key (unquoted it errors in ClickHouse)', () => { + // `event.200` unquoted is parsed as tuple-element access and rejected for a + // JSON column; backticks make it a key reference. Tuple positional access + // is reported by ClickHouse as `tupleElement(...)`, never a bare `t.1`. + expect(quoteResultColumnNameIfNeeded('event.200')).toBe('event.`200`'); + expect(quoteResultColumnNameIfNeeded('tup.1')).toBe('tup.`1`'); + }); + + it('preserves a sub-object accessor', () => { + // ClickHouse reports `event.^sub` as event.^`sub`. + expect(quoteResultColumnNameIfNeeded('event.^`sub`')).toBe('event.^`sub`'); + expect(quoteResultColumnNameIfNeeded('event.@a.^`b.c`')).toBe( + 'event.`@a`.^`b.c`', + ); + }); + + it('quotes a key that starts with `^`', () => { + expect(quoteResultColumnNameIfNeeded('event.^k')).toBe('event.`^k`'); + }); + + it('leaves a malformed path with an empty segment untouched', () => { + expect(quoteResultColumnNameIfNeeded('a..b')).toBe('a..b'); + }); }); describe('parametric aggregate arguments are inlined as literals', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 7471296951..09062eeaa2 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -190,19 +190,56 @@ export const quoteIdentifierIfNeeded = (identifier: string): string => { : quoteJsonPathSegment(unquoted); }; +// A single column or JSON path segment. No quoting/grouping characters, no `.` separator. +const PLAIN_PATH_SEGMENT = /^[^`'"()[\],.]+/; + +// A JSON sub-object accessor or type cast. ClickHouse reports the operand +// backticked (j.^`a`, j.a.:`Int64`). +const OPERATOR_PATH_SEGMENT = /^[\^:]`(?:[^`]|``)*`/; + +// Split a name into `.`-separated path segments, or undefined when it isn't a +// plain JSON path (function calls, literals, empty segments). +const splitResultColumnPath = (name: string): string[] | undefined => { + const segments: string[] = []; + let rest = name; + while (true) { + const segment = + rest.match(OPERATOR_PATH_SEGMENT)?.[0] ?? + rest.match(PLAIN_PATH_SEGMENT)?.[0]; + if (segment == null) return undefined; + segments.push(segment); + rest = rest.slice(segment.length); + if (rest === '') return segments; + if (!rest.startsWith('.')) return undefined; + rest = rest.slice(1); + } +}; + +const quotePathSegment = (segment: string): string => + OPERATOR_PATH_SEGMENT.test(segment) + ? segment + : quoteIdentifierIfNeeded(segment); + /** * Quote a query result column name so it can be referenced in raw SQL. * * Most result column names do not need quoting: * - Bare identifiers (alphanumeric and underscore, starting with a letter or underscore) * - Numeric literals (e.g., `1`, `0x10`) - * - Dotted column names (e.g., `my.col`) * - Function calls (e.g., `plus(a, b)`, `arrayElement(m, 'key')`) + * + * Dotted names are JSON/nested subcolumn paths (e.g. `event.@request_id`, + * which ClickHouse reports for the select `` event.`@request_id` ``). Each + * segment is quoted on its own. JSON type casts (eg. `` .:`String` ``) and + * `` ^`sub` `` sub-object accessors are preserved. */ -export const quoteResultColumnNameIfNeeded = (name: string): string => - /^[^`'"()[\],.]+$/.test(name) && !Number.isFinite(Number(name)) - ? quoteIdentifierIfNeeded(name) - : name; +export const quoteResultColumnNameIfNeeded = (name: string): string => { + if (Number.isFinite(Number(name))) { + return name; + } + const segments = splitResultColumnPath(name); + return segments ? segments.map(quotePathSegment).join('.') : name; +}; const columnAppearsInMvSelect = (sql: string, columnName: string): boolean => { const escaped = columnName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');