Skip to content

feat: Add Raw SQL Chart support to external dashboard APIs#1877

Merged
kodiakhq[bot] merged 6 commits intomainfrom
drew/raw-sql-external-apis-2
Mar 11, 2026
Merged

feat: Add Raw SQL Chart support to external dashboard APIs#1877
kodiakhq[bot] merged 6 commits intomainfrom
drew/raw-sql-external-apis-2

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Mar 10, 2026

Summary

This PR adds support for Raw SQL Charts to the external dashboards APIs.

Screenshots or video

Screenshot 2026-03-10 at 1 25 16 PM

How to test locally or on Vercel

  • yarn dev locally
  • Grab your Personal API Key from Team Settings
  • Make a request to the dashboard endpoints
curl http://localhost:8000/api/v2/dashboards -H "Authorization: Bearer <Key>"  -H "Content-Type: application/json" 

Schema is available at http://localhost:8000/api/v2/docs/#/

References

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2026

🦋 Changeset detected

Latest commit: 804a484

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app 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
Copy link

vercel bot commented Mar 10, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 11, 2026 1:31pm

Request Review

@github-actions
Copy link
Contributor

Code Review

PR: feat: Add Raw SQL Chart support to external dashboard APIs

  • 🔒 Missing connectionId ownership validation -> The getMissingSources function in dashboards.ts:36 only validates sourceIds for builder configs, but there is no equivalent check to verify that the connectionId in a raw SQL chart config belongs to the requesting team. A user could reference another team's connection ID. Add a getMissingConnections check parallel to getMissingSources, called in both POST (~line 1516) and PUT (~line 1739) handlers.

  • Warning: externalDashboardOtherRawSqlChartConfigSchema groups table/number/pie -> These three display types are grouped into one schema (zod.ts:1078), but the OpenAPI spec defines them as separate schemas (TableRawSqlChartConfig, NumberRawSqlChartConfig, PieRawSqlChartConfig). The union type allows e.g. fillNulls on a number chart, which is not reflected in the API docs. Consider splitting into per-display-type schemas or noting the mismatch.

  • Warning: Removed discriminator from TileConfig oneOf -> The discriminator was removed from both the OpenAPI spec and inline JSDoc. Without it, code generation tools cannot efficiently determine which schema to use. Consider adding a composite discriminator or noting this as a known limitation.

Conversion logic to/from RawSqlSavedChartConfig looks correct, type safety is well-enforced with satisfies, and test coverage is thorough.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

  • ⚠️ Misleading error log message in convertToExternalTileChartConfig (utils/dashboards.ts) for Search/Markdown/Heatmap raw SQL cases: message says 'raw SQL charts should have display type "sql"' but displayType is never "sql" — that's configType. Should say something like 'unsupported display type for raw SQL chart'.

  • ⚠️ z.discriminatedUnionz.union: The change is architecturally necessary given dual discriminants, but Zod will now try all schemas in order on every validation, producing generic union errors instead of targeted ones when invalid input is sent. Consider documenting this trade-off or using a custom pre-check to route to the right sub-union (e.g., check configType === 'sql' first).

  • ✅ Multi-tenancy is correctly enforced: getMissingConnections validates that all connectionIds belong to the requesting team before accepting the dashboard.

  • ✅ Round-trip tests for all 5 raw SQL chart types are present and comprehensive.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

Overall the implementation is solid — correct ownership validation, good test coverage, clean separation of builder vs. raw-SQL paths. A couple of items worth addressing:

  • ⚠️ OpenAPI discriminator removed — Removing the discriminator block from TileConfig is a potentially breaking change for API clients that use code generators (e.g. openapi-generator, swagger-codegen). Since the schema now requires two fields to discriminate (configType + displayType), a single discriminator can't cover both, but clients relying on the old discriminator mapping will break. Consider calling this out explicitly in the changeset or PR description.

  • ⚠️ Double-parse pattern in zod.ts — silent 500 risk — In externalDashboardTileConfigSchema, the .superRefine() forwards issues via ctx.addIssue() (without fatal: true), then .transform() calls schema.parse(data). In Zod v3, a non-fatal issue in superRefine does not prevent the transform from running, so if superRefine marks a field invalid but doesn't abort the parse, schema.parse(data) in the transform will throw an uncaught ZodError → 500 instead of 400. Fix: add { fatal: true } to each ctx.addIssue call in the superRefine block, or guard the transform with an early return on failed validation.

  • ℹ️ sqlTemplate accepts empty strings — No minimum length is enforced on the sqlTemplate field (only maxLength: 100000). An empty template will pass validation but likely fail at query execution time. Consider z.string().min(1).max(100000) to surface the error earlier.

✅ Security check (cross-team connection ownership via getMissingConnections) is correctly implemented and tested.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

No critical issues found.

The implementation is solid:

  • Multi-tenancy enforced correctly: getMissingConnections validates connection IDs belong to the requesting team before saving, with tests for the cross-team rejection case
  • fillNulls round-trips correctly: true→stores undefined→reads back as true (mirrors existing builder pattern)
  • Type exhaustiveness is maintained via satisfies checks and externalConfig satisfies never guards
  • Zod validation properly routes to the correct sub-schema via configType discriminant and strips unknown fields in the transform step

One minor note: the getMissingConnections function fetches all team connections to validate IDs (same pattern as getMissingSources), which is fine at current scale.

🤖 Generated with Claude Code

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2026

E2E Test Results

All tests passed • 88 passed • 3 skipped • 892s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew requested review from a team and fleon and removed request for a team March 10, 2026 19:45
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

PR Review

  • ⚠️ Duplicated routing logic in zod.ts: The configType === 'sql' branching is copy-pasted into both superRefine and transform (lines ~2137-2143 and ~2165-2171). If the routing condition changes in one place but not the other, validation and stripping will silently diverge → extract into a shared helper getRawSqlSchema(data).

  • ⚠️ getMissingConnections fetches all team connections: getConnectionsByTeam(team) loads the full connection list then filters in memory. For teams with many connections this is unnecessary — a targeted Connection.find({ team, _id: { $in: [...connectionIds] } }) query would be more efficient and avoid the full collection scan on every POST/PUT.

  • ℹ️ stacked_bar raw SQL drops fillNulls in output conversion: In convertToExternalTileChartConfig, the StackedBar raw SQL branch sets fillNulls: config.fillNulls !== false, but the round-trip test for barRawSql sets fillNulls: false and expects it back — verify this is actually preserved correctly end-to-end (the false sentinel is fillNulls === false ? false : undefined, which returns undefined when fillNulls is true, potentially losing the explicit true value in existing data).

✅ Security looks solid: cross-team connectionId ownership is validated on both POST and PUT. Alert deletion on tile-to-raw-SQL conversion is handled correctly with good test coverage.

Copy link
Contributor

@fleon fleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kodiakhq kodiakhq bot merged commit e2a82c6 into main Mar 11, 2026
14 checks passed
@kodiakhq kodiakhq bot deleted the drew/raw-sql-external-apis-2 branch March 11, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants