fix: stringify JSON search sidebar filters - #2551
Conversation
🦋 Changeset detectedLatest commit: f58f69f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@mfroembgen is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes JSON-backed sidebar filters by replacing the unreliable
Confidence Score: 4/5Safe to merge for the common case; one narrow routing defect in getAllKeyValues affects Load More for empty-string JSON keys in MV-backed deployments only. The empty-key JSON expression assigns mapKey: '' (falsy), causing the MV dispatch to intercept it with rollupKey: '' and bypass the rawTableExpression fallback. In MV-backed setups, Load More for ResourceAttributes[''] returns zero results. The defect is limited to empty-string JSON attribute keys, which are rare in practice. Files Needing Attention: packages/common-utils/src/core/metadata.ts — the getAllKeyValues routing loop around lines 2405–2481, specifically the empty-key branch where mapKey: '' is falsy.
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/metadata.ts | Replaces .:String typed-subcolumn rendering with toString(col.path) expressions; adds renderJsonStringExpression, parseRenderedJsonStringExpression, stripClickHouseJsonTypeSuffix, and routes getAllKeyValues through the new parsed form. Empty-key JSON expression (JSONExtractString(...)) has a falsy mapKey that can be intercepted by the MV path before the rawTableExpression branch in MV-backed setups. |
| packages/app/src/components/DBSearchPageFilters/utils.ts | Adds jsonColumns option to toQuotedClickHouseKeyExpression so JSON-backed sidebar keys render as toString(col.path) instead of bracket-map access; imports and delegates to the new common-utils helpers; removes the local quoteIdentifierIfNeeded in favour of the shared export. |
| packages/app/src/searchFilters.tsx | Adds canonicalizeFilterQuery that rewrites stale bracket-form or typed-subcolumn JSON filters to toString(...) form once source columns load, with a round-trip guard to avoid dropping compound SQL predicates; threads jsonColumns through escapeFilterStateKeys and useSearchPageFilterState. |
| packages/app/src/DBSearchPage.tsx | Moves inputSourceTableConnection earlier to share with the new useJsonColumns call; adds canonicalizedFiltersSyncRef to prevent the prevSearched effect from resetting the form on canonicalization-only URL updates; wires jsonColumns through to useSearchPageFilterState. |
| packages/app/src/components/DBSearchPageFilters/hooks.ts | Threads jsonColumns through useFetchFacets into key-escaping and load-more paths; adds loadMoreGenerationRef to discard stale out-of-order load-more responses and clears loadMoreLoadingKeys on scope change. |
| packages/app/src/components/DBSearchPageFilters.tsx | Threads jsonColumns into sqlKey rendering; refactors FilterGroup expansion logic to distinguish isDefaultExpanded transitions from auto-expand-for-Load-More, respecting user manual toggles via hasUserToggledExpansionRef. |
| packages/app/src/tests/searchFilters.test.ts | Adds thorough unit coverage for JSON filter serialization, hydration, and canonicalization (idempotency, bracket form, typed subcolumn form, empty key, compound predicates, non-JSON passthrough). |
| packages/common-utils/src/tests/metadata.test.ts | Updates existing JSON tests from .:String to toString(...) form; adds coverage for backslash escaping, empty-key JSONExtractString form, MV and raw-table routing for rendered JSON expressions, and renderJsonStringExpression/parseRenderedJsonStringExpression round-trips. |
| packages/app/src/components/tests/DBSearchPageFilters.test.tsx | Adds UI-level tests for empty-facet Load More visibility, user-toggle persistence, default-expanded transitions, and stale-result rejection during concurrent load-more requests. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Sidebar filter key] --> B{Base column in jsonColumns?}
B -- Yes --> C[renderJsonStringExpression
toString col.path]
B -- No --> D[bracket form col key]
C --> E[Persisted in URL/DB]
D --> E
E --> F{canonicalizeFilterQuery on load}
F -- Already toString form --> G[idempotent: no change]
F -- Stale bracket or .:String form --> H[parse → clean → re-escape]
H --> I[toString expression written back]
I --> J[getAllKeyValues]
J --> K{parseRenderedJsonStringExpression}
K -- Non-empty key --> L[correct MV or raw-table routing]
K -- Empty key mapKey=empty falsy --> M[⚠ MV intercepts rollupKey=empty
raw table JSONExtractString bypassed]
K -- Not a JSON expression --> N[parseKeyPath bracket or native column]
Reviews (35): Last reviewed commit: "fix: stringify JSON search sidebar filte..." | Re-trigger Greptile
5116fd3 to
73196cd
Compare
fe66a5f to
424cc9a
Compare
424cc9a to
16061b0
Compare
🔴 P0/P1 -- must fix
🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (8): correctness, security, adversarial, julik-frontend-races, kieran-typescript, testing, maintainability, api-contract. Testing gaps:
|
d2d943e to
7f46a87
Compare
7f46a87 to
0d7e71d
Compare
Deep ReviewScope: PR #2551 — stringify JSON search sidebar filters (base Intent: Render JSON-backed source column filter keys as ClickHouse string expressions ( ✅ No critical issues found. The escape/decode pair ( 🟡 P2 — recommended
🔵 P3 nitpicks (4)
Reviewers (12): correctness, security, adversarial, testing, maintainability, project-standards, api-contract, kieran-typescript, julik-frontend-races, performance, agent-native, learnings-researcher. Testing gaps:
|
545e99c to
64547ea
Compare
64547ea to
8b35ce0
Compare
ebba10e to
b9ea6bd
Compare
b9ea6bd to
f58f69f
Compare
| @@ -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, | |||
There was a problem hiding this comment.
Empty-key JSON expression bypasses
rawTableExpression in MV-backed setups
When keyExpressions contains JSONExtractString(toJSONString(ResourceAttributes), ''), parseRenderedJsonStringExpression returns { column: 'ResourceAttributes', key: '' }, so mapKey is assigned '' (an empty string, which is falsy). The routing loop then:
- Skips the map-text-index branch (
if (key.mapKey)→ false) - Falls into the native-text-index
elsebranch (no match, nocontinue) - Reaches the MV check — if
ResourceAttributesis inmetadataMVs, it pushesrollupKey: ''into the MV query options andcontinues, never reaching therawTableExpressionbranch below
The MV has no entry for an empty rollup key, so the query returns zero results. In a MV-backed deployment, "Load more" for a sidebar facet canonicalized from ResourceAttributes[''] silently returns nothing instead of executing the correct JSONExtractString(toJSONString(ResourceAttributes), '') scan.
The fix is to guard on key.rawTableExpression before the MV check for the empty-key case, or to use mapKey: renderedJsonKey.key || undefined so the empty-key path falls straight through to the raw table branch.
Summary
toString(ResourceAttributes.\k8s`.`namespace`.`name`), instead of JSON map access or a fixed.:String` suffix.Load more.Why
toString(...)Testing against ClickHouse
26.5.1.882withJSON(max_dynamic_types=8, max_dynamic_paths=64)columns showed that.:Stringis only safe for JSON paths whose active dynamic type is actually String. Integer, boolean, and mixed-type paths can silently return no values with.:String, whiletoString(<JSON path>)returned usable values for all of these real path shapes:ResourceAttributes.k8s.namespace.name(string)ResourceAttributes.cloud.account.id(integer)LogAttributes.endOfBatch(boolean)LogAttributes.level(mixed integer/string)The same
toString(...)expression also worked for distribution/grouping queries.Manual verification
26.5.1.882, whereResourceAttributesandLogAttributesareJSON(max_dynamic_types=8, max_dynamic_paths=64)columns.toString(...).toString(ResourceAttributes.\cloud`.`account`.`id`)andtoString(LogAttributes.`endOfBatch`)`.ResourceAttributes['k8s.namespace.name']filters were canonicalized, results loaded without the ClickHousearrayElementJSON error, Show Distribution loaded, and selected/pinned facets refreshed with matching values plusLoad more.Automated verification
mise exec node@22.16.0 -- node .yarn/releases/yarn-4.13.0.cjs lint:fixmise exec node@22.16.0 -- node ../../.yarn/releases/yarn-4.13.0.cjs ci:lintinpackages/common-utilsmise exec node@22.16.0 -- node ../../.yarn/releases/yarn-4.13.0.cjs ci:unitinpackages/common-utilsmise exec node@22.16.0 -- node .yarn/releases/yarn-4.13.0.cjs workspace @hyperdx/app jest packages/app/src/components/__tests__/DBSearchPageFilters.test.tsx packages/app/src/components/DBSearchPageFilters/utils.test.ts packages/app/src/__tests__/searchFilters.test.ts --runInBand --coverage=falsemise exec node@22.16.0 -- bash -lc 'corepack enable >/dev/null 2>&1 || true; make dev-e2e FILE=filter-key-edge-cases'(18 passed)References
Fixes #2549.
Related to #2482 and #2537.