Skip to content

fix: keep parsed-JSON "Add to Filters" keys as valid SQL#2561

Merged
kodiakhq[bot] merged 5 commits into
mainfrom
alex/fix-json-add-to-filter-sql
Jul 1, 2026
Merged

fix: keep parsed-JSON "Add to Filters" keys as valid SQL#2561
kodiakhq[bot] merged 5 commits into
mainfrom
alex/fix-json-add-to-filter-sql

Conversation

@alex-fedotyev

Copy link
Copy Markdown
Contributor

"Add to Filters" on a value inside parsed JSON from a String column (for example Body) builds a query ClickHouse rejects. Expand a log whose Body is a JSON string, then filter on a nested value such as app.user.currency = USD, and the generated condition is:

JSONExtractString(Body, 'app['user.currency')'] IN ('USD')

which fails to parse. This fixes the filter-key serialization so the generated SQL is valid.

Resolves HDX-4427.

Summary

The JSON viewer builds a correct ClickHouse expression for the filter key, JSONExtractString(Body, 'app.user.currency'). That key then flows through escapeFilterStateKeys into toClickHouseKeyExpression, which only short-circuited bracket access, backtick paths, and toString(...). A JSONExtract*(...) expression fell through to parseMapFieldName, whose dot pattern split on the dot inside the quoted JSON path argument, and mergePath then rewrote it into the invalid JSONExtractString(Body, 'app['user.currency')'].

The pass-through guard now treats any raw SQL function-call expression (toString(...), JSONExtractString/Float/Bool(...)) as an already-valid ClickHouse expression and returns it unchanged, so it reaches filtersToQuery intact and serializes to JSONExtractString(Body, 'app.user.currency') IN ('USD'). Dot-form Map sub-keys like LogAttributes.host.name contain no parentheses, so they still rewrite to bracket form as before.

Changes

  • packages/app/src/components/DBSearchPageFilters/utils.ts: generalize the toClickHouseKeyExpression pass-through guard from toString( to any function-call expression.
  • Unit tests for the guard; an integration test that runs the real escapeFilterStateKeys -> filtersToQuery -> parseQuery pipeline and asserts every emitted condition is valid, round-trippable SQL across a range of JSON and Map key shapes; and a component test locking the "Add to Filters" contract for a parsed-JSON String column.
  • An end-to-end test that adds a filter on a nested JSON Body value from the row side panel and confirms the search runs without a ClickHouse error and returns the matching row.

Test plan

  • nx run-many -t ci:lint (eslint + tsc)
  • nx run-many -t ci:unit
  • Confirmed the unit and integration tests fail without the fix and pass with it
  • Drove the real search UI against ClickHouse: "Add to Filters" on a nested JSON Body value builds JSONExtractString(Body, 'app.user.currency') IN ('USD'), the query runs, and the row is returned
  • e2e: packages/app/tests/e2e/features/search/json-add-to-filter.spec.ts

[ui-check: allow] No visual surface; the change is filter-key SQL serialization, verified via unit, integration, and end-to-end tests.
[viewport: allow] Layout-neutral logic fix.
[ui-states: allow] No empty/loading/error UI states in scope.

alex-fedotyev and others added 2 commits June 30, 2026 23:00
"Add to Filters" on a value inside parsed JSON from a String column
(for example Body) builds a JSONExtractString(...) expression as the
filter key. toClickHouseKeyExpression only short-circuited bracket
access, backtick paths, and toString(...), so the JSONExtract
expression fell through to parseMapFieldName, whose dot pattern split
on the dot inside the quoted JSON path argument. mergePath then
rewrote it into the invalid JSONExtractString(Body, 'app['user.currency')'],
which ClickHouse rejects.

Generalize the pass-through guard to any raw SQL function-call
expression so JSONExtractString/Float/Bool (and toString) keys reach
filtersToQuery unchanged and serialize to valid SQL.

Add unit, pipeline, and component coverage asserting the JSON
add-to-filter path emits valid, round-trippable SQL.

HDX-4427

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Seed a log whose Body is a JSON string with a flat dotted key into the
default logs table, add a SidePanelComponent helper that adds a nested
JSON value to filters from the parsed tab, and assert the resulting
search runs against ClickHouse without error and returns the matching
row.

Verified by driving the real app: the action builds
JSONExtractString(Body, 'app.user.currency') IN ('USD') and the query
succeeds instead of the previously mangled, rejected expression.

HDX-4427

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c055770

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

@vercel

vercel Bot commented Jul 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 1, 2026 4:19pm
hyperdx-storybook Ready Ready Preview, Comment Jul 1, 2026 4:19pm

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 19 (+ 368 in test files, excluded from tier calculation)
  • Branch: alex/fix-json-add-to-filter-sql
  • Author: alex-fedotyev

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a SQL-generation bug where "Add to Filters" on a value inside a parsed-JSON String column (e.g. Body) produced an invalid ClickHouse expression. The JSONExtractString(Body, 'app.user.currency') key produced by the JSON viewer fell through toClickHouseKeyExpression to parseMapFieldName, which split on the dot inside the quoted JSON path argument, and mergePath rewrote it into the malformed JSONExtractString(Body, 'app['user.currency')'].

  • Core fix (utils.ts): adds isSqlFunctionCallExpression — a one-line regex guard /^[A-Za-z_]\\w*\\(/ — that short-circuits any function-call expression before parseMapFieldName is reached, generalizing the previous toString(-specific pass-through.
  • Tests: unit tests for the pass-through guard, integration pipeline test driving the full escapeFilterStateKeysfiltersToQueryparseQuery round-trip for seven key shapes, component test pinning the "Add to Filters" contract, and E2E spec seeding a JSON-body row and confirming valid SQL execution.
  • Test infrastructure: data-testid="json-viewer-line" added to HyperJson.tsx (resolving the previously flagged XPath fragility); E2E page object updated to use it.

Confidence Score: 5/5

Safe to merge — the change is a one-line regex guard in a single utility function with no broader side effects on existing key shapes.

The fix is minimal: one new predicate and one replaced condition in toClickHouseKeyExpression. All pre-existing Map/bracket/backtick/plain-column paths are untouched. The regex correctly identifies function-call expressions without false-positives. Unit, integration, component, and E2E tests all confirm correct behavior.

No files require special attention.

Important Files Changed

Filename Overview
packages/app/src/components/DBSearchPageFilters/utils.ts Core fix: adds isSqlFunctionCallExpression regex guard to toClickHouseKeyExpression; minimal, correctly scoped, generalizes the prior toString(-specific pass-through.
packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts New integration pipeline test for the full escapeFilterStateKeys -> filtersToQuery -> parseQuery path across seven key shapes, with round-trip and single-quote assertions.
packages/app/src/components/HyperJson.tsx Adds data-testid="json-viewer-line" to the Line element, replacing the fragile XPath class-substring selector.
packages/app/tests/e2e/features/search/json-add-to-filter.spec.ts E2E spec that seeds a JSON-body row, adds the nested value to filters from the side panel, and asserts valid SQL execution against ClickHouse.
packages/app/tests/e2e/seed-clickhouse.ts Adds JSON_BODY_LOG seed constant and generateJsonBodyLogData using only hardcoded values — no SQL injection surface.

Reviews (4): Last reviewed commit: "Merge branch 'main' into alex/fix-json-a..." | Re-trigger Greptile

Comment thread packages/app/tests/e2e/components/SidePanelComponent.ts Outdated
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 224 passed • 3 skipped • 1345s

Status Count
✅ Passed 224
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

…t id

The e2e failed in CI because ServiceName:"json-body-filter-svc" (hyphens)
tokenizes in Lucene and did not isolate to the seeded row (48 rows matched).
Use an underscore ServiceName so the quoted Lucene term matches exactly,
matching the pattern in total-count-matches-table.

Address the review note on the fragile CSS-module class selector: add
data-testid="json-viewer-line" to the JSON viewer line and locate lines by
that test id instead of an xpath class-substring match.

Verified end to end in a real browser: the search isolates to one row and
"Add to Filters" builds JSONExtractString(Body, 'app.user.currency') IN ('USD').

HDX-4427

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Selecting the source resets the search bar, so the ServiceName query never
applied and the results stayed unfiltered (48 rows in CI, failing the
toHaveCount(1) setup). The default source is already the logs source, the
same assumption total-count-matches relies on, so goto -> selectRelativeTime
-> performSearch isolates correctly. Verified end to end in a real browser
against ClickHouse: search isolates, "Add to Filters" builds
JSONExtractString(Body, 'app.user.currency') IN ('USD'), no error.

HDX-4427

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Focused SQL-serialization bug fix: toClickHouseKeyExpression now passes through any identifier( function-call key (generalizing the prior toString(-only guard) so a JSONExtractString(...) filter key from parsed-JSON "Add to Filters" is no longer mangled into invalid SQL. The core logic change is ~10 lines; the rest is unit, integration, component, and e2e coverage. The primary fix is sound and well-tested — reviewers confirmed the regex is anchored, linear (no ReDoS), a strict superset of the old toString( case, and does not regress any realistic dot-form Map key. The one substantive concern is a security regression that the widened pass-through now exposes.

🔴 P0/P1 — must fix

  • packages/app/src/components/DBRowJsonViewer.tsx:66buildJSONExtractQuery interpolates each JSON path segment as '${p}' with no quote escaping, and the widened guard at utils.ts:148 now routes the resulting key verbatim into the WHERE clause; a JSON key containing both a dot and a single quote (e.g. log Body {"a.b') OR 1=1 --":"USD"}) previously fell into parseMapFieldName/mergePath and was mangled into a ClickHouse-rejected query, but now serializes to the valid, executable JSONExtractString(Body, 'a.b') OR 1=1 --') IN ('USD'), turning a filter click into an injected SQL predicate.
    • Fix: Escape single quotes and backslashes in each nested path segment inside buildJSONExtractQuery before interpolating, so the emitted expression is well-formed regardless of JSON key content.
    • ce-security-reviewer, ce-adversarial-reviewer

🟡 P2 — recommended

  • packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts:1 — the new pipeline and unit tests exercise dotted JSON keys and single quotes in filter values but never a JSON key containing a single quote, parenthesis, or backslash, so the behavior the guard now newly trusts as "already valid SQL" is unverified for adversarial key content.
    • Fix: Add a case feeding a quote-bearing JSON key through buildJSONExtractQueryescapeFilterStateKeysfiltersToQuery and assert the emitted condition is valid SQL and round-trips through parseQuery.
    • ce-correctness-reviewer, ce-testing-reviewer, ce-security-reviewer, ce-adversarial-reviewer, ce-kieran-typescript-reviewer
🔵 P3 nitpicks (2)
  • packages/app/tests/e2e/components/SidePanelComponent.ts:64addParsedJsonFieldToFilter uses an isVisible().catch(() => false) expand-probe plus exact-text locators that couple the helper to the current single-row seed layout, making it the most flake-prone method in the page object.

    • Fix: Consider a more explicit expanded/collapsed state check rather than a visibility-probe fallback, and keep the exact-text locator in mind if JSON rendering changes.
  • packages/app/src/components/DBSearchPageFilters/jsonAddToFilter.pipeline.test.ts:1 — the new unit test is a directory sibling rather than living under a __tests__/ directory as the root AGENTS.md testing rule states, though it matches the pre-existing sibling pattern already in that folder (utils.test.ts, DBRowJsonViewer.test.tsx).

    • Fix: Optionally relocate to a __tests__/ directory, or leave as-is to match the established local convention.

Reviewers (7): ce-correctness-reviewer, ce-testing-reviewer, ce-maintainability-reviewer, ce-project-standards-reviewer, ce-kieran-typescript-reviewer, ce-security-reviewer, ce-adversarial-reviewer.

Testing gaps: No coverage for adversarial JSON keys (embedded quote/paren/backslash) through the widened pass-through guard, nor a producer-level test asserting buildJSONExtractQuery escapes quotes in path segments.

@kodiakhq kodiakhq Bot merged commit 555d88a into main Jul 1, 2026
20 checks passed
@kodiakhq kodiakhq Bot deleted the alex/fix-json-add-to-filter-sql branch July 1, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants