feat(analytics): named run params — inject {tenant}, bind {period} (#1966) - #2078
Conversation
…1966) /v1/queries/{id}/run now binds ClickHouse server-side parameters: {tenant} is always injected from the session context (never client-settable), and an optional `period` on the run body binds {period}. Values pass through the param interface, so a value can never alter query structure; the single-SELECT gate already tolerates {name:Type} placeholders. A query that references an unbound named parameter is surfaced as a 400, not a bare 500. PRD/DESIGN: cpt-presentation-fr-query-params marked shipped; run endpoint contract + OpenAPI regenerated. Coverage: offline classifier + deserializer unit tests, live e2e for tenant injection, period binding, and missing-param. Closes #1966 Part of #1803 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe saved-query run endpoint now accepts an optional ChangesSaved-query named parameters
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsAPI
participant SecurityContext
participant ClickHouse
Client->>AnalyticsAPI: POST /v1/queries/{id}/run with optional period
AnalyticsAPI->>SecurityContext: Read tenant
AnalyticsAPI->>ClickHouse: Execute query with tenant and period parameters
ClickHouse-->>AnalyticsAPI: Rows or missing-parameter error
AnalyticsAPI-->>Client: JSON rows or HTTP 400/500 response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/domain/presentation-layer/specs/DESIGN.md`:
- Around line 361-363: Update the POST /v1/queries/{id}/run documentation to
state that tenant-row filtering is deferred to `#1967` rather than already
injected, while preserving the existing description of tenant context binding
and the explicit no-predicate-yet wording elsewhere.
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 211-218: Update the UNKNOWN_QUERY_PARAMETER handling in the
SavedQueryError mapping to extract the actually unbound parameter name and
report that name in the field violation. Use period only when the missing
parameter is period; otherwise return the generic/SQL contract violation or
reject unsupported placeholders during saving, while preserving the existing
handling for Code: 456.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27adc37b-e2fe-4922-960b-7da6c3bcd687
📒 Files selected for processing (7)
docs/components/backend/analytics/openapi.jsondocs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/api/saved_queries.rssrc/backend/services/analytics/src/domain/saved_query.rssrc/ingestion/tests/e2e/api/test_queries.py
…r defer (#1966) CodeRabbit review on #2078: - classify_run_error named every UNKNOWN_QUERY_PARAMETER as missing `period`; a query using `{region}` was mislabeled. Extract the backtick-quoted name from the ClickHouse message and report that, falling back to a generic `params` violation when the server does not name it. - DESIGN §3.3 run row read as if the tenant-row filter were already injected; say the filter is deferred to #1967 (the value is bound, no predicate yet), matching the boundary note. Part of #1803 Refs #1966 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
2 participants
What
Adds named query parameters to the saved-query run path (Phase A, presentation layer).
POST /v1/queries/{id}/runnow binds ClickHouse server-side parameters:{tenant}— always injected from the sessionSecurityContext; never client-settable.{period}— optional, supplied on the run body{ "period": "<value>" }.Values are passed through ClickHouse's
param_<name>interface (Query::param), so a parameter value can never alter query structure — no string interpolation. The single-SELECT gate already tolerates{name:Type}placeholders, so authored SQL likeSELECT ... WHERE insight_tenant_id = {tenant:UUID} AND ts >= {period:Date}passes on write and run. A query that references an unbound named parameter (e.g.{period}with noperiodsupplied) is surfaced as a 400, not a bare 500.The run request body is optional (
Option<Json<..>>), so existing param-less runs are unchanged.Scope boundary
This binds the
{tenant}value. The injected tenant-row filter (insight_tenant_id = {tenant}added to the compiler's sharedWHERE) remains #1967.Specs
Registered
cfsartifacts updated:cpt-presentation-fr-query-paramsmarked shipped; run endpoint contract in DESIGN §3.3 and the saved-query-api component updated.cfs validategreen on both PRD and DESIGN. OpenAPI contract regenerated (newRunSavedQueryRequestschema + optional request body).Tests
classify_run_error(456 → 400, others → 500) andRunSavedQueryRequestdeserializer (absent/{}/value).test_queries.py):{tenant}injection returns the signed session tenant,{period}binding echoes the supplied value, missing-param → 400.cargo test -p analytics(487 passed), clippy + fmt clean, OpenAPI drift gate passes.Closes #1966
Part of #1803
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
periodvalue when run.Documentation