From 4f7d8ddd840291a62e9ddad9ec5ee51fb80ee775 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Fr=C3=B6mbgen?= <23717573+mfroembgen@users.noreply.github.com> Date: Wed, 1 Jul 2026 08:17:13 +0200 Subject: [PATCH] fix: stringify JSON search sidebar filters --- .changeset/tame-json-sidebar-filters.md | 6 + packages/app/src/DBSearchPage.tsx | 61 +++- .../DBSearchPage.directTrace.test.tsx | 109 ++++++- .../app/src/__tests__/searchFilters.test.ts | 259 ++++++++++++++++ .../src/components/DBSearchPageFilters.tsx | 50 +++- .../{ => __tests__}/hooks.test.tsx | 283 +++++++++++++++++- .../{ => __tests__}/utils.test.ts | 111 ++++++- .../components/DBSearchPageFilters/hooks.ts | 60 ++-- .../components/DBSearchPageFilters/utils.ts | 45 ++- .../__tests__/DBSearchPageFilters.test.tsx | 198 ++++++++++++ packages/app/src/searchFilters.tsx | 76 ++++- .../src/__tests__/metadata.test.ts | 186 +++++++++++- packages/common-utils/src/core/metadata.ts | 227 ++++++++++++-- 13 files changed, 1581 insertions(+), 90 deletions(-) create mode 100644 .changeset/tame-json-sidebar-filters.md rename packages/app/src/components/DBSearchPageFilters/{ => __tests__}/hooks.test.tsx (75%) rename packages/app/src/components/DBSearchPageFilters/{ => __tests__}/utils.test.ts (75%) diff --git a/.changeset/tame-json-sidebar-filters.md b/.changeset/tame-json-sidebar-filters.md new file mode 100644 index 0000000000..4f1c1fb3c3 --- /dev/null +++ b/.changeset/tame-json-sidebar-filters.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +Fix JSON-backed search sidebar filters and metadata value queries to serialize resource attributes as ClickHouse string expressions, prioritize selected and pinned fields during facet loading, refresh loaded facet values when the active filter context changes, and surface load-more actions for empty facets that can fetch additional values. diff --git a/packages/app/src/DBSearchPage.tsx b/packages/app/src/DBSearchPage.tsx index 9995a976dd..581a6ab47c 100644 --- a/packages/app/src/DBSearchPage.tsx +++ b/packages/app/src/DBSearchPage.tsx @@ -115,7 +115,10 @@ import { useSavedSearch, useUpdateSavedSearch, } from '@/savedSearch'; -import { useSearchPageFilterState } from '@/searchFilters'; +import { + canonicalizeFilterQuery, + useSearchPageFilterState, +} from '@/searchFilters'; import { getEventBody, useSource, useSources } from '@/source'; import { useAppTheme, useBrandDisplayName } from '@/theme/ThemeProvider'; import { @@ -144,6 +147,7 @@ import { } from './components/TimePicker/utils'; import { useColumns, + useJsonColumns, useResolvedDateTimeColumns, useTableMetadata, } from './hooks/useMetadata'; @@ -1069,6 +1073,7 @@ export function DBSearchPage() { }, resolver: zodResolver(SearchConfigSchema), }); + const canonicalizedFiltersSyncRef = useRef(null); const inputSource = useWatch({ name: 'source', control }); @@ -1103,6 +1108,10 @@ export function DBSearchPage() { // const { data: inputSourceObj } = useSource({ id: inputSource }); const { data: inputSourceObjs } = useSources(); const inputSourceObj = inputSourceObjs?.find(s => s.id === inputSource); + const inputSourceTableConnection = useMemo( + () => tcFromSource(inputSourceObj), + [inputSourceObj], + ); const [displayedTimeInputValue, setDisplayedTimeInputValue] = useState('Live Tail'); @@ -1122,6 +1131,15 @@ export function DBSearchPage() { const prevSearched = usePrevious(searchedConfig); useEffect(() => { if (JSON.stringify(prevSearched) !== JSON.stringify(searchedConfig)) { + if ( + canonicalizedFiltersSyncRef.current != null && + JSON.stringify(canonicalizedFiltersSyncRef.current) === + JSON.stringify(searchedConfig.filters ?? []) + ) { + canonicalizedFiltersSyncRef.current = null; + return; + } + reset({ select: searchedConfig?.select ?? '', where: searchedConfig?.where ?? '', @@ -1255,6 +1273,10 @@ export function DBSearchPage() { : new Set(), [inputSourceColumns], ); + const { data: inputSourceJsonColumns = [] } = useJsonColumns( + inputSourceTableConnection, + { enabled: !!inputSourceObj }, + ); const watchedSource = useWatch({ control, @@ -1285,11 +1307,43 @@ export function DBSearchPage() { useResolvedDateTimeColumns(inputSourceColumns); const filters = useWatch({ name: 'filters', control }); + const canonicalizedFilters = useMemo(() => { + if (!filters?.length || !inputSourceColumns) { + return null; + } + + const nextFilters = canonicalizeFilterQuery( + filters, + knownColumns, + inputSourceJsonColumns, + dateTimeColumns, + ); + + return JSON.stringify(nextFilters) === JSON.stringify(filters) + ? null + : nextFilters; + }, [ + dateTimeColumns, + filters, + inputSourceColumns, + inputSourceJsonColumns, + knownColumns, + ]); + useEffect(() => { + if (!canonicalizedFilters) { + return; + } + + setValue('filters', canonicalizedFilters); + canonicalizedFiltersSyncRef.current = canonicalizedFilters; + setSearchedConfig({ filters: canonicalizedFilters }); + }, [canonicalizedFilters, setSearchedConfig, setValue]); const searchFilters = useSearchPageFilterState({ searchQuery: filters ?? undefined, onFilterChange: handleSetFilters, dateTimeColumns, knownColumns, + jsonColumns: inputSourceJsonColumns, }); useEffect(() => { @@ -1878,11 +1932,6 @@ export function DBSearchPage() { ], ); - const inputSourceTableConnection = useMemo( - () => tcFromSource(inputSourceObj), - [inputSourceObj], - ); - const [isSourceSchemaPreviewOpen, setIsSourceSchemaPreviewOpen] = useState(false); diff --git a/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx b/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx index 5c46ed7f18..d72e442304 100644 --- a/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx +++ b/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx @@ -11,10 +11,15 @@ const mockSetAnalysisMode = jest.fn(); const mockSetIsLive = jest.fn(); const mockOnSearch = jest.fn(); const mockOnTimeRangeSelect = jest.fn(); +const mockCanonicalizeFilterQuery = jest.fn(); +const mockFormReset = jest.fn(); let mockDirectTraceId: string | null = null; let mockSearchedConfig: Record = {}; +let mockApplySearchedConfigUpdates = false; let mockSources: any[] = []; +let mockColumns: any[] | undefined; +let mockJsonColumns: string[] = []; let latestDirectTracePanelProps: Record | null = null; jest.mock('@/layout', () => ({ @@ -69,9 +74,42 @@ jest.mock('nuqs', () => ({ return [null, jest.fn()]; } }, - useQueryStates: () => [mockSearchedConfig, mockSetSearchedConfig], + useQueryStates: () => { + const [, forceRender] = React.useReducer(value => value + 1, 0); + const setSearchedConfig = React.useCallback( + (value: Record) => { + mockSetSearchedConfig(value); + if (mockApplySearchedConfigUpdates) { + mockSearchedConfig = { ...mockSearchedConfig, ...value }; + forceRender(); + } + }, + [], + ); + return [mockSearchedConfig, setSearchedConfig]; + }, })); +jest.mock('react-hook-form', () => { + const actual = jest.requireActual('react-hook-form'); + + return { + ...actual, + useForm: (options: unknown) => { + const form = actual.useForm(options); + const originalReset = form.reset; + const reset = React.useCallback( + (...args: Parameters) => { + mockFormReset(...args); + return originalReset(...args); + }, + [originalReset], + ); + return { ...form, reset }; + }, + }; +}); + jest.mock('@/source', () => ({ getEventBody: () => 'Body', getFirstTimestampValueExpression: () => 'Timestamp', @@ -106,6 +144,8 @@ jest.mock('@/savedSearch', () => ({ })); jest.mock('@/searchFilters', () => ({ + canonicalizeFilterQuery: (...args: unknown[]) => + mockCanonicalizeFilterQuery(...args), useSearchPageFilterState: () => ({ filters: [], whereSuggestions: [], @@ -134,7 +174,11 @@ jest.mock('../hooks/useMetadata', () => ({ isLoading: false, }), useColumns: () => ({ - data: undefined, + data: mockColumns, + isLoading: false, + }), + useJsonColumns: () => ({ + data: mockJsonColumns, isLoading: false, }), })); @@ -245,7 +289,14 @@ jest.mock('@/utils', () => ({ initialValue, jest.fn(), ], - usePrevious: (value: unknown) => value, + usePrevious: (value: unknown) => { + const ref = React.useRef(value); + const previous = ref.current; + React.useEffect(() => { + ref.current = value; + }, [value]); + return previous; + }, })); jest.mock('@tanstack/react-query', () => ({ @@ -257,6 +308,10 @@ describe('DBSearchPage direct trace flow', () => { jest.clearAllMocks(); latestDirectTracePanelProps = null; mockDirectTraceId = 'trace-123'; + mockApplySearchedConfigUpdates = false; + mockColumns = undefined; + mockJsonColumns = []; + mockCanonicalizeFilterQuery.mockImplementation(filters => filters); mockSearchedConfig = { source: undefined, where: '', @@ -380,4 +435,52 @@ describe('DBSearchPage direct trace flow', () => { expect(mockSetDirectTraceId).toHaveBeenCalledWith(null); }); + + it('canonicalizes only the applied filters without resetting form drafts', async () => { + mockDirectTraceId = null; + mockApplySearchedConfigUpdates = true; + const staleFilters = [ + { + type: 'sql', + condition: "ResourceAttributes['k8s.namespace.name'] IN ('backend')", + }, + ]; + const canonicalFilters = [ + { + type: 'sql', + condition: + "toString(ResourceAttributes.`k8s`.`namespace`.`name`) IN ('backend')", + }, + ]; + mockSearchedConfig = { + source: 'log-source', + where: 'ServiceName:api', + select: 'Timestamp,Body', + whereLanguage: 'lucene', + filters: staleFilters, + orderBy: 'Timestamp DESC', + }; + mockColumns = [ + { + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + }, + ]; + mockJsonColumns = ['ResourceAttributes']; + mockCanonicalizeFilterQuery.mockReturnValue(canonicalFilters); + + renderWithMantine(); + + await waitFor(() => { + expect(mockSetSearchedConfig).toHaveBeenCalledWith({ + filters: canonicalFilters, + }); + }); + await waitFor(() => { + expect(mockSearchedConfig.filters).toEqual(canonicalFilters); + }); + + expect(mockSetSearchedConfig).toHaveBeenCalledTimes(1); + expect(mockFormReset).not.toHaveBeenCalled(); + }); }); diff --git a/packages/app/src/__tests__/searchFilters.test.ts b/packages/app/src/__tests__/searchFilters.test.ts index 37e6e03355..b72781daf8 100644 --- a/packages/app/src/__tests__/searchFilters.test.ts +++ b/packages/app/src/__tests__/searchFilters.test.ts @@ -4,6 +4,7 @@ import { act, renderHook } from '@testing-library/react'; import { areFiltersEqual, + canonicalizeFilterQuery, parseQuery, useSearchPageFilterState, } from '@/searchFilters'; @@ -896,6 +897,264 @@ describe('searchFilters', () => { ]); }); + it('serializes JSON column filters as string expressions', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [], + onFilterChange: onFilterChangeLocal, + knownColumns: new Set(['ResourceAttributes']), + jsonColumns: ['ResourceAttributes'], + }), + ); + + act(() => { + result.current.setFilterValue( + 'ResourceAttributes.k8s.namespace.name', + 'alert-service', + 'only', + ); + }); + + expect(onFilterChangeLocal).toHaveBeenLastCalledWith([ + { + type: 'sql', + condition: + "toString(ResourceAttributes.`k8s`.`namespace`.`name`) IN ('alert-service')", + }, + ]); + }); + + it('serializes non-string JSON column filters as string expressions', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [], + onFilterChange: onFilterChangeLocal, + knownColumns: new Set(['ResourceAttributes']), + jsonColumns: ['ResourceAttributes'], + }), + ); + + act(() => { + result.current.setFilterValue( + 'ResourceAttributes.cloud.account.id', + '47452524847', + 'only', + ); + }); + + expect(onFilterChangeLocal).toHaveBeenLastCalledWith([ + { + type: 'sql', + condition: + "toString(ResourceAttributes.`cloud`.`account`.`id`) IN ('47452524847')", + }, + ]); + }); + + it('hydrates string JSON column filters back to clean keys', () => { + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { + type: 'sql', + condition: + "toString(ResourceAttributes.`k8s`.`namespace`.`name`) IN ('alert-service')", + }, + ], + onFilterChange: jest.fn(), + knownColumns: new Set(['ResourceAttributes']), + jsonColumns: ['ResourceAttributes'], + }), + ); + + expect(result.current.filters).toEqual({ + 'ResourceAttributes.k8s.namespace.name': { + included: new Set(['alert-service']), + excluded: new Set(), + }, + }); + }); + + it('hydrates legacy typed JSON column filters back to clean keys', () => { + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { + type: 'sql', + condition: + "ResourceAttributes.`k8s`.`namespace`.`name`.:String IN ('alert-service')", + }, + ], + onFilterChange: jest.fn(), + knownColumns: new Set(['ResourceAttributes']), + jsonColumns: ['ResourceAttributes'], + }), + ); + + expect(result.current.filters).toEqual({ + 'ResourceAttributes.k8s.namespace.name': { + included: new Set(['alert-service']), + excluded: new Set(), + }, + }); + }); + + it('canonicalizes persisted JSON column filters as string expressions', () => { + expect( + canonicalizeFilterQuery( + [ + { + type: 'sql', + condition: + "ResourceAttributes['k8s.namespace.name'] IN ('traefik-private')", + }, + ], + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ), + ).toEqual([ + { + type: 'sql', + condition: + "toString(ResourceAttributes.`k8s`.`namespace`.`name`) IN ('traefik-private')", + }, + ]); + }); + + it.each([ + [ + 'bracket form', + "ResourceAttributes['k8s.namespace.name'] IN ('traefik-private')", + ], + [ + 'typed subcolumn form', + "ResourceAttributes.`k8s`.`namespace`.`name`.:String IN ('traefik-private')", + ], + [ + 'string expression form', + "toString(ResourceAttributes.`k8s`.`namespace`.`name`) IN ('traefik-private')", + ], + ])( + 'canonicalizes JSON filters idempotently from %s', + (_name, condition) => { + const filters = [{ type: 'sql' as const, condition }]; + const once = canonicalizeFilterQuery( + filters, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + const twice = canonicalizeFilterQuery( + once, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + + expect(twice).toEqual(once); + }, + ); + + it('canonicalizes escaped JSON paths idempotently', () => { + const filters = [ + { + type: 'sql' as const, + condition: + "toString(ResourceAttributes.`path\\\\segment`.`tick``segment`) IN ('value')", + }, + ]; + + const once = canonicalizeFilterQuery( + filters, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + const twice = canonicalizeFilterQuery( + once, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + + expect(once).toEqual(filters); + expect(twice).toEqual(once); + }); + + it('canonicalizes empty JSON paths idempotently', () => { + const legacyFilters = [ + { + type: 'sql' as const, + condition: "ResourceAttributes[''] IN ('value')", + }, + ]; + const canonicalFilters = [ + { + type: 'sql' as const, + condition: + "JSONExtractString(toJSONString(ResourceAttributes), '') IN ('value')", + }, + ]; + + const once = canonicalizeFilterQuery( + legacyFilters, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + const twice = canonicalizeFilterQuery( + once, + new Set(['ResourceAttributes']), + ['ResourceAttributes'], + ); + + expect(once).toEqual(canonicalFilters); + expect(twice).toEqual(once); + }); + + it('does not canonicalize non-JSON sidebar filters', () => { + const filters = [ + { + type: 'sql' as const, + condition: "SpanKind IN ('Server')", + }, + ]; + + expect( + canonicalizeFilterQuery(filters, new Set(['ResourceAttributes']), [ + 'ResourceAttributes', + ]), + ).toBe(filters); + }); + + it('does not canonicalize compound SQL predicates that contain sidebar clauses', () => { + const filters = [ + { + type: 'sql' as const, + condition: "SpanName = 'foo' AND SpanKind IN ('Server')", + }, + ]; + + expect( + canonicalizeFilterQuery(filters, new Set(['ResourceAttributes']), [ + 'ResourceAttributes', + ]), + ).toBe(filters); + }); + + it('does not drop surrounding predicates while canonicalizing legacy JSON filters', () => { + const filters = [ + { + type: 'sql' as const, + condition: + "SpanName = 'foo' AND ResourceAttributes['k8s.namespace.name'] IN ('traefik-private')", + }, + ]; + + expect( + canonicalizeFilterQuery(filters, new Set(['ResourceAttributes']), [ + 'ResourceAttributes', + ]), + ).toBe(filters); + }); + it('updating filter query', () => { const { result } = renderHook(() => useSearchPageFilterState({ diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index 67a154aae7..f727aa0338 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -1,4 +1,4 @@ -import { memo, useCallback, useEffect, useMemo, useState } from 'react'; +import { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import cx from 'classnames'; import { TableMetadata, @@ -905,7 +905,9 @@ export const FilterGroup = ({ distributionKey, onRangeChange, }: FilterGroupProps) => { - const [isExpanded, setExpanded] = useState(isDefaultExpanded ?? false); + const [isExpanded, setIsExpanded] = useState(isDefaultExpanded ?? false); + const hasUserToggledExpansionRef = useRef(false); + const previousDefaultExpandedRef = useRef(Boolean(isDefaultExpanded)); const [showDistributions, setShowDistributions] = useState(false); const [isFetchingDistribution, setIsFetchingDistribution] = useState(false); @@ -919,7 +921,7 @@ export const FilterGroup = ({ const toggleShowDistributions = useCallback(() => { setShowDistributions(prev => { if (!prev) { - setExpanded(true); + setIsExpanded(true); } return !prev; }); @@ -929,17 +931,39 @@ export const FilterGroup = ({ setShowDistributions(false); }, []); - useEffect(() => { - if (isDefaultExpanded) { - setExpanded(true); - } - }, [isDefaultExpanded]); - const totalAppliedFiltersSize = selectedValues.included.size + selectedValues.excluded.size + (hasRange ? 1 : 0); + const shouldExpandForLoadMore = + !optionsLoading && + options.length === 0 && + totalAppliedFiltersSize === 0 && + !hasLoadedMore && + !loadMoreLoading; + + useEffect(() => { + const becameDefaultExpanded = + !previousDefaultExpandedRef.current && Boolean(isDefaultExpanded); + previousDefaultExpandedRef.current = Boolean(isDefaultExpanded); + + if ( + becameDefaultExpanded || + (shouldExpandForLoadMore && !hasUserToggledExpansionRef.current) + ) { + setIsExpanded(true); + } + }, [isDefaultExpanded, shouldExpandForLoadMore]); + + const handleLoadMore = useCallback( + (key: string) => { + setIsExpanded(true); + onLoadMore(key); + }, + [onLoadMore], + ); + const hasOptions = options.length > 0 || totalAppliedFiltersSize > 0; // An empty map attribute key (e.g. LogAttributes['']) yields an empty name, @@ -953,7 +977,8 @@ export const FilterGroup = ({ classNames={{ chevron: classes.chevron }} value={isExpanded ? displayName : null} onChange={v => { - setExpanded(v === displayName); + hasUserToggledExpansionRef.current = true; + setIsExpanded(v === displayName); }} > @@ -1037,7 +1062,7 @@ export const FilterGroup = ({ onPinClick={valuePins.onPinClick} isSharedPinned={valuePins.isSharedPinned} onSharedPinClick={valuePins.onSharedPinClick} - onLoadMore={onLoadMore} + onLoadMore={handleLoadMore} loadMoreLoading={loadMoreLoading} hasLoadedMore={hasLoadedMore} chartConfig={chartConfig} @@ -1458,6 +1483,7 @@ const DBSearchPageFiltersComponent = ({ sqlKey: toQuotedClickHouseKeyExpression( child.key, knownColumns, + { jsonColumns: jsonColumns ?? [] }, ), }))} selectedValues={group.children.reduce((acc, child) => { @@ -1524,6 +1550,7 @@ const DBSearchPageFiltersComponent = ({ const facetSqlKey = toQuotedClickHouseKeyExpression( facet.key, knownColumns, + { jsonColumns: jsonColumns ?? [] }, ); return ( { }); }); + describe('JSON key rendering', () => { + it('renders JSON facet keys as toString expressions in the raw-tables pipeline', () => { + setupDefaultMocks({ withMVs: false }); + useColumns.mockReturnValue({ + data: [ + { name: 'Timestamp', type: 'DateTime' }, + { name: 'ResourceAttributes', type: 'JSON' }, + ], + isLoading: false, + } as any); + useJsonColumns.mockReturnValue({ + data: ['ResourceAttributes'], + } as any); + useAllFields.mockReturnValue({ + data: [ + { + path: ['ResourceAttributes', 'k8s', 'namespace', 'name'], + type: 'String', + jsType: 'string', + }, + ], + } as any); + + const { wrapper } = makeWrapper(); + + renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'exact', + }), + { wrapper }, + ); + + expect(useGetKeyValues.mock.calls.at(-1)?.[0].keys).toEqual([ + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`)', + ]); + }); + }); + describe('loadMoreFacetsForKey (raw-tables pipeline)', () => { it('reports the key as loading while the fetch is in flight, then clears it', async () => { setupDefaultMocks({ withMVs: false }); @@ -605,6 +646,62 @@ describe('useFetchFacets', () => { ]); }); + it('replaces an extra facet when concurrent requests return the same key', async () => { + setupDefaultMocks({ withMVs: false }); + useGetKeyValues.mockReturnValue({ + data: [{ key: 'ServiceName', value: ['api'] }], + isLoading: false, + isFetching: false, + error: null, + } as any); + + let resolveFirstLoadMore: (val: unknown) => void = () => undefined; + const firstLoadMorePromise = new Promise(resolve => { + resolveFirstLoadMore = resolve; + }); + let resolveSecondLoadMore: (val: unknown) => void = () => undefined; + const secondLoadMorePromise = new Promise(resolve => { + resolveSecondLoadMore = resolve; + }); + useMetadataWithSettings.mockReturnValue({ + getKeyValuesWithMVs: jest + .fn() + .mockReturnValueOnce(firstLoadMorePromise) + .mockReturnValueOnce(secondLoadMorePromise), + } as any); + + const { wrapper } = makeWrapper(); + const { result } = renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'exact', + }), + { wrapper }, + ); + + let firstPending: Promise; + let secondPending: Promise; + act(() => { + firstPending = result.current.loadMoreFacetsForKey('NewKey'); + secondPending = result.current.loadMoreFacetsForKey('NewKey'); + }); + + await act(async () => { + resolveFirstLoadMore([{ key: 'NewKey', value: ['first'] }]); + await firstPending; + resolveSecondLoadMore([{ key: 'NewKey', value: ['second'] }]); + await secondPending; + }); + + expect(result.current.data.keyValues).toEqual([ + { key: 'ServiceName', value: ['api'] }, + { key: 'NewKey', value: ['second'] }, + ]); + }); + it('does not mutate state when the load-more strategy returns undefined (e.g. on error)', async () => { setupDefaultMocks({ withMVs: false }); useGetKeyValues.mockReturnValue({ @@ -681,6 +778,56 @@ describe('useFetchFacets', () => { { key: 'ServiceName', value: ['api', 'web'] }, ]); }); + + it('renders JSON keys as toString expressions before delegating to getAllKeyValues', async () => { + setupDefaultMocks({ withMVs: true }); + useColumns.mockReturnValue({ + data: [ + { name: 'Timestamp', type: 'DateTime' }, + { name: 'ResourceAttributes', type: 'JSON' }, + ], + isLoading: false, + } as any); + useJsonColumns.mockReturnValue({ + data: ['ResourceAttributes'], + } as any); + const getAllKeyValues = jest.fn().mockResolvedValue([ + { + key: 'ResourceAttributes.k8s.namespace.name', + value: ['api', 'web'], + }, + ]); + useMetadataWithSettings.mockReturnValue({ + getAllKeyValues, + getKeyValuesWithMVs: jest.fn(), + } as any); + + const { wrapper } = makeWrapper(); + const { result } = renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'all', + }), + { wrapper }, + ); + + await act(async () => { + await result.current.loadMoreFacetsForKey( + 'ResourceAttributes.k8s.namespace.name', + ); + }); + + expect(getAllKeyValues).toHaveBeenCalledWith( + expect.objectContaining({ + keyExpressions: [ + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`)', + ], + }), + ); + }); }); describe('extraFacets reset on prop change', () => { @@ -913,5 +1060,137 @@ describe('useFetchFacets', () => { expect(result.current.extraFacetKeys.size).toBe(0); }); + + it('ignores stale load-more results after the query scope changes', async () => { + setupDefaultMocks({ withMVs: false }); + useGetKeyValues.mockReturnValue({ + data: [{ key: 'ServiceName', value: ['api'] }], + isLoading: false, + isFetching: false, + error: null, + } as any); + + let resolveLoadMore: (val: unknown) => void = () => undefined; + const loadMorePromise = new Promise(resolve => { + resolveLoadMore = resolve; + }); + useMetadataWithSettings.mockReturnValue({ + getKeyValuesWithMVs: jest.fn().mockReturnValue(loadMorePromise), + } as any); + + const { wrapper } = makeWrapper(); + const { result, rerender } = renderHook( + (props: { chartConfig: BuilderChartConfigWithDateRange }) => + useFetchFacets({ + chartConfig: props.chartConfig, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'exact', + }), + { wrapper, initialProps: { chartConfig: CHART_CONFIG } }, + ); + + let pending: Promise; + act(() => { + pending = result.current.loadMoreFacetsForKey('NewKey'); + }); + + rerender({ + chartConfig: { ...CHART_CONFIG, where: 'level = "error"' }, + }); + + await act(async () => { + resolveLoadMore([{ key: 'NewKey', value: ['n1'] }]); + await pending; + }); + + expect(result.current.extraFacetKeys.size).toBe(0); + expect(result.current.data.keyValues).toEqual([ + { key: 'ServiceName', value: ['api'] }, + ]); + }); + + it('does not let a stale same-key load-more request clear a newer loading state', async () => { + setupDefaultMocks({ withMVs: false }); + useGetKeyValues.mockReturnValue({ + data: [{ key: 'ServiceName', value: ['api'] }], + isLoading: false, + isFetching: false, + error: null, + } as any); + + let resolveFirstLoadMore: (val: unknown) => void = () => undefined; + const firstLoadMorePromise = new Promise(resolve => { + resolveFirstLoadMore = resolve; + }); + let resolveSecondLoadMore: (val: unknown) => void = () => undefined; + const secondLoadMorePromise = new Promise(resolve => { + resolveSecondLoadMore = resolve; + }); + useMetadataWithSettings.mockReturnValue({ + getKeyValuesWithMVs: jest + .fn() + .mockReturnValueOnce(firstLoadMorePromise) + .mockReturnValueOnce(secondLoadMorePromise), + } as any); + + const { wrapper } = makeWrapper(); + const { result, rerender } = renderHook( + (props: { chartConfig: BuilderChartConfigWithDateRange }) => + useFetchFacets({ + chartConfig: props.chartConfig, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'exact', + }), + { wrapper, initialProps: { chartConfig: CHART_CONFIG } }, + ); + + let stalePending: Promise; + act(() => { + stalePending = result.current.loadMoreFacetsForKey('ServiceName'); + }); + + await waitFor(() => { + expect(result.current.loadMoreLoadingKeys.has('ServiceName')).toBe( + true, + ); + }); + + rerender({ + chartConfig: { ...CHART_CONFIG, where: 'level = "error"' }, + }); + + let freshPending: Promise; + act(() => { + freshPending = result.current.loadMoreFacetsForKey('ServiceName'); + }); + + await waitFor(() => { + expect(result.current.loadMoreLoadingKeys.has('ServiceName')).toBe( + true, + ); + }); + + await act(async () => { + resolveFirstLoadMore([{ key: 'ServiceName', value: ['api', 'stale'] }]); + await stalePending; + }); + + expect(result.current.loadMoreLoadingKeys.has('ServiceName')).toBe(true); + expect(result.current.extraFacetKeys.size).toBe(0); + + await act(async () => { + resolveSecondLoadMore([ + { key: 'ServiceName', value: ['api', 'fresh'] }, + ]); + await freshPending; + }); + + expect(result.current.loadMoreLoadingKeys.has('ServiceName')).toBe(false); + expect(result.current.data.keyValues).toEqual([ + { key: 'ServiceName', value: ['api', 'fresh'] }, + ]); + }); }); }); diff --git a/packages/app/src/components/DBSearchPageFilters/utils.test.ts b/packages/app/src/components/DBSearchPageFilters/__tests__/utils.test.ts similarity index 75% rename from packages/app/src/components/DBSearchPageFilters/utils.test.ts rename to packages/app/src/components/DBSearchPageFilters/__tests__/utils.test.ts index 10fa53c521..5dd61d5ac2 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.test.ts +++ b/packages/app/src/components/DBSearchPageFilters/__tests__/utils.test.ts @@ -6,7 +6,7 @@ import { groupFacetsByBaseName, toClickHouseKeyExpression, toQuotedClickHouseKeyExpression, -} from './utils'; +} from '@/components/DBSearchPageFilters/utils'; describe('cleanClickHouseExpression', () => { it('strips backticks from a bare quoted identifier', () => { @@ -19,6 +19,51 @@ describe('cleanClickHouseExpression', () => { ).toBe('ResourceAttributes.hdx.sdk'); }); + it('decodes escaped JSON path segments', () => { + expect( + cleanClickHouseExpression( + 'toString(ResourceAttributes.`path\\\\segment`.`tick``segment`)', + ), + ).toBe('ResourceAttributes.path\\segment.tick`segment'); + }); + + it('decodes the empty JSON key expression', () => { + expect( + cleanClickHouseExpression( + "JSONExtractString(toJSONString(ResourceAttributes), '')", + ), + ).toBe("ResourceAttributes['']"); + }); + + it('strips typed JSON subcolumn suffixes', () => { + expect( + cleanClickHouseExpression( + 'ResourceAttributes.`k8s`.`namespace`.`name`.:String', + ), + ).toBe('ResourceAttributes.k8s.namespace.name'); + expect( + cleanClickHouseExpression( + 'ResourceAttributes.`http`.`status_code`.:Int64', + ), + ).toBe('ResourceAttributes.http.status_code'); + expect( + cleanClickHouseExpression('ResourceAttributes.`duration_ms`.:Float64'), + ).toBe('ResourceAttributes.duration_ms'); + expect( + cleanClickHouseExpression('LogAttributes.`time`.:`DateTime64(9)`'), + ).toBe('LogAttributes.time'); + expect( + cleanClickHouseExpression( + 'ResourceAttributes.`http`.`headers`.:LowCardinality(Nullable(String))', + ), + ).toBe('ResourceAttributes.http.headers'); + expect( + cleanClickHouseExpression( + 'ResourceAttributes.`status_codes`.:Array(Nullable(Int64))', + ), + ).toBe('ResourceAttributes.status_codes'); + }); + it('leaves a plain identifier unchanged', () => { expect(cleanClickHouseExpression('ServiceName')).toBe('ServiceName'); }); @@ -240,6 +285,70 @@ describe('toQuotedClickHouseKeyExpression', () => { ).toBe("LogAttributes['host']"); }); + it('converts JSON dot-form keys to string expressions', () => { + expect( + toQuotedClickHouseKeyExpression( + 'ResourceAttributes.k8s.namespace.name', + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe('toString(ResourceAttributes.`k8s`.`namespace`.`name`)'); + }); + + it('converts JSON bracket-form keys to string expressions', () => { + expect( + toQuotedClickHouseKeyExpression( + "ResourceAttributes['k8s.namespace.name']", + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe('toString(ResourceAttributes.`k8s`.`namespace`.`name`)'); + }); + + it('escapes backslashes inside JSON path segments', () => { + expect( + toQuotedClickHouseKeyExpression( + "ResourceAttributes['foo\\bar.name']", + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe('toString(ResourceAttributes.`foo\\\\bar`.`name`)'); + }); + + it('renders an empty JSON path without invalid dot notation', () => { + expect( + toQuotedClickHouseKeyExpression( + "ResourceAttributes['']", + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe("JSONExtractString(toJSONString(ResourceAttributes), '')"); + }); + + it('converts typed JSON subcolumn keys to string expressions', () => { + expect( + toQuotedClickHouseKeyExpression( + 'ResourceAttributes.`cloud`.`account`.`id`.:Int64', + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe('toString(ResourceAttributes.`cloud`.`account`.`id`)'); + expect( + toQuotedClickHouseKeyExpression( + 'LogAttributes.`time`.:`DateTime64(9)`', + new Set(['LogAttributes']), + { jsonColumns: ['LogAttributes'] }, + ), + ).toBe('toString(LogAttributes.`time`)'); + expect( + toQuotedClickHouseKeyExpression( + 'ResourceAttributes.`status_codes`.:Array(Nullable(Int64))', + new Set(['ResourceAttributes']), + { jsonColumns: ['ResourceAttributes'] }, + ), + ).toBe('toString(ResourceAttributes.`status_codes`)'); + }); + it('quotes only the root of a bracket map access that needs it', () => { expect(toQuotedClickHouseKeyExpression("my-map['k']", knownColumns)).toBe( "`my-map`['k']", diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index 8c4b924bb8..47c3cd3c81 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import produce from 'immer'; import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; import { @@ -135,12 +135,14 @@ function useFacets({ const sqlKeyToUiKey = new Map(); const escapedKeysToFetch = keysToFetch.map(key => { - const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns); + const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns, { + jsonColumns: jsonColumns ?? [], + }); sqlKeyToUiKey.set(sqlKey, key); return sqlKey; }); return { escapedKeysToFetch, sqlKeyToUiKey }; - }, [isColumnsLoading, keysToFetch, knownColumns]); + }, [isColumnsLoading, jsonColumns, keysToFetch, knownColumns]); const facetsChartConfig = useMemo( () => @@ -174,7 +176,9 @@ function useFacets({ const loadMoreFacetsForKey = useCallback( async (key: string): Promise => { try { - const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns); + const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns, { + jsonColumns: jsonColumns ?? [], + }); if (mode === 'exact') { const strippedFilterState: FilterState = { ...filterState }; delete strippedFilterState[key]; @@ -184,7 +188,11 @@ function useFacets({ ...chartConfig, dateRange, filters: filtersToQuery( - escapeFilterStateKeys(strippedFilterState, knownColumns), + escapeFilterStateKeys( + strippedFilterState, + knownColumns, + jsonColumns ?? [], + ), { dateTimeColumns }, ), }, @@ -234,6 +242,7 @@ function useFacets({ source, filterState, dateTimeColumns, + jsonColumns, ], ); @@ -275,6 +284,7 @@ export function useFetchFacets({ }); const [extraFacets, setExtraFacets] = useState(null); + const loadMoreGenerationRef = useRef(0); const facets = useMemo(() => { const base = facetsQuery.data.keyValues; const hasExtras = !!extraFacets && extraFacets.length > 0; @@ -321,26 +331,35 @@ export function useFetchFacets({ const areExtraFacetsLoading = loadMoreLoadingKeys.size > 0; const loadMoreFacetsForKey = useCallback( async (key: string) => { + const requestGeneration = loadMoreGenerationRef.current; const strategy = facetsQuery.loadMoreFacetsForKey; setLoadMoreLoadingKeys(prev => produce(prev, draft => { draft.add(key); }), ); - const newFacet = await strategy(key); - if (newFacet) { - setExtraFacets(prev => [...(prev ?? []), newFacet]); - setExtraFacetKeys(prev => - produce(prev, draft => { - draft.add(key); - }), - ); + try { + const newFacet = await strategy(key); + if (newFacet && requestGeneration === loadMoreGenerationRef.current) { + setExtraFacets(prev => [ + ...(prev ?? []).filter(facet => facet.key !== newFacet.key), + newFacet, + ]); + setExtraFacetKeys(prev => + produce(prev, draft => { + draft.add(key); + }), + ); + } + } finally { + if (requestGeneration === loadMoreGenerationRef.current) { + setLoadMoreLoadingKeys(prev => + produce(prev, draft => { + draft.delete(key); + }), + ); + } } - setLoadMoreLoadingKeys(prev => - produce(prev, draft => { - draft.delete(key); - }), - ); }, [facetsQuery.loadMoreFacetsForKey], ); @@ -348,9 +367,12 @@ export function useFetchFacets({ // Clear extras when the query scope that produced them changes; otherwise // they'd persist against a query they were never fetched for. useEffect(() => { - // eslint-disable-next-line react-hooks/set-state-in-effect + loadMoreGenerationRef.current += 1; + setExtraFacets(null); setExtraFacetKeys(new Set()); + + setLoadMoreLoadingKeys(new Set()); }, [ sourceId, dateRange, diff --git a/packages/app/src/components/DBSearchPageFilters/utils.ts b/packages/app/src/components/DBSearchPageFilters/utils.ts index 86f0465426..c73677924a 100644 --- a/packages/app/src/components/DBSearchPageFilters/utils.ts +++ b/packages/app/src/components/DBSearchPageFilters/utils.ts @@ -1,16 +1,33 @@ // Utility functions for parsing and grouping map-like field names -import SqlString from 'sqlstring'; -import { parseKeyPath } from '@hyperdx/common-utils/dist/core/metadata'; +import { + parseKeyPath, + parseRenderedJsonStringExpression, + quoteIdentifierIfNeeded, + renderJsonStringExpression, + stripClickHouseJsonTypeSuffix, +} from '@hyperdx/common-utils/dist/core/metadata'; import type { FilterState } from '@hyperdx/common-utils/dist/filters'; import { mergePath } from '@/utils'; // Clean ClickHouse expressions to extract clean property paths export function cleanClickHouseExpression(key: string): string { + const renderedJsonKey = parseRenderedJsonStringExpression(key); + if (renderedJsonKey) { + return renderedJsonKey.key === '' + ? `${renderedJsonKey.column}['']` + : `${renderedJsonKey.column}.${renderedJsonKey.key}`; + } + // Remove toString() wrapper if present let cleanKey = key.replace(/^toString\((.+)\)$/, '$1'); + // Typed JSON subcolumns use a terminal ClickHouse type suffix + // (ResourceAttributes.`k8s`.`namespace`.`name`.:String). The sidebar keeps + // clean, type-free keys in memory so persisted URL filters match facet keys. + cleanKey = stripClickHouseJsonTypeSuffix(cleanKey); + // Convert backtick dot notation to clean dot notation // e.g., `host`.`arch` -> host.arch cleanKey = cleanKey.replace(/`([^`]+)`/g, '$1'); @@ -187,16 +204,9 @@ export function toClickHouseKeyExpression(key: string): string { ); } -/** - * Quote a single identifier if it isn't already a valid bare ClickHouse identifier. - * @param id - The identifier to quote - * @returns The quoted identifier if needed, otherwise the original identifier - */ -function quoteIdentifierIfNeeded(id: string): string { - return /^[A-Za-z_][A-Za-z0-9_]*$/.test(id) - ? id - : SqlString.escapeId(id, true); -} +type KeyExpressionOptions = { + jsonColumns?: readonly string[]; +}; /** * Coerce a filterState key into a ClickHouse expression suitable for raw SQL, @@ -209,13 +219,24 @@ function quoteIdentifierIfNeeded(id: string): string { export function toQuotedClickHouseKeyExpression( key: string, knownColumns: Set, + options: KeyExpressionOptions = {}, ): string { + const jsonColumns = new Set(options.jsonColumns ?? []); + // A whole-key match against a real column wins: quote the entire name as one // identifier (handles flat columns whose name contains dots/hyphens/etc.). if (knownColumns.has(key)) { return quoteIdentifierIfNeeded(key); } + const parsedKey = parseMapFieldName(key); + if (parsedKey && jsonColumns.has(parsedKey.baseName)) { + return renderJsonStringExpression( + parsedKey.baseName, + parsedKey.propertyPath, + ); + } + // Normalize dot-form (ResourceAttributes.host.name) to map access form (ResourceAttributes['host.name']) const expr = toClickHouseKeyExpression(key); diff --git a/packages/app/src/components/__tests__/DBSearchPageFilters.test.tsx b/packages/app/src/components/__tests__/DBSearchPageFilters.test.tsx index 0353c68d4f..02cc50ceeb 100644 --- a/packages/app/src/components/__tests__/DBSearchPageFilters.test.tsx +++ b/packages/app/src/components/__tests__/DBSearchPageFilters.test.tsx @@ -1,3 +1,4 @@ +import { useState } from 'react'; import { UseQueryResult } from '@tanstack/react-query'; import { screen, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; @@ -655,6 +656,203 @@ describe('FilterGroup', () => { expect(screen.getAllByTestId(/filter-checkbox-.+-input/)).toHaveLength(3); }); + it('should show Load more for empty facets that have not loaded more yet', async () => { + const onLoadMore = jest.fn(); + renderWithMantine( + , + ); + + const loadMoreButton = screen.getByTestId('filter-load-more-Test Filter'); + await userEvent.click(loadMoreButton); + + expect(onLoadMore).toHaveBeenCalledWith('Test Filter'); + }); + + it('should stay expanded when an empty facet receives options', async () => { + const EmptyFacetHarness = () => { + const [options, setOptions] = useState([]); + + return ( + <> + + + + ); + }; + + renderWithMantine(); + + const panel = screen.getByTestId('filter-group-panel'); + expect(panel).toHaveAttribute('aria-hidden', 'false'); + + await userEvent.click(screen.getByTestId('load-options')); + + expect(panel).toHaveAttribute('aria-hidden', 'false'); + expect(screen.getByText('loaded')).toBeInTheDocument(); + }); + + it('should stay expanded when the default-expanded state clears', async () => { + const DefaultExpandedHarness = () => { + const [isDefaultExpanded, setIsDefaultExpanded] = useState(true); + + return ( + <> + + + + ); + }; + + renderWithMantine(); + + const panel = screen.getByTestId('filter-group-panel'); + expect(panel).toHaveAttribute('aria-hidden', 'false'); + + await userEvent.click(screen.getByTestId('clear-default-expanded')); + + expect(panel).toHaveAttribute('aria-hidden', 'false'); + }); + + it('should not reopen an empty facet after it is manually collapsed', async () => { + const onLoadMore = jest.fn(); + const EmptyFacetHarness = () => { + const [optionsLoading, setOptionsLoading] = useState(false); + + return ( + <> + + + + ); + }; + + renderWithMantine(); + + const panel = screen.getByTestId('filter-group-panel'); + expect(panel).toHaveAttribute('aria-hidden', 'false'); + + await userEvent.click(screen.getByTestId('filter-group-control')); + expect(panel).toHaveAttribute('aria-hidden', 'true'); + + await userEvent.click(screen.getByTestId('toggle-options-loading')); + await userEvent.click(screen.getByTestId('toggle-options-loading')); + + expect(panel).toHaveAttribute('aria-hidden', 'true'); + }); + + it('should keep a default-expanded facet collapsed during a refetch', async () => { + const DefaultExpandedHarness = () => { + const [options, setOptions] = useState(defaultProps.options); + + return ( + <> + + + + ); + }; + + renderWithMantine(); + + const panel = screen.getByTestId('filter-group-panel'); + await userEvent.click(screen.getByTestId('filter-group-control')); + expect(panel).toHaveAttribute('aria-hidden', 'true'); + + await userEvent.click(screen.getByTestId('clear-options')); + + expect(panel).toHaveAttribute('aria-hidden', 'true'); + }); + + it('should reopen a manually collapsed facet when a filter is applied', async () => { + const AppliedFilterHarness = () => { + const [isDefaultExpanded, setIsDefaultExpanded] = useState(false); + + return ( + <> + + + + ); + }; + + renderWithMantine(); + + const panel = screen.getByTestId('filter-group-panel'); + const control = screen.getByTestId('filter-group-control'); + await userEvent.click(control); + await userEvent.click(control); + expect(panel).toHaveAttribute('aria-hidden', 'true'); + + await userEvent.click(screen.getByTestId('apply-filter')); + + expect(panel).toHaveAttribute('aria-hidden', 'false'); + }); + + it('should hide Load more for empty facets after load more has completed', () => { + renderWithMantine( + , + ); + + expect( + screen.queryByTestId('filter-load-more-Test Filter'), + ).not.toBeInTheDocument(); + }); + // https://github.com/hyperdxio/hyperdx/issues/2576 — an empty map attribute // key (e.g. LogAttributes['']) yields an empty name, which Mantine rejects // as an Accordion.Item value and crashes the filter panel. diff --git a/packages/app/src/searchFilters.tsx b/packages/app/src/searchFilters.tsx index ac193b7f26..eb758d8343 100644 --- a/packages/app/src/searchFilters.tsx +++ b/packages/app/src/searchFilters.tsx @@ -9,6 +9,7 @@ import type { Filter } from '@hyperdx/common-utils/dist/types'; import { cleanClickHouseExpression, + parseMapFieldName, toQuotedClickHouseKeyExpression, } from './components/DBSearchPageFilters/utils'; import { usePinnedFiltersApi, useUpdatePinnedFilters } from './pinnedFilters'; @@ -26,10 +27,13 @@ export const IS_ROOT_SPAN_COLUMN_NAME = 'isRootSpan'; export const escapeFilterStateKeys = ( filters: FilterState, knownColumns: Set, + jsonColumns: readonly string[] = [], ): FilterState => { const escaped: FilterState = {}; for (const [key, value] of Object.entries(filters)) { - escaped[toQuotedClickHouseKeyExpression(key, knownColumns)] = value; + escaped[ + toQuotedClickHouseKeyExpression(key, knownColumns, { jsonColumns }) + ] = value; } return escaped; }; @@ -43,6 +47,69 @@ const unescapeFilterStateKeys = (filters: FilterState): FilterState => { return cleaned; }; +const normalizeFilterCondition = (condition: string): string => + condition.replace(/\s+/g, ' ').trim(); + +const areSameFilterQueries = (a: Filter[], b: Filter[]): boolean => + a.length === b.length && + a.every((filter, idx) => { + const other = b[idx]; + return ( + other != null && + filter.type === other.type && + 'condition' in filter && + 'condition' in other && + normalizeFilterCondition(filter.condition) === + normalizeFilterCondition(other.condition) + ); + }); + +export const canonicalizeFilterQuery = ( + filters: Filter[], + knownColumns: Set, + jsonColumns: readonly string[] = [], + dateTimeColumns?: ReadonlyMap, +): Filter[] => { + if (filters.some(filter => filter.type !== 'sql')) { + return filters; + } + + const parsedFilters = parseQuery(filters).filters; + const cleanFilters = unescapeFilterStateKeys(parsedFilters); + const jsonColumnSet = new Set(jsonColumns); + const hasJsonFilter = Object.keys(cleanFilters).some(key => { + const parsed = parseMapFieldName(key); + return parsed != null && jsonColumnSet.has(parsed.baseName); + }); + if (!hasJsonFilter) { + return filters; + } + + // `parseQuery` intentionally extracts sidebar-style clauses from SQL, even + // inside compound predicates. Only rewrite filters that fully round-trip + // through the sidebar parser; otherwise canonicalization could drop unrelated + // SQL predicates from saved/URL state. + if ( + !areSameFilterQueries( + filtersToQuery(parsedFilters, { dateTimeColumns }), + filters, + ) + ) { + return filters; + } + + const escapedFilters = escapeFilterStateKeys( + cleanFilters, + knownColumns, + jsonColumns, + ); + const canonicalFilters = filtersToQuery(escapedFilters, { dateTimeColumns }); + + return canonicalFilters.length === filters.length + ? canonicalFilters + : filters; +}; + export const areFiltersEqual = (a: FilterState, b: FilterState) => { const aKeys = Object.keys(a); const bKeys = Object.keys(b); @@ -83,6 +150,7 @@ export const useSearchPageFilterState = ({ onFilterChange, dateTimeColumns, knownColumns, + jsonColumns = [], }: { searchQuery?: Filter[]; onFilterChange: (filters: Filter[]) => void; @@ -93,6 +161,7 @@ export const useSearchPageFilterState = ({ * (eg. service-name --> `service-name`). **/ knownColumns: Set; + jsonColumns?: readonly string[]; }) => { // Access knownColumns through a ref so the returned mutators (which depend on // updateFilterQuery) keep stable identities across knownColumns reference @@ -102,6 +171,10 @@ export const useSearchPageFilterState = ({ useEffect(() => { knownColumnsRef.current = knownColumns; }, [knownColumns]); + const jsonColumnsRef = useRef(jsonColumns); + useEffect(() => { + jsonColumnsRef.current = jsonColumns; + }, [jsonColumns]); // Persisted filters carry canonical (escaped) keys; convert back to the clean // keys the sidebar/comparisons use as they enter in-memory FilterState. @@ -133,6 +206,7 @@ export const useSearchPageFilterState = ({ const escapedFilters = escapeFilterStateKeys( newFilters, knownColumnsRef.current, + jsonColumnsRef.current, ); onFilterChange(filtersToQuery(escapedFilters, { dateTimeColumns })); }, diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 94d4d38772..d2140c2456 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -4,6 +4,8 @@ import { Metadata, MetadataCache, parseKeyPath, + parseRenderedJsonStringExpression, + renderJsonStringExpression, } from '@/core/metadata'; import * as renderChartConfigModule from '@/core/renderChartConfig'; import { timeFilterExpr } from '@/core/renderChartConfig'; @@ -693,7 +695,7 @@ describe('Metadata', () => { expect(renderChartConfigSpy).not.toHaveBeenCalled(); }); - it('renders JSON attribute keys as typed subcolumns', async () => { + it('renders JSON attribute keys as string expressions', async () => { jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => Promise.resolve( column === 'ResourceAttributes' @@ -722,7 +724,40 @@ describe('Metadata', () => { expect(actualConfig.with?.[0]).toMatchObject({ chartConfig: { select: - 'ResourceAttributes.`k8s`.`namespace`.`name`.:String as param0', + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`) as param0', + }, + }); + }); + + it('escapes backslashes while rendering JSON attribute keys', async () => { + jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => + Promise.resolve( + column === 'ResourceAttributes' + ? ({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any) + : undefined, + ), + ); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getKeyValues({ + chartConfig: mockChartConfig, + keys: ["ResourceAttributes['foo\\bar.name']"], + limit: 10, + source, + }); + + const actualConfig = renderChartConfigSpy.mock.calls[0][0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.with?.[0]).toMatchObject({ + chartConfig: { + select: 'toString(ResourceAttributes.`foo\\\\bar`.`name`) as param0', }, }); }); @@ -756,7 +791,7 @@ describe('Metadata', () => { expect(actualConfig.with?.[0]).toMatchObject({ chartConfig: { select: - 'ResourceAttributes.`foo`.`:String, count() AS injected`.:String as param0', + 'toString(ResourceAttributes.`foo`.`:String, count() AS injected`) as param0', }, }); }); @@ -1080,6 +1115,66 @@ describe('Metadata', () => { expect(configArg.select).toContain('param0'); }); + it('routes rendered JSON expressions through metadata MV rollup keys', async () => { + setupDefaultLogsSchema(); + jest.spyOn(metadata, 'getColumns').mockResolvedValue([ + { name: 'Timestamp', type: 'DateTime64(9)' }, + { + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + }, + ] as any); + jest + .spyOn(metadata as any, 'doMetadataMVsAggregateColumn') + .mockResolvedValue(true); + + const keyExpression = + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`)'; + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: [keyExpression], + }); + + const calls = (mockClickhouseClient.query as jest.Mock).mock.calls; + const lastCall = calls[calls.length - 1][0]; + expect(Object.values(lastCall.query_params)).toEqual( + expect.arrayContaining(['ResourceAttributes', 'k8s.namespace.name']), + ); + expect(Object.values(lastCall.query_params)).not.toContain( + 'NativeColumn', + ); + }); + + it('preserves rendered JSON expressions on the raw-table fallback path', async () => { + setupDefaultLogsSchema(); + jest.spyOn(metadata, 'getColumns').mockResolvedValue([ + { name: 'Timestamp', type: 'DateTime64(9)' }, + { + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + }, + ] as any); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + const keyExpression = + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`)'; + await metadata.getAllKeyValues({ + ...baseArgs, + metadataMVs: undefined, + keyExpressions: [keyExpression], + }); + + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + const innerSelect = configArg.with?.[0]?.chartConfig?.select as string; + expect(innerSelect).toContain(`${keyExpression} as param0`); + expect(innerSelect).not.toContain("ResourceAttributes['"); + }); + it('SQL-escapes map keys with single quotes on the raw-table fallback path (SQL-injection regression)', async () => { setupDefaultLogsSchema(); const renderChartConfigSpy = jest.spyOn( @@ -1535,7 +1630,7 @@ describe('Metadata', () => { }); }); - it('renders JSON distribution keys as typed subcolumns', async () => { + it('renders JSON distribution keys as string expressions', async () => { jest.spyOn(metadata, 'getColumn').mockImplementation(({ column }) => Promise.resolve( column === 'ResourceAttributes' @@ -1561,7 +1656,7 @@ describe('Metadata', () => { if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); expect(actualConfig.select).toBe( - 'ResourceAttributes.`k8s`.`namespace`.`name`.:String AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`) AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', ); expect(actualConfig.groupBy).toBe('__hdx_value'); }); @@ -1584,7 +1679,7 @@ describe('Metadata', () => { await metadata.getValuesDistribution({ chartConfig: mockChartConfig, - key: 'ResourceAttributes.k8s.namespace.name.:String', + key: 'ResourceAttributes.cloud.account.id.:Int64', source, }); @@ -1592,7 +1687,7 @@ describe('Metadata', () => { if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); expect(actualConfig.select).toBe( - 'ResourceAttributes.`k8s`.`namespace`.`name`.:String AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', + 'toString(ResourceAttributes.`cloud`.`account`.`id`) AS __hdx_value, count() as __hdx_count, __hdx_count / (sum(__hdx_count) OVER ()) * 100 AS __hdx_percentage', ); }); }); @@ -2078,7 +2173,7 @@ describe('Metadata', () => { expect(valuesCall.query).toContain('__TIME_FILTER__'); }); - it('uses typed JSON subcolumns for JSON attribute values', async () => { + it('uses string JSON expressions for JSON attribute values', async () => { const md = buildMetadata(); jest.spyOn(md, 'getColumn').mockResolvedValue({ name: 'ResourceAttributes', @@ -2100,11 +2195,38 @@ describe('Metadata', () => { const valuesCall = (mockClickhouseClient.query as jest.Mock).mock .calls[0][0]; expect(valuesCall.query).toContain( - 'ResourceAttributes.`k8s`.`namespace`.`name`.:String as value', + 'toString(ResourceAttributes.`k8s`.`namespace`.`name`) as value', ); expect(valuesCall.query).not.toContain('ResourceAttributes['); }); + it('uses the JSON value expression for an empty attribute key', async () => { + const md = buildMetadata(); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'ResourceAttributes', + type: 'JSON(max_dynamic_types=8, max_dynamic_paths=64)', + } as any); + + (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ + json: () => Promise.resolve({ data: [] }), + }); + + await md.getMapValues({ + databaseName: 'otel', + tableName: 'generic_logs', + column: 'ResourceAttributes', + key: '', + connectionId: 'conn-1', + }); + + expect(md.getColumn).toHaveBeenCalled(); + const valuesCall = (mockClickhouseClient.query as jest.Mock).mock + .calls[0][0]; + expect(valuesCall.query).toContain( + "JSONExtractString(toJSONString(ResourceAttributes), '') as value", + ); + }); + it('caches values distinctly for different dateRange values', async () => { const md = buildMetadata(); jest.spyOn(md, 'getColumn').mockResolvedValue({ @@ -2902,6 +3024,52 @@ describe('Metadata', () => { }); }); +describe('JSON string expressions', () => { + it('parses underscore-prefixed JSON columns', () => { + expect( + parseRenderedJsonStringExpression( + 'toString(_ResourceAttributes.`k8s`.`namespace`.`name`)', + ), + ).toEqual({ + column: '_ResourceAttributes', + key: 'k8s.namespace.name', + }); + }); + + it('round-trips escaped path segments', () => { + const expression = renderJsonStringExpression( + 'ResourceAttributes', + 'path\\segment.tick`segment', + ); + + expect(expression).toBe( + 'toString(ResourceAttributes.`path\\\\segment`.`tick``segment`)', + ); + expect(parseRenderedJsonStringExpression(expression)).toEqual({ + column: 'ResourceAttributes', + key: 'path\\segment.tick`segment', + }); + }); + + it('uses a parseable string expression for an empty JSON key', () => { + const expression = renderJsonStringExpression('ResourceAttributes', ''); + + expect(expression).toBe( + "JSONExtractString(toJSONString(ResourceAttributes), '')", + ); + expect(parseRenderedJsonStringExpression(expression)).toEqual({ + column: 'ResourceAttributes', + key: '', + }); + }); + + it('does not emit a dangling dot for separator-only JSON paths', () => { + expect(renderJsonStringExpression('ResourceAttributes', '.')).toBe( + "JSONExtractString(toJSONString(ResourceAttributes), '')", + ); + }); +}); + describe('parseKeyPath', () => { it('parses single-quoted bracket notation', () => { expect(parseKeyPath("ResourceAttributes['service.name']")).toEqual([ diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index eddd1675b2..f46554bde2 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -86,12 +86,12 @@ const unquoteIdentifier = (identifier: string): string => { return identifier; }; -const quoteJsonPathSegment = (segment: string): string => { +export const quoteJsonPathSegment = (segment: string): string => { const unquoted = unquoteIdentifier(segment); - return `\`${unquoted.replace(/`/g, '``')}\``; + return `\`${unquoted.replace(/\\/g, '\\\\').replace(/`/g, '``')}\``; }; -const quoteIdentifierIfNeeded = (identifier: string): string => { +export const quoteIdentifierIfNeeded = (identifier: string): string => { const unquoted = unquoteIdentifier(identifier); return /^[A-Za-z_][A-Za-z0-9_]*$/.test(unquoted) ? unquoted @@ -121,27 +121,182 @@ const identifierPattern = (identifier: string): string => { return `(?:\`${quotedEscaped}\`|${rawEscaped})`; }; -const JSON_STRING_TYPE_SUFFIX = '.:String'; +const isIdentifierStart = (char: string): boolean => + (char >= 'A' && char <= 'Z') || (char >= 'a' && char <= 'z') || char === '_'; -const renderJsonStringSubcolumn = ( +const isIdentifierChar = (char: string): boolean => + isIdentifierStart(char) || (char >= '0' && char <= '9') || char === '_'; + +const isClickHouseJsonTypeSuffix = (suffix: string): boolean => { + if (suffix.startsWith('`') && suffix.endsWith('`')) { + return suffix.length > 2 && !suffix.slice(1, -1).includes('`'); + } + + const parenIdx = suffix.indexOf('('); + const typeName = + parenIdx === -1 + ? suffix + : suffix.endsWith(')') + ? suffix.slice(0, parenIdx) + : ''; + + return ( + typeName.length > 0 && + isIdentifierStart(typeName[0]) && + [...typeName].every(isIdentifierChar) + ); +}; + +export const stripClickHouseJsonTypeSuffix = (jsonPath: string): string => { + const suffixIdx = jsonPath.lastIndexOf('.:'); + if (suffixIdx === -1) { + return jsonPath; + } + + const suffix = jsonPath.slice(suffixIdx + 2); + return isClickHouseJsonTypeSuffix(suffix) + ? jsonPath.slice(0, suffixIdx) + : jsonPath; +}; + +const parseClickHouseIdentifier = ( + expression: string, + startIdx = 0, +): { value: string; endIdx: number } | undefined => { + if (expression.charAt(startIdx) === '`') { + let value = ''; + for (let i = startIdx + 1; i < expression.length; i++) { + if (expression.charAt(i) !== '`') { + if (expression.charAt(i) === '\\') { + const escaped = expression.charAt(i + 1); + if (escaped) { + value += escaped; + i++; + continue; + } + } + value += expression.charAt(i); + continue; + } + + if (expression.charAt(i + 1) === '`') { + value += '`'; + i++; + continue; + } + + return { value, endIdx: i + 1 }; + } + return undefined; + } + + if (!isIdentifierStart(expression.charAt(startIdx))) { + return undefined; + } + + let endIdx = startIdx + 1; + while ( + endIdx < expression.length && + isIdentifierChar(expression.charAt(endIdx)) + ) { + endIdx++; + } + + return { value: expression.slice(startIdx, endIdx), endIdx }; +}; + +const parseRenderedJsonPath = ( + pathExpression: string, +): string[] | undefined => { + const segments: string[] = []; + let idx = 0; + + while (idx < pathExpression.length) { + const parsed = parseClickHouseIdentifier(pathExpression, idx); + if (!parsed || pathExpression.charAt(idx) !== '`') { + return undefined; + } + + segments.push(parsed.value); + idx = parsed.endIdx; + + if (idx === pathExpression.length) { + break; + } + + if (pathExpression.charAt(idx) !== '.') { + return undefined; + } + idx++; + } + + return segments.length > 0 ? segments : undefined; +}; + +export const parseRenderedJsonStringExpression = ( + keyExpression: string, +): { column: string; key: string } | undefined => { + const emptyPathPrefix = 'JSONExtractString(toJSONString('; + const emptyPathSuffix = "), '')"; + if ( + keyExpression.startsWith(emptyPathPrefix) && + keyExpression.endsWith(emptyPathSuffix) + ) { + const columnExpression = keyExpression.slice( + emptyPathPrefix.length, + -emptyPathSuffix.length, + ); + const parsedColumn = parseClickHouseIdentifier(columnExpression); + if (parsedColumn?.endIdx === columnExpression.length) { + return { column: parsedColumn.value, key: '' }; + } + return undefined; + } + + const prefix = 'toString('; + if (!keyExpression.startsWith(prefix) || !keyExpression.endsWith(')')) { + return undefined; + } + + const innerExpression = keyExpression.slice(prefix.length, -1); + const parsedColumn = parseClickHouseIdentifier(innerExpression); + if (!parsedColumn || innerExpression[parsedColumn.endIdx] !== '.') { + return undefined; + } + + const path = parseRenderedJsonPath( + innerExpression.slice(parsedColumn.endIdx + 1), + ); + if (!path) { + return undefined; + } + + return { column: parsedColumn.value, key: path.join('.') }; +}; + +export const renderJsonStringExpression = ( column: string, jsonPath: string, - options: { preserveStringTypeSuffix?: boolean } = {}, + options: { stripTypeSuffix?: boolean } = {}, ): string => { const columnIdentifier = quoteIdentifierIfNeeded(column); - const untypedJsonPath = - options.preserveStringTypeSuffix && - jsonPath.endsWith(JSON_STRING_TYPE_SUFFIX) - ? jsonPath.slice(0, -JSON_STRING_TYPE_SUFFIX.length) - : jsonPath; - - const path = untypedJsonPath - .split('.') - .filter(Boolean) - .map(quoteJsonPathSegment) - .join('.'); - - return `${columnIdentifier}.${path}${JSON_STRING_TYPE_SUFFIX}`; + const untypedJsonPath = options.stripTypeSuffix + ? stripClickHouseJsonTypeSuffix(jsonPath) + : jsonPath; + + const pathSegments = untypedJsonPath.split('.').filter(Boolean); + + // ClickHouse's dot notation cannot represent an empty JSON key (`` is a + // syntax error). JSONExtractString preserves the checkbox filter's string + // semantics for string, numeric, and boolean values. Treat malformed paths + // made only of separators the same way so they never emit a dangling dot. + if (pathSegments.length === 0) { + return `JSONExtractString(toJSONString(${columnIdentifier}), '')`; + } + + const path = pathSegments.map(quoteJsonPathSegment).join('.'); + + return `toString(${columnIdentifier}.${path})`; }; export class MetadataCache { @@ -305,7 +460,7 @@ export class Metadata { if ( convertCHDataTypeToJSType(columnMeta?.type ?? '') === JSDataType.JSON ) { - return renderJsonStringSubcolumn(column, bracketPath[1]); + return renderJsonStringExpression(column, bracketPath[1]); } return keyExpression; @@ -326,8 +481,8 @@ export class Metadata { }); if (convertCHDataTypeToJSType(columnMeta?.type ?? '') === JSDataType.JSON) { - return renderJsonStringSubcolumn(column, jsonPath, { - preserveStringTypeSuffix: true, + return renderJsonStringExpression(column, jsonPath, { + stripTypeSuffix: true, }); } @@ -1048,7 +1203,8 @@ export class Metadata { ]; const where = chSql`WHERE ${concatChSql(' AND ', ...whereConditions)}`; - const colMeta = key + const hasKey = key !== undefined; + const colMeta = hasKey ? await this.getColumn({ databaseName, tableName, @@ -1057,8 +1213,9 @@ export class Metadata { }) : undefined; const jsonValueExpression = - key && convertCHDataTypeToJSType(colMeta?.type ?? '') === JSDataType.JSON - ? renderJsonStringSubcolumn(column, key) + hasKey && + convertCHDataTypeToJSType(colMeta?.type ?? '') === JSDataType.JSON + ? renderJsonStringExpression(column, key) : undefined; let sql: ChSql; @@ -1073,7 +1230,7 @@ export class Metadata { Int32: maxValues, }} `; - } else if (key) { + } else if (hasKey) { sql = chSql` SELECT DISTINCT ${{ Identifier: column, @@ -2204,6 +2361,18 @@ export class Metadata { // Parse all keys into (rollupColumn, rollupKey) pairs const parsed = keyExpressions.map(keyExpr => { + const renderedJsonKey = parseRenderedJsonStringExpression(keyExpr); + if (renderedJsonKey) { + return { + keyExpression: keyExpr, + rollupColumn: renderedJsonKey.column, + rollupKey: renderedJsonKey.key, + column: renderedJsonKey.column, + mapKey: renderedJsonKey.key, + rawTableExpression: keyExpr, + }; + } + const path = parseKeyPath(keyExpr); const isMapKey = path.length >= 2; return { @@ -2212,6 +2381,7 @@ export class Metadata { rollupKey: isMapKey ? path[1] : unquoteIdentifier(path[0]), column: unquoteIdentifier(path[0]), mapKey: isMapKey ? path[1] : undefined, + rawTableExpression: undefined, }; }); @@ -2294,6 +2464,11 @@ export class Metadata { // SQL-escaped before being embedded as a literal — `SqlString.escape` // returns a fully-quoted, safely-escaped ClickHouse string literal. if (keyValueFetchingStrategies.rawTable.includes(key.column)) { + if (key.rawTableExpression) { + rawQueryOptions.push(key.rawTableExpression); + continue; + } + const quotedColumn = quoteIdentifierIfNeeded(key.column); if (key.mapKey) { rawQueryOptions.push(