feat(analytics): saved-query CRUD + run (presentation.queries) (#1965) - #2051
Conversation
📝 WalkthroughWalkthroughAdds tenant-scoped saved-query persistence and CRUD/run endpoints, validates single-SELECT SQL, executes saved queries through ClickHouse, and updates OpenAPI, tests, and presentation-layer documentation. ChangesSaved-query API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsAPI
participant MariaDB
participant ClickHouse
Client->>AnalyticsAPI: POST /v1/queries/{id}/run
AnalyticsAPI->>MariaDB: Load tenant-scoped saved query
MariaDB-->>AnalyticsAPI: Saved query SQL
AnalyticsAPI->>AnalyticsAPI: Validate single SELECT
AnalyticsAPI->>ClickHouse: Execute SQL as JSONEachRow
ClickHouse-->>AnalyticsAPI: JSON result rows
AnalyticsAPI-->>Client: RunResponse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🧹 Nitpick comments (2)
src/backend/services/analytics/src/infra/db/entities.rs (1)
26-45: 🚀 Performance & Scalability | 🔵 TrivialVerify
insight_tenant_idis indexed in the migration.Every saved-query read path (
list_saved_queries,find_saved_query) filters byinsight_tenant_id; without an index on that column (or a composite(insight_tenant_id, id)index) list/get/update/delete/run will full-scansaved_queriesas it grows. The migration file isn't in this batch, so please confirm it adds one.🤖 Prompt for 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. In `@src/backend/services/analytics/src/infra/db/entities.rs` around lines 26 - 45, Verify the migration creating the saved_queries table adds an index on insight_tenant_id, optionally using a composite (insight_tenant_id, id) index. If absent, add the index so the list_saved_queries and find_saved_query query paths avoid full scans.src/backend/services/analytics/src/api/saved_queries.rs (1)
140-156: 🔒 Security & Privacy | 🔵 TrivialConfirm
presentation_rogrants don't allow cross-tenant reads until tenant-row filtering (#1967) ships.Since
/runexecutes the stored single-SELECT as authored with no injected tenant predicate, any query a tenant can author is only as safe as the underlying ClickHouse grants forpresentation_ro. This is explicitly called out as deferred scope, so just flagging for confirmation: ispresentation_roalready restricted (e.g., row policies/per-tenant views) such that a broadSELECT * FROM <shared table>can't surface another tenant's rows in the interim?🤖 Prompt for 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. In `@src/backend/services/analytics/src/api/saved_queries.rs` around lines 140 - 156, Confirm the security guarantees of the ClickHouse `presentation_ro` grants used by `execute_read` in `run_saved_query`: verify that row policies or per-tenant views prevent cross-tenant results for broad queries such as `SELECT *` before tenant-row filtering (`#1967`) ships. If the grants do not provide this isolation, update the interim execution path or explicitly block `/run` until equivalent tenant isolation is enforced.
🤖 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 176-182: Update the architecture diagram for presentation.queries
to show MariaDB in the analytics service DB as its metadata store, matching the
saved_queries relationship. Remove its placement in the ClickHouse presentation
namespace, and reserve ClickHouse presentation usage for the /run execution
path.
- Around line 274-276: Require tenant-row isolation before enabling saved-query
execution: update docs/domain/presentation-layer/specs/DESIGN.md lines 274-276
to require server-side predicate injection or keep /run unavailable until `#1967`;
update docs/domain/presentation-layer/specs/PRD.md lines 188-190 to withhold
shipped status until isolation is enforced; update
docs/domain/presentation-layer/specs/PRD.md lines 308-314 to describe the API as
CRUD-only or unavailable for execution until the run path is tenant-filtered.
---
Nitpick comments:
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 140-156: Confirm the security guarantees of the ClickHouse
`presentation_ro` grants used by `execute_read` in `run_saved_query`: verify
that row policies or per-tenant views prevent cross-tenant results for broad
queries such as `SELECT *` before tenant-row filtering (`#1967`) ships. If the
grants do not provide this isolation, update the interim execution path or
explicitly block `/run` until equivalent tenant isolation is enforced.
In `@src/backend/services/analytics/src/infra/db/entities.rs`:
- Around line 26-45: Verify the migration creating the saved_queries table adds
an index on insight_tenant_id, optionally using a composite (insight_tenant_id,
id) index. If absent, add the index so the list_saved_queries and
find_saved_query query paths avoid full scans.
🪄 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: ba8f5efe-2de0-46b9-b492-f0745abd9027
📥 Commits
Reviewing files that changed from the base of the PR and between 405154e and c9ed9950a569b5452ddb2bb7b5b06cabc4ae963e.
📒 Files selected for processing (13)
docs/components/backend/analytics/openapi.jsondocs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/api/error.rssrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/api/openapi_tests.rssrc/backend/services/analytics/src/api/saved_queries.rssrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/domain/saved_query.rssrc/backend/services/analytics/src/infra/db/entities.rssrc/backend/services/analytics/src/migration/m20260730_000001_saved_queries.rssrc/backend/services/analytics/src/migration/mod.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/domain/presentation-layer/specs/DESIGN.md (1)
53-53: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winComplete the storage-topology update.
Line 53 correctly places
saved_queriesmetadata in MariaDB, but the surrounding architecture still presents ClickHouse as the only presentation storage and labels the ClickHouse area “query results.” The current/runimplementation returnsJSONEachRowdata and does not persist saved-query results. Update the diagram and infrastructure inventory so MariaDB is listed for metadata and ClickHouse is reserved for actual query execution/storage.Also applies to: 89-97
🤖 Prompt for 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. In `@docs/domain/presentation-layer/specs/DESIGN.md` at line 53, The presentation-layer architecture documentation still treats ClickHouse as the sole presentation store and incorrectly implies persisted query results. Update the diagram and infrastructure inventory sections around the storage topology to list MariaDB for saved-query metadata, relabel ClickHouse around query execution/storage rather than “query results,” and reflect that `/run` returns JSONEachRow without persisting saved-query results; preserve the existing saved-query CRUD and read-path descriptions.
🤖 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.
Outside diff comments:
In `@docs/domain/presentation-layer/specs/DESIGN.md`:
- Line 53: The presentation-layer architecture documentation still treats
ClickHouse as the sole presentation store and incorrectly implies persisted
query results. Update the diagram and infrastructure inventory sections around
the storage topology to list MariaDB for saved-query metadata, relabel
ClickHouse around query execution/storage rather than “query results,” and
reflect that `/run` returns JSONEachRow without persisting saved-query results;
preserve the existing saved-query CRUD and read-path descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f416f49-304a-4bdf-a496-1e87b0391a09
📥 Commits
Reviewing files that changed from the base of the PR and between c9ed9950a569b5452ddb2bb7b5b06cabc4ae963e and 4dc152869df8242a5ad8f09f4d259090c745a68a.
📒 Files selected for processing (1)
docs/domain/presentation-layer/specs/DESIGN.md
|
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 `@src/ingestion/tests/e2e/api/test_queries.py`:
- Around line 30-31: Assert the direct saved-query cleanup in
create_scratch_saved_query’s test path returns 204 instead of discarding the
DELETE response. In src/ingestion/tests/e2e/api/conftest.py lines 52-54, assert
fixture teardown accepts only 204 or the intentional post-delete 404, failing
for all other statuses.
- Around line 55-67: Add a cross-tenant isolation test alongside
test_list_saved_queries_200 and test_list_saved_queries_200_excludes_deleted:
create the scratch saved query under one tenant, switch to a second tenant,
assert it is absent from GET /v1/queries, and verify GET, PUT, DELETE, and POST
operations targeting that query return not found. Reuse the existing
tenant-switching and saved-query fixtures/helpers.
🪄 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: fdfb349d-9efe-4e78-a6c6-5663c9459e70
📥 Commits
Reviewing files that changed from the base of the PR and between 4dc152869df8242a5ad8f09f4d259090c745a68a and 6a812abc0ae4bccbe04e6996589ab55ab8e38828.
📒 Files selected for processing (7)
src/backend/services/analytics/src/api/saved_queries.rssrc/backend/services/analytics/src/domain/saved_query.rssrc/ingestion/tests/e2e/api/__init__.pysrc/ingestion/tests/e2e/api/conftest.pysrc/ingestion/tests/e2e/api/endpoint_helpers.pysrc/ingestion/tests/e2e/api/test_queries.pysrc/ingestion/tests/e2e/lib/api_coverage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/services/analytics/src/api/saved_queries.rs
Add the presentation-layer "Data Analytics" surface: CRUD and read-only
run over saved queries, tenant-scoped.
- `saved_queries` service-DB (MariaDB) SeaORM entity + migration, mirroring
the metric CRUD entities. CRUD is metadata management — it never touches
ClickHouse; only `/run` does.
- `GET/POST/PUT/DELETE /v1/queries` + `POST /v1/queries/{id}/run`. The
single-SELECT gate validates SQL on create, update, and run; `/run`
executes the stored SELECT read-only as `presentation_ro` and returns
untyped JSONEachRow rows (same shape as the metric query path).
- Named parameters (#1966) and the injected tenant-row filter (#1967) are
separate sub-issues; #1965 runs the stored SQL as authored.
Specs: reconcile the PRD/DESIGN storage story to service-DB (MariaDB), not
the ClickHouse `presentation` namespace, and mark the saved-query
component/interface/table implemented. Regenerate the analytics OpenAPI doc.
Closes #1965
Part of #1803
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Drive the real /v1/queries* route table via tower::oneshot against the live MariaDB (INTEGRATION_TESTS_MARIADB_URL), same harness as the metric and admin-threshold HTTP tests. Covers every non-5xx response: - CRUD round-trip: create 201, list 200, get 200, update 200, delete 204, get-after-delete 404. - Gate rejections: create/update with non-read or multi-statement SQL 400. - Not-found: get/update/delete/run of an unknown id 404 (run resolves the row before any ClickHouse call, so its 404 needs no live CH). - Tenant scoping: tenant B cannot see tenant A's saved query (404). The /run 200 path needs a live ClickHouse (dead_ch maps it to 5xx), so it is exercised by the compose e2e, not here. Part of #1965 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
… not ClickHouse The §1.3 architecture diagram still placed presentation.queries in the ClickHouse presentation-write box after §3.1/§3.7 were reconciled to the service DB (MariaDB). Move it out of the ClickHouse box and note the metadata store explicitly (CodeRabbit review, PR #2051). Part of #1965 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…1965) The new-code coverage gate failed (77.9% < 80%): execute_read's JSONEachRow parsing and the description triple-state deserializer had no coverage, since a successful /run needs a live ClickHouse (harness uses dead_ch) and no test sent a description through PUT. - Extract the JSONEachRow parsing into a pure parse_json_each_row and unit-test it (empty / multi-row / blank-line skip / malformed). This also de-duplicates the fetch-parse block flagged in review. - Unit-test deserialize_optional_nullable for absent/null/value. Both are DB- and ClickHouse-free, so they run in the coverage job. Part of #1965 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The API endpoint coverage gate failed: the 6 saved-query operations were in the committed OpenAPI spec but exercised by no black-box e2e test. Add api/test_queries.py mirroring test_metrics.py — one case per (path, method, status) across all non-5xx responses: - POST 201 · 400 bad-sql · 415 · (400 off-schema xfail #1670) - GET list 200 · 200-excludes-deleted - GET {id} 200 · 400 non-uuid · 404 unknown · 404 deleted - PUT {id} 200 · 400 bad-sql · 400 non-uuid · 404 · 415 · (400 xfail) - DELETE {id} 204 · 400 non-uuid · 404 - POST {id}/run 200 (rows == [{one:1}], executed as presentation_ro) · 400 non-uuid · 404 Add the scratch-saved-query helper + fixture, and BLOCKED entries for the 403/409 (and list 400/404) .standard_errors boilerplate these routes never answer (403 is n/a — no role gate; cross-tenant is 404 by opacity), mirroring the metric analogues so per-code coverage stays 100%. Part of #1965 Signed-off-by: Anton Zelenov <antonz@constructor.tech>
6a812ab to
b47258b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/backend/services/analytics/src/api/error.rs (1)
204-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
context.resource_nametoo.The test pins
.with_resource("q-123")but never checks it, so the id-pinning thatinvalid_sql_forrelies on is uncovered — unlikethreshold_update_invalid_operator_only.♻️ Proposed addition
assert_eq!( p["context"]["resource_type"], "gts.cf.insight.analytics_api.saved_query.v1~" ); + assert_eq!(p["context"]["resource_name"], "q-123");🤖 Prompt for 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. In `@src/backend/services/analytics/src/api/error.rs` around lines 204 - 213, Extend the test around the SavedQueryError construction to assert that p["context"]["resource_name"] equals "q-123", matching the value supplied to with_resource. Keep the existing status and resource_type assertions unchanged.src/backend/services/analytics/src/api/http_live_tests.rs (1)
476-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the update actually applied, not just the status.
The round trip only checks
200; a handler that silently droppedname/sqlwould still pass.💚 Proposed addition
assert_eq!(resp.status(), StatusCode::OK, "update should 200"); + let updated = body_json(resp).await?; + assert_eq!(updated["name"], "renamed"); + assert_eq!(updated["sql"], "SELECT 2");🤖 Prompt for 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. In `@src/backend/services/analytics/src/api/http_live_tests.rs` around lines 476 - 482, Extend the update test after the successful PUT request to verify that the query’s name and SQL were changed to “renamed” and “SELECT 2”, not only that the response status is OK. Reuse the existing query-fetching or response-parsing helper in this test module to assert both updated fields.src/backend/services/analytics/src/api/saved_queries.rs (1)
47-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReturn the declared
SavedQueryListResponseinstead of an ad-hocjson!.The route registers
saved_query::SavedQueryListResponseas the 200 schema (api/mod.rsLine 242), but the handler serializes an untyped map, so DTO changes can drift from the published contract silently.♻️ Proposed change
- let items: Vec<SavedQuerySummary> = rows.into_iter().map(model_to_summary).collect(); - Ok(Json(serde_json::json!({ "items": items }))) + let items: Vec<SavedQuerySummary> = rows.into_iter().map(model_to_summary).collect(); + Ok(Json(SavedQueryListResponse { items }))Add
SavedQueryListResponseto thecrate::domain::saved_queryimport list (Lines 27-29).🤖 Prompt for 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. In `@src/backend/services/analytics/src/api/saved_queries.rs` around lines 47 - 48, Update the saved-query handler to construct and return the declared SavedQueryListResponse instead of an ad-hoc serde_json::json! map, adding SavedQueryListResponse to the crate::domain::saved_query imports and preserving the collected items in its expected field.
🤖 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 `@src/backend/services/analytics/src/api/http_live_tests.rs`:
- Around line 617-630: Replace the unwrap_or_default() extraction of the created
query ID in the tenant-isolation test with the explicit-panic form used around
the existing ID assertions near lines 452-455, so a missing or non-string ID
fails immediately instead of issuing a request with an empty path segment. Apply
the same correction to the matching ID extraction around line 530.
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 145-155: Prevent the `/run` path in the saved-query execution flow
from running SQL without tenant scoping: either implement the `#1967` tenant-row
predicate injection before `execute_read`, or gate/restrict this endpoint until
that protection exists. Keep `find_saved_query` and `validate_single_select`
checks intact, and ensure every query reaching `execute_read` is constrained to
`ctx.subject_tenant_id()`.
- Around line 65-76: Validate the saved-query name immediately after
validate_single_select and before constructing saved_queries::ActiveModel,
rejecting missing or out-of-contract lengths with the existing 400
field_violation response using field: "name". Reuse the established
validation/error helper and preserve the migration-backed created_at and
updated_at defaults.
- Around line 161-185: Update execute_read to apply ClickHouse query settings
before fetch_bytes/collect, including a bounded max_result_rows value and
result_overflow_mode set to throw. Reuse the client’s query options/settings API
so oversized saved-query responses are rejected server-side before
cursor.collect accumulates them, while preserving existing error handling.
---
Nitpick comments:
In `@src/backend/services/analytics/src/api/error.rs`:
- Around line 204-213: Extend the test around the SavedQueryError construction
to assert that p["context"]["resource_name"] equals "q-123", matching the value
supplied to with_resource. Keep the existing status and resource_type assertions
unchanged.
In `@src/backend/services/analytics/src/api/http_live_tests.rs`:
- Around line 476-482: Extend the update test after the successful PUT request
to verify that the query’s name and SQL were changed to “renamed” and “SELECT
2”, not only that the response status is OK. Reuse the existing query-fetching
or response-parsing helper in this test module to assert both updated fields.
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 47-48: Update the saved-query handler to construct and return the
declared SavedQueryListResponse instead of an ad-hoc serde_json::json! map,
adding SavedQueryListResponse to the crate::domain::saved_query imports and
preserving the collected items in its expected field.
🪄 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: a73911ad-4f0b-4735-9f3c-53f0debe9079
📥 Commits
Reviewing files that changed from the base of the PR and between 6a812abc0ae4bccbe04e6996589ab55ab8e38828 and 0eff4bb.
📒 Files selected for processing (18)
docs/components/backend/analytics/openapi.jsondocs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/api/error.rssrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/api/openapi_tests.rssrc/backend/services/analytics/src/api/saved_queries.rssrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/domain/saved_query.rssrc/backend/services/analytics/src/infra/db/entities.rssrc/backend/services/analytics/src/migration/m20260730_000001_saved_queries.rssrc/backend/services/analytics/src/migration/mod.rssrc/ingestion/tests/e2e/api/__init__.pysrc/ingestion/tests/e2e/api/conftest.pysrc/ingestion/tests/e2e/api/endpoint_helpers.pysrc/ingestion/tests/e2e/api/test_queries.pysrc/ingestion/tests/e2e/lib/api_coverage.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ingestion/tests/e2e/api/init.py
- docs/domain/presentation-layer/specs/PRD.md
- docs/domain/presentation-layer/specs/DESIGN.md
| let resp = app_a | ||
| .oneshot(json_req("POST", "/v1/queries", &create_body())?) | ||
| .await?; | ||
| assert_eq!(resp.status(), StatusCode::CREATED); | ||
| let created = body_json(resp).await?; | ||
| let id = created["id"].as_str().unwrap_or_default().to_owned(); | ||
|
|
||
| let app_b = app(db, Uuid::now_v7()); | ||
| let resp = app_b.oneshot(get(&format!("/v1/queries/{id}"))?).await?; | ||
| assert_eq!( | ||
| resp.status(), | ||
| StatusCode::NOT_FOUND, | ||
| "tenant B must not see tenant A's saved query" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
unwrap_or_default() makes the tenant-isolation assertion vacuous.
If the create response lacks a string id, id becomes "", the request degrades to /v1/queries/, and the 404 assertion still passes — so the test could green without ever exercising cross-tenant scoping. Line 530 has the same pattern (there it merely fails confusingly). Use the explicit-panic form already used at Lines 452-455.
💚 Proposed change
- let id = created["id"].as_str().unwrap_or_default().to_owned();
+ let id = created["id"]
+ .as_str()
+ .unwrap_or_else(|| panic!("created payload missing string id: {created}"))
+ .to_owned();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let resp = app_a | |
| .oneshot(json_req("POST", "/v1/queries", &create_body())?) | |
| .await?; | |
| assert_eq!(resp.status(), StatusCode::CREATED); | |
| let created = body_json(resp).await?; | |
| let id = created["id"].as_str().unwrap_or_default().to_owned(); | |
| let app_b = app(db, Uuid::now_v7()); | |
| let resp = app_b.oneshot(get(&format!("/v1/queries/{id}"))?).await?; | |
| assert_eq!( | |
| resp.status(), | |
| StatusCode::NOT_FOUND, | |
| "tenant B must not see tenant A's saved query" | |
| ); | |
| let resp = app_a | |
| .oneshot(json_req("POST", "/v1/queries", &create_body())?) | |
| .await?; | |
| assert_eq!(resp.status(), StatusCode::CREATED); | |
| let created = body_json(resp).await?; | |
| let id = created["id"] | |
| .as_str() | |
| .unwrap_or_else(|| panic!("created payload missing string id: {created}")) | |
| .to_owned(); | |
| let app_b = app(db, Uuid::now_v7()); | |
| let resp = app_b.oneshot(get(&format!("/v1/queries/{id}"))?).await?; | |
| assert_eq!( | |
| resp.status(), | |
| StatusCode::NOT_FOUND, | |
| "tenant B must not see tenant A's saved query" | |
| ); |
🤖 Prompt for 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.
In `@src/backend/services/analytics/src/api/http_live_tests.rs` around lines 617 -
630, Replace the unwrap_or_default() extraction of the created query ID in the
tenant-isolation test with the explicit-panic form used around the existing ID
assertions near lines 452-455, so a missing or non-string ID fails immediately
instead of issuing a request with an empty path segment. Apply the same
correction to the matching ID extraction around line 530.
| validate_single_select(&req.sql).map_err(invalid_sql)?; | ||
|
|
||
| let id = Uuid::now_v7(); | ||
| let model = saved_queries::ActiveModel { | ||
| id: Set(id), | ||
| insight_tenant_id: Set(ctx.subject_tenant_id()), | ||
| name: Set(req.name), | ||
| description: Set(req.description), | ||
| sql: Set(req.sql), | ||
| created_at: NotSet, | ||
| updated_at: NotSet, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'saved_queries' -p src/backend/services/analytics/src/migration --exec cat -nRepository: constructorfabric/insight
Length of output: 3518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find saved_queries.rs and relevant validation symbols =="
git ls-files | rg 'src/backend/services/analytics/src/api/saved_queries\.rs|validate_single_select|SavedQuery|saved_query|saved_queries'
echo
echo "== outline saved_queries.rs =="
ast-grep outline src/backend/services/analytics/src/api/saved_queries.rs --view expanded || true
echo
echo "== saved_queries.rs relevant lines =="
cat -n src/backend/services/analytics/src/api/saved_queries.rs | sed -n '1,140p'
echo
echo "== name validation usages/imports =="
rg -n "validate_short|validate_name|name validation|name:" src/backend/services/analytics/src -S || true
rg -n "validate_single_select|sql_len|name" src/backend/services/analytics/src -S | head -n 200 || trueRepository: constructorfabric/insight
Length of output: 42409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== saved_query domain model =="
cat -n src/backend/services/analytics/src/domain/saved_query.rs | sed -n '1,120p'
echo
echo "== canonical JSON error path around 4xx handling =="
cat -n src/backend/services/analytics/src/api/canonical_json.rs | sed -n '350,530p'
echo
echo "== invalid_sql helper =="
cat -n src/backend/services/analytics/src/api/saved_queries.rs | sed -n '220,232p'
echo
echo "== HTTP live test saving snippets =="
cat -n src/backend/services/analytics/src/api/http_live_tests.rs | sed -n '420,495p'Repository: constructorfabric/insight
Length of output: 8580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== error module outline =="
ast-grep outline src/backend/services/analytics/src/api/error.rs --view expanded || true
echo
echo "== SavedQueryError definitions =="
cat -n src/backend/services/analytics/src/api/error.rs | sed -n '1,220p'Repository: constructorfabric/insight
Length of output: 12375
Validate saved-query name bounds before insert
name is deserialized freely and persisted as string_len(255), so values that exceed that length or are otherwise outside the intended contract can still reach a 500 write failure. Add a name length/presence check next to validate_single_select so the API returns a 400 field_violation with field: "name" instead.
The timestamp defaults are already covered by the migration defaults.
🤖 Prompt for 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.
In `@src/backend/services/analytics/src/api/saved_queries.rs` around lines 65 -
76, Validate the saved-query name immediately after validate_single_select and
before constructing saved_queries::ActiveModel, rejecting missing or
out-of-contract lengths with the existing 400 field_violation response using
field: "name". Reuse the established validation/error helper and preserve the
migration-backed created_at and updated_at defaults.
| let saved = find_saved_query(&state, ctx.subject_tenant_id(), id).await?; | ||
|
|
||
| // Re-validate on run: the gate is the write-side barrier, but stored SQL is | ||
| // gated again here so a run can never reach ClickHouse with anything but a | ||
| // single read (defense in depth alongside the `presentation_ro` grants). | ||
| validate_single_select(&saved.sql).map_err(|e| invalid_sql_for(id, e))?; | ||
|
|
||
| // #1966 (named params) and #1967 (injected tenant-row filter) extend this | ||
| // path; Phase-A #1965 executes the stored single-SELECT as authored. | ||
| let rows = execute_read(&state, &saved.sql).await?; | ||
| Ok(Json(RunResponse { rows })) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Shipping /run before the tenant-row filter (#1967) leaves a cross-tenant read path.
find_saved_query scopes which query a caller may run, but the SQL itself executes as authored with no tenant predicate, so a saved query that selects without an insight_tenant_id filter returns other tenants' rows to whoever runs it. The single-SELECT gate and presentation_ro grants constrain the shape of the read, not its scope.
If #1967 can't land in this PR, consider gating /run behind a feature flag or restricting execution to tenant-filtered views until the predicate injection exists.
🤖 Prompt for 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.
In `@src/backend/services/analytics/src/api/saved_queries.rs` around lines 145 -
155, Prevent the `/run` path in the saved-query execution flow from running SQL
without tenant scoping: either implement the `#1967` tenant-row predicate
injection before `execute_read`, or gate/restrict this endpoint until that
protection exists. Keep `find_saved_query` and `validate_single_select` checks
intact, and ensure every query reaching `execute_read` is constrained to
`ctx.subject_tenant_id()`.
| async fn execute_read( | ||
| state: &AppState, | ||
| sql: &str, | ||
| ) -> Result<Vec<serde_json::Value>, CanonicalError> { | ||
| tracing::debug!(sql = %sql, "executing saved query"); | ||
|
|
||
| let mut cursor = state | ||
| .ch | ||
| .query(sql) | ||
| .fetch_bytes("JSONEachRow") | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, sql = %sql, "ClickHouse query failed"); | ||
| CanonicalError::internal("query execution failed").create() | ||
| })?; | ||
|
|
||
| let raw_bytes = cursor.collect().await.map_err(|e| { | ||
| tracing::error!(error = %e, sql = %sql, "ClickHouse fetch failed"); | ||
| CanonicalError::internal("query execution failed").create() | ||
| })?; | ||
|
|
||
| parse_json_each_row(&raw_bytes).map_err(|e| { | ||
| tracing::error!(error = %e, "failed to parse ClickHouse JSON response"); | ||
| CanonicalError::internal("failed to parse query results").create() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo fail
echo "== find relevant files =="
fd -a 'saved_queries\.rs|Cargo\.toml|Cargo\.lock' . | sed 's#^\./##'
echo
echo "== inspect saved_queries context =="
if [ -f src/backend/services/analytics/src/api/saved_queries.rs ]; then
nl -ba src/backend/services/analytics/src/api/saved_queries.rs | sed -n '1,240p'
fi
echo
echo "== clickhouse crate deps =="
rg -n 'clickhouse|user-agent|fetch_bytes|with_option|max_execution_time|max_result_rows|saved-query|user|Query' Cargo.toml Cargo.lock src/backend/services/analytics -S || true
echo
echo "== lock clickhouse versions =="
python3 - <<'PY'
from pathlib import Path
p=Path('Cargo.lock')
if p.exists():
text=p.read_text()
for i,line in enumerate(text.splitlines(),1):
if 'name = "clickhouse"' in line or 'name = "clickhouse-rs"' in line or 'name = "clickhouse"' in line:
print(f'--- around line {i} ---')
for k in range(max(1,i-15), min(len(text.splitlines()),i+25)+1):
print(f'{k}: {text.splitlines()[k-1]}')
PYRepository: constructorfabric/insight
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -e
echo "== find relevant files =="
fd -a 'saved_queries\.rs|Cargo\.toml|Cargo\.lock' . | sed 's#^\./##'
echo
echo "== inspect saved_queries context =="
if [ -f src/backend/services/analytics/src/api/saved_queries.rs ]; then
nl -ba src/backend/services/analytics/src/api/saved_queries.rs | sed -n '1,240p'
fi
echo
echo "== clickhouse crate deps =="
rg -n 'clickhouse|user-agent|fetch_bytes|with_option|max_execution_time|max_result_rows|saved-query|user|Query' Cargo.toml Cargo.lock src/backend/services/analytics -S || true
echo
echo "== lock clickhouse versions =="
python3 - <<'PY'
from pathlib import Path
p=Path('Cargo.lock')
if p.exists():
text=p.read_text()
lines=text.splitlines()
for i,line in enumerate(lines, 1):
if 'name = "clickhouse"' in line or 'name = "clickhouse-rust"' in line:
print(f'--- around line {i} ---')
for k in range(max(1, i-15), min(len(lines), i+25)+1):
print(f'{k}: {lines[k-1]}')
PYRepository: constructorfabric/insight
Length of output: 44532
🏁 Script executed:
#!/bin/bash
set -e
echo "== saved_queries selected =="
sed -n '130,210p' src/backend/services/analytics/src/api/saved_queries.rs
echo
echo "== metric_results fetch_rows context =="
sed -n '160,210p' src/backend/services/analytics/src/api/metric_results.rs
sed -n '430,510p' src/backend/services/analytics/src/api/metric_results.rs
echo
echo "== clickhouse crate source presence =="
fd 'clickhouse|insight-clickhouse' . | sed 's#^\./##' | head -50
echo
echo "== exact file sizes relevant =="
wc -l src/backend/services/analytics/src/api/saved_queries.rs src/backend/services/analytics/src/api/metric_results.rsRepository: constructorfabric/insight
Length of output: 5728
🌐 Web query:
clickhouse-rs 1.4.0 select max_result_rows max_execution_time result_overflow_mode query options fetch_bytes collect Cursor documentation
💡 Result:
The crate named clickhouse (formerly often referred to or associated with the clickhouse-rs ecosystem) provides a typed Rust client for ClickHouse [1][2][3]. As of version 0.14.x, the official crate is available under the name clickhouse [1][3]. Query Options and Settings The settings you mentioned—max_result_rows, max_execution_time, and result_overflow_mode—are ClickHouse server-side settings [4][5]. They are not specifically mapped as dedicated builder methods on the client query object in the Rust crate's public API [6][7]. Instead, you typically apply these settings by passing them in your query using the ClickHouse SETTINGS clause, for example: SELECT * FROM table SETTINGS max_result_rows = 1000, max_execution_time = 60, result_overflow_mode = 'break' Fetch, BytesCursor, and Collection The client provides flexible ways to execute queries and handle results [6][8]: 1. fetch_bytes: The Query::fetch_bytes(format) method executes a query and returns a BytesCursor [6][7]. This cursor allows you to consume the raw response bytes without deserialization [9]. 2. BytesCursor::collect: You can use the collect method on a BytesCursor to read the entire response body into a single Bytes buffer [9]. 3. RowCursor::collect: For typed results, a RowCursor (returned by query.fetch::) also provides collection methods like collect and try_collect to transform the stream of rows into a collection (e.g., a Vec) [10]. Note on documentation: Always ensure you are referencing the official crate documentation on docs.rs under the name clickhouse, as it replaced older iterations of ClickHouse Rust clients [1][3].
Citations:
- 1: https://docs.rs/crate/clickhouse/0.14.0
- 2: https://clickhouse.com/docs/integrations/rust
- 3: https://github.com/clickhouse/clickhouse-rs
- 4: https://clickhouse.com/docs/concepts/features/configuration/settings/query-complexity
- 5: https://github.com/ClickHouse/ClickHouse/blob/master/docs/en/operations/settings/query-complexity.md
- 6: https://docs.rs/clickhouse/latest/clickhouse/query/struct.Query.html
- 7: https://docs.rs/clickhouse/latest/src/clickhouse/query.rs.html
- 8: https://docs.rs/clickhouse/latest/clickhouse/query/index.html
- 9: https://docs.rs/sea-clickhouse/latest/clickhouse/query/struct.BytesCursor.html
- 10: https://docs.rs/clickhouse/latest/clickhouse/query/struct.RowCursor.html
🏁 Script executed:
#!/bin/bash
set -e
echo "== insight-clickhouse library files =="
find src/backend/libs/insight-clickhouse src/backend -path '*/insight-clickhouse/src/*' -maxdepth 4 -type f 2>/dev/null | sort
echo
echo "== inspect insight-clickhouse files =="
for f in $(find src/backend/libs/insight-clickhouse -maxdepth 5 \( -name '*.rs' -o -name 'Cargo.toml' \) -type f | sort); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
done
echo
echo "== constants related fetch timeout =="
rg -n 'QUERY_FETCH_TIMEOUT|fetch_bytes|max_execution_time|result_overflow|max_result' src/backend/services/analytics/src -S || true
echo
echo "== parsed query gateway for saved query =="
rg -n 'validate_single_select|presentation_ro|QueryGate|gate|saved' src/backend/services/analytics/src/domain src/backend/services/analytics/src/api -S || trueRepository: constructorfabric/insight
Length of output: 43179
🌐 Web query:
crates clickhouse with_option max_execution_time result_overflow_mode latest docs.rs clickhouse query with_option
💡 Result:
In the popular ClickHouse Rust client (crate clickhouse), you can apply query settings such as max_execution_time and result_overflow_mode using the with_setting method [1][2]. While older versions of the crate provided a with_option method, it has been deprecated since version 0.14.3 in favor of with_setting [1][2][3]. Both the Client (for global settings) and Query (for per-query settings) structs support this [1][2]. Example usage: let client = Client::default.with_url("http://localhost:8123"); let result = client.query("SELECT * FROM large_table").with_setting("max_execution_time", "60") // Time in seconds.with_setting("result_overflow_mode", "break") // e.g., 'throw', 'break'.fetch_all::.await?; You should use with_setting instead of with_option to ensure compatibility with modern versions of the crate [1][2][3].
Citations:
- 1: https://docs.rs/clickhouse/latest/clickhouse/struct.Client.html
- 2: https://docs.rs/clickhouse/latest/clickhouse/query/struct.Query.html
- 3: https://docs.rs/clickhouse/latest/src/clickhouse/query.rs.html
Bound saved-query execution before collecting the full response.
execute_read() sends the stored SELECT only with the wrapper’s default timeout/memory settings and then calls cursor.collect() before parsing, so rows keep accumulating in memory until the entire response is stored. Add a server-side row limit/overflow mode, e.g. result_overflow_mode = "throw" plus max_result_rows, so rejected oversized result sets don’t OOM the service in this path.
The local option name is not a blocker here; the ClickHouse client underlies these settings via the query options/settings API.
🤖 Prompt for 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.
In `@src/backend/services/analytics/src/api/saved_queries.rs` around lines 161 -
185, Update execute_read to apply ClickHouse query settings before
fetch_bytes/collect, including a bounded max_result_rows value and
result_overflow_mode set to throw. Reuse the client’s query options/settings API
so oversized saved-query responses are rejected server-side before
cursor.collect accumulates them, while preserving existing error handling.
4 participants
What
Phase A of the presentation-layer split (#1803): the Saved-Query CRUD + run surface — the one new "Data Analytics" API the phase adds.
GET/POST/PUT/DELETE /v1/queries+POST /v1/queries/{id}/run, tenant-scoped from the sessionSecurityContext.saved_queriestable in the analytics service DB (MariaDB), a SeaORM entity mirroring the existing metric CRUD. CRUD is metadata management — it never touches ClickHouse. Only/runreaches ClickHouse.validate_single_select, [pres] Single-SELECT gate on the query path #1962) validates the storedsqlon create, update, and run — defense in depth alongside thepresentation_rogrants ([pres] presentation_ro ClickHouse role #1963/[pres] Create presentation DB + wire analytics to presentation_ro #1964)./runexecutes the stored SELECT read-only aspresentation_roand returns untypedJSONEachRowrows (same shape as the metric query path).Scope boundary
Named query parameters (
tenant/period, #1966) and the injected tenant-row filter (#1967) are separate sub-issues. #1965 executes the stored single-SELECT as authored; the run path is the seam those extend.Specs
The presentation PRD/DESIGN are governed
cfsartifacts. This PR:cpt-presentation-fr-saved-query-crud, "like metric definitions … no write grant on the contract is ever needed"), not the ClickHousepresentationnamespace that §3.1/§3.7 previously stated.[x]).cfs validate --skip-code --local-only; the change adds zero new registry errors.Tests
cargo test -p analytics— 479 passed (route-table + OpenAPI drift-doc coverage extended for/v1/queries*;SavedQueryErrorenvelope test added).cargo clippy -p analytics --all-targets— clean.docs/components/backend/analytics/openapi.jsonviascripts/ci/openapi_spec.py update(drift gate passes).Closes #1965
Part of #1803
Summary by CodeRabbit
POST /v1/queries/{id}/run.