feat(analytics): saved-query export/import — promote experiment queries dev->prod (#2259) - #2260
Conversation
…es across stands (constructorfabric#2259) Two thin endpoints over the saved-query CRUD so a promoted tier-3 experiment arrives with the queries it depends on, instead of leaving them stranded on the dev stand's database: - GET /v1/queries/export dumps the tenant's queries as portable JSON ({name, description, sql} only — no id, tenant, or timestamps). - POST /v1/queries/import bulk-creates from that document, re-validating each SQL through the single-SELECT gate, dropping any source id/tenant and re-homing each row to the importing session's tenant with a fresh id. Portable by construction: the SQL is contract-relative and the tenant is session-injected at run time, so no rewriting is needed. Same-name collisions are skipped (never overwritten), so a re-import is idempotent; the response reports imported vs skipped counts. No schema change — reuses saved_queries. Extends the presentation PRD/DESIGN (export/import FR + the two endpoints under API Contracts), regenerates the analytics OpenAPI contract, and adds stand endpoint-contract tests plus the coverage-gate wiring for the two operations. Closes constructorfabric#2259 Part of constructorfabric#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: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (7)
📝 WalkthroughWalkthroughAdds portable saved-query export and import contracts and authenticated endpoints. Imports revalidate SQL, re-home queries to the importing tenant with fresh IDs, skip name collisions, and return imported and skipped counts. ChangesSaved-query transfer
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant saved_queries
participant saved_queries_table
participant validate_single_select
Client->>saved_queries: GET /v1/queries/export
saved_queries->>saved_queries_table: Load tenant queries
saved_queries_table-->>saved_queries: Tenant saved queries
saved_queries-->>Client: SavedQueryExport
Client->>saved_queries: POST /v1/queries/import
saved_queries->>validate_single_select: Validate each SQL statement
validate_single_select-->>saved_queries: Validation result
saved_queries->>saved_queries_table: Insert accepted queries for session tenant
saved_queries_table-->>saved_queries: Insert result
saved_queries-->>Client: ImportResponse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (5)
tests/stand/api/analytics/test_queries.py (2)
339-346: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the all-or-nothing import contract.
The document contains only an invalid query. It cannot show whether a valid query before the invalid query was persisted. Add a unique valid query before
DROP TABLE metrics, then assert that neither name exists after the 400 response.🤖 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 `@tests/stand/api/analytics/test_queries.py` around lines 339 - 346, Add a uniquely named valid query before the invalid DROP TABLE metrics entry in the import document, then update the post-response assertions to verify that neither the valid query name nor the existing invalid query name appears in _ids_by_name(api) after the 400 response, preserving the all-or-nothing contract check.
292-296: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert
descriptionin both transfer directions.The portable contract includes
description. The export test checks only name and SQL. The import test checks only the imported name. A regression that dropsdescriptionpasses both tests.
tests/stand/api/analytics/test_queries.py#L292-L296: Assert the exported query description equalsscratch_saved_query.description.tests/stand/api/analytics/test_queries.py#L307-L319: Read the imported list item and assert its description equals"imported by the stand suite".🤖 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 `@tests/stand/api/analytics/test_queries.py` around lines 292 - 296, Extend the export assertion around the SavedQueryExport response at tests/stand/api/analytics/test_queries.py:292-296 to verify the exported query description matches scratch_saved_query.description. Also update the import assertions at tests/stand/api/analytics/test_queries.py:307-319 to read the imported list item and verify its description is "imported by the stand suite".src/backend/services/analytics/src/api/saved_queries.rs (1)
463-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one table-driven collision test.
The three tests exercise one partitioning rule with different inputs. Put the cases in one table-driven loop and add assertion messages that identify the case.
As per coding guidelines: “Make tests read as specifications: use table-driven loops with per-case assertion messages.”
🤖 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 463 - 490, Replace the three separate tests around select_new_queries with one table-driven test containing the existing collision, duplicate, and distinct-name cases. Iterate over named cases, preserve each expected fresh-name and skipped-count assertion, and include the case name in assertion messages so failures identify the scenario.Source: Coding guidelines
src/backend/services/analytics/src/api/mod.rs (1)
226-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove delivery-context comments from the route table.
The route names, DTOs, and handlers already express the behavior. Keep issue numbers and promotion scope in the PR and design documentation.
As per coding guidelines, “Use comments only when code cannot express the reason” and “Do not use module headers, issue numbers, or phase/scope notes in source comments.”
🤖 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/mod.rs` around lines 226 - 228, Remove the delivery-context comment block above the export/import route definitions, including the issue number and promotion-scope notes; leave the route names, DTOs, and handlers unchanged.Source: Coding guidelines
src/backend/services/analytics/src/domain/saved_query.rs (1)
83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove service-level documentation comments.
These comments describe DTO fields and endpoint behavior in a service crate. Keep this content in the API contract or design documentation instead.
As per coding guidelines, “Use
///documentation comments only on exported items in shared library crates; … Do not add documentation comments to binaries or services.”Also applies to: 97-105
🤖 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/domain/saved_query.rs` around lines 83 - 88, Remove the service-level `///` documentation comments associated with the saved-query export/import record, including the block near the record definition and the additional block around lines 97–105. Leave the underlying types and implementation unchanged.Source: Coding guidelines
🤖 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/components/backend/analytics/openapi.json`:
- Around line 2122-2225: Add 415 Unsupported Media Type and 422 Unprocessable
Entity responses to the POST operation identified by operationId
analytics_api.queries.import, using the existing application/problem+json
Problem schema pattern. Preserve the current 400 response and accurately
document the actual text/plain and off-schema JSON failure statuses.
In `@docs/domain/presentation-layer/specs/PRD.md`:
- Line 213: Update the saved-query export/import requirement to use the
implemented document envelope with a top-level queries property containing the
query objects, matching the OpenAPI contract. Preserve the existing fields and
import/export behavior while replacing the top-level array representation.
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 176-192: Update the saved-query import flow around
load_query_names and Entity::insert_many to enforce uniqueness on
(insight_tenant_id, name), use conflict-ignore insertion, and count rows
rejected by the conflict as skipped. Preserve the existing fresh/skipped
classification while ensuring concurrent imports do not return a 500 for
duplicate names.
- Around line 159-199: Refactor import_saved_queries so it only extracts the
tenant and request data, calls a focused application/domain import operation,
and maps its result or error to the HTTP response. Move SQL validation,
existing-name loading, duplicate/collision selection, row construction,
persistence, and import-specific error mapping from import_saved_queries into
that operation, preserving the current all-or-nothing validation, skip behavior,
and ImportResponse fields.
- Line 140: Remove redundant commentary from
src/backend/services/analytics/src/api/saved_queries.rs at lines 140, 166-175,
201-218, 237-238, 403-404, 463-475, and 484: delete the module header,
import-flow narration, private helper and loader documentation, private
error-helper documentation, and test comments that duplicate test names or
intent. Keep code behavior unchanged and retain documentation only for exported
items or comments expressing non-obvious rationale.
In `@src/backend/services/analytics/src/domain/saved_query.rs`:
- Around line 100-102: Bound SavedQueryExport.queries at the route boundary for
both import and export: enforce documented item-count and payload-size limits
before deserialization or persistence, reject oversized imports, and cap or
paginate exports so responses cannot exceed the limits. Define and reuse symbols
such as MAX_SAVED_QUERY_TRANSFER_ITEMS and the payload-size limit across the
relevant handlers.
---
Nitpick comments:
In `@src/backend/services/analytics/src/api/mod.rs`:
- Around line 226-228: Remove the delivery-context comment block above the
export/import route definitions, including the issue number and promotion-scope
notes; leave the route names, DTOs, and handlers unchanged.
In `@src/backend/services/analytics/src/api/saved_queries.rs`:
- Around line 463-490: Replace the three separate tests around
select_new_queries with one table-driven test containing the existing collision,
duplicate, and distinct-name cases. Iterate over named cases, preserve each
expected fresh-name and skipped-count assertion, and include the case name in
assertion messages so failures identify the scenario.
In `@src/backend/services/analytics/src/domain/saved_query.rs`:
- Around line 83-88: Remove the service-level `///` documentation comments
associated with the saved-query export/import record, including the block near
the record definition and the additional block around lines 97–105. Leave the
underlying types and implementation unchanged.
In `@tests/stand/api/analytics/test_queries.py`:
- Around line 339-346: Add a uniquely named valid query before the invalid DROP
TABLE metrics entry in the import document, then update the post-response
assertions to verify that neither the valid query name nor the existing invalid
query name appears in _ids_by_name(api) after the 400 response, preserving the
all-or-nothing contract check.
- Around line 292-296: Extend the export assertion around the SavedQueryExport
response at tests/stand/api/analytics/test_queries.py:292-296 to verify the
exported query description matches scratch_saved_query.description. Also update
the import assertions at tests/stand/api/analytics/test_queries.py:307-319 to
read the imported list item and verify its description is "imported by the stand
suite".
🪄 Autofix
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: 4f6cf336-432c-44ec-984d-6461ae8f575b
📒 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/mod.rssrc/backend/services/analytics/src/api/openapi_tests.rssrc/backend/services/analytics/src/api/saved_queries.rssrc/backend/services/analytics/src/domain/saved_query.rstests/lib/insight_stand/coverage.pytests/stand/api/analytics/test_queries.pytests/stand/api/analytics/test_request_contracts.pytests/stand/api/operations.pytests/stand/api/schemas/__init__.pytests/stand/api/schemas/analytics.py
| "/v1/queries/import": { | ||
| "post": { | ||
| "operationId": "analytics_api.queries.import", | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/SavedQueryExport" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Portable saved-query document to bulk-create", | ||
| "required": true | ||
| }, | ||
| "responses": { | ||
| "201": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ImportResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Imported and skipped counts" | ||
| }, | ||
| "400": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Bad Request" | ||
| }, | ||
| "401": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Unauthorized" | ||
| }, | ||
| "403": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Forbidden" | ||
| }, | ||
| "404": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Not Found" | ||
| }, | ||
| "409": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Conflict" | ||
| }, | ||
| "429": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Too Many Requests" | ||
| }, | ||
| "500": { | ||
| "content": { | ||
| "application/problem+json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/Problem" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Internal Server Error" | ||
| } | ||
| }, | ||
| "security": [ | ||
| { | ||
| "bearerAuth": [] | ||
| } | ||
| ], | ||
| "summary": "Import saved queries" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the actual import failure responses.
POST /v1/queries/import returns 415 for text/plain and 422 for off-schema JSON. This contract declares 400 but omits both actual responses. Clients generated from this OpenAPI document cannot rely on the documented error contract.
Add the 415 and 422 responses, or map extractor failures to the documented status code.
🤖 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/components/backend/analytics/openapi.json` around lines 2122 - 2225, Add
415 Unsupported Media Type and 422 Unprocessable Entity responses to the POST
operation identified by operationId analytics_api.queries.import, using the
existing application/problem+json Problem schema pattern. Preserve the current
400 response and accurately document the actual text/plain and off-schema JSON
failure statuses.
| Ok(StatusCode::NO_CONTENT) | ||
| } | ||
|
|
||
| // ── Export / Import ───────────────────────────────────────── |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove commentary that duplicates code or test intent.
Keep source comments only where code cannot express the reason. Remove private /// documentation comments from this service. Use test names to state the collision and import rules.
src/backend/services/analytics/src/api/saved_queries.rs#L140-L140: Remove the module-style source header.src/backend/services/analytics/src/api/saved_queries.rs#L166-L175: Remove the import-flow narration.src/backend/services/analytics/src/api/saved_queries.rs#L201-L218: Remove private helper documentation comments.src/backend/services/analytics/src/api/saved_queries.rs#L237-L238: Remove the private loader documentation comment.src/backend/services/analytics/src/api/saved_queries.rs#L403-L404: Remove the private error-helper documentation comment.src/backend/services/analytics/src/api/saved_queries.rs#L463-L475: Remove test documentation comments that restate test names.src/backend/services/analytics/src/api/saved_queries.rs#L484-L484: Remove the test documentation comment that restates test intent.
As per coding guidelines: “Use comments only when code cannot express the reason” and “Use /// documentation comments only on exported items in shared library crates.”
📍 Affects 1 file
src/backend/services/analytics/src/api/saved_queries.rs#L140-L140(this comment)src/backend/services/analytics/src/api/saved_queries.rs#L166-L175src/backend/services/analytics/src/api/saved_queries.rs#L201-L218src/backend/services/analytics/src/api/saved_queries.rs#L237-L238src/backend/services/analytics/src/api/saved_queries.rs#L403-L404src/backend/services/analytics/src/api/saved_queries.rs#L463-L475src/backend/services/analytics/src/api/saved_queries.rs#L484-L484
🤖 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` at line 140, Remove
redundant commentary from
src/backend/services/analytics/src/api/saved_queries.rs at lines 140, 166-175,
201-218, 237-238, 403-404, 463-475, and 484: delete the module header,
import-flow narration, private helper and loader documentation, private
error-helper documentation, and test comments that duplicate test names or
intent. Keep code behavior unchanged and retain documentation only for exported
items or comments expressing non-obvious rationale.
Source: Coding guidelines
| pub async fn import_saved_queries( | ||
| Extension(state): Extension<Arc<AppState>>, | ||
| Extension(ctx): Extension<SecurityContext>, | ||
| Json(doc): Json<SavedQueryExport>, | ||
| ) -> Result<impl IntoResponse, CanonicalError> { | ||
| let tenant_id = ctx.subject_tenant_id(); | ||
|
|
||
| // Re-gate every SQL before writing anything: one bad statement rejects the | ||
| // whole document, so an import is all-valid-or-nothing. | ||
| for query in &doc.queries { | ||
| validate_single_select(&query.sql).map_err(|e| invalid_import_sql(&query.name, e))?; | ||
| } | ||
|
|
||
| // Names already taken for this tenant are skipped, never overwritten; the | ||
| // pure partition also absorbs duplicate names inside the document, so a | ||
| // re-import is idempotent. Source id/tenant are dropped — every kept row is | ||
| // re-homed to the importing session's tenant with a fresh id. | ||
| let existing = load_query_names(&state, tenant_id).await?; | ||
| let (fresh, skipped) = select_new_queries(existing, doc.queries); | ||
|
|
||
| let rows: Vec<saved_queries::ActiveModel> = fresh | ||
| .into_iter() | ||
| .map(|query| new_query_row(tenant_id, query)) | ||
| .collect(); | ||
|
|
||
| let imported = rows.len(); | ||
| if !rows.is_empty() { | ||
| saved_queries::Entity::insert_many(rows) | ||
| .exec(&state.db) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "failed to import saved queries"); | ||
| CanonicalError::internal("failed to import saved queries").create() | ||
| })?; | ||
| } | ||
|
|
||
| Ok(( | ||
| StatusCode::CREATED, | ||
| Json(ImportResponse { imported, skipped }), | ||
| )) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move import policy out of the API handler.
import_saved_queries performs validation, collision policy, row construction, persistence, and response mapping. Keep the handler to extraction, one application call, and HTTP response mapping. Put the import use case and its error mapping in a focused application or domain module.
As per coding guidelines: “Keep API handlers to an orchestration skeleton of extract → validate → domain call → map → respond, with approximately 30 lines maximum; keep business logic and serialization formats out of the API layer.”
🤖 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 159 -
199, Refactor import_saved_queries so it only extracts the tenant and request
data, calls a focused application/domain import operation, and maps its result
or error to the HTTP response. Move SQL validation, existing-name loading,
duplicate/collision selection, row construction, persistence, and
import-specific error mapping from import_saved_queries into that operation,
preserving the current all-or-nothing validation, skip behavior, and
ImportResponse fields.
Source: Coding guidelines
| let existing = load_query_names(&state, tenant_id).await?; | ||
| let (fresh, skipped) = select_new_queries(existing, doc.queries); | ||
|
|
||
| let rows: Vec<saved_queries::ActiveModel> = fresh | ||
| .into_iter() | ||
| .map(|query| new_query_row(tenant_id, query)) | ||
| .collect(); | ||
|
|
||
| let imported = rows.len(); | ||
| if !rows.is_empty() { | ||
| saved_queries::Entity::insert_many(rows) | ||
| .exec(&state.db) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "failed to import saved queries"); | ||
| CanonicalError::internal("failed to import saved queries").create() | ||
| })?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make name collision handling atomic.
Two imports can both load the same free name, classify it as fresh, and then insert it. Without a database uniqueness constraint, this creates duplicates. With a uniqueness constraint, the later request returns a 500 instead of reporting a skipped name.
Enforce uniqueness on (insight_tenant_id, name) and use a conflict-ignore insert. Include conflict-skipped rows in skipped.
🤖 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 176 -
192, Update the saved-query import flow around load_query_names and
Entity::insert_many to enforce uniqueness on (insight_tenant_id, name), use
conflict-ignore insertion, and count rows rejected by the conflict as skipped.
Preserve the existing fresh/skipped classification while ensuring concurrent
imports do not return a 500 for duplicate names.
| pub struct SavedQueryExport { | ||
| pub queries: Vec<PortableSavedQuery>, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound saved-query transfer size.
queries is unbounded for both import input and export output. The handlers deserialize or collect the full document before persistence or response serialization. A tenant can cause excessive memory use and an oversized database batch.
Add documented limits such as MAX_SAVED_QUERY_TRANSFER_ITEMS and a payload-size limit at the route boundary. Reject oversized imports. Cap or paginate exports.
As per coding guidelines, “Bound every unbounded resource at the edge, including concurrent requests, response sizes, and queue depths; a missing bound is a bug.”
🤖 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/domain/saved_query.rs` around lines 100 -
102, Bound SavedQueryExport.queries at the route boundary for both import and
export: enforce documented item-count and payload-size limits before
deserialization or persistence, reject oversized imports, and cap or paginate
exports so responses cannot exceed the limits. Define and reuse symbols such as
MAX_SAVED_QUERY_TRANSFER_ITEMS and the payload-size limit across the relevant
handlers.
Source: Coding guidelines
…sion semantics (constructorfabric#2259) Address review feedback on the saved-query export/import PR: - Bound import input at the route boundary: reject a document over MAX_IMPORT_QUERIES with a 400 before any load or write, so one request cannot force an unbounded batch (src/backend/CLAUDE.md: bound every unbounded thing at the edge). - Align the PRD FR with the implemented `{ "queries": [...] }` document envelope (was a top-level array), matching the OpenAPI contract and DESIGN. - Document in DESIGN that the name-collision skip is best-effort: the surface carries no (insight_tenant_id, name) uniqueness invariant — create already permits duplicate names — so concurrent imports race exactly as concurrent creates do, and export is bounded by construction and never truncates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Thanks @coderabbitai — went through all six. Addressed in a006d64: Applied
Documented rather than changed
Skipped, with reasons
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline a006d64f
git show --format=fuller --find-renames --find-copies -- \
src/backend/services/analytics/src/api/saved_queries.rs \
src/backend/services/analytics/src/domain/saved_query.rs \
docs/domain/presentation-layer/specs/PRD.md \
docs/domain/presentation-layer/specs/DESIGN.md \
.github/workflows/openapi-specs.yml \
docs/components/backend/analytics/openapi.json \
a006d64f
printf '\n--- Relevant symbols and documented statements ---\n'
rg -n -C 4 \
'MAX_IMPORT_QUERIES|import_saved_queries|select_new_queries|new_query_row|best-effort|atomic|queries' \
src/backend/services/analytics/src/api/saved_queries.rs \
docs/domain/presentation-layer/specs/PRD.md \
docs/domain/presentation-layer/specs/DESIGN.md
printf '\n--- OpenAPI generation workflow references ---\n'
rg -n -C 3 'openapi|generate|drift|standard_errors' \
.github/workflows/openapi-specs.yml \
src/backend/services/analytics/src/api \
-g '*.rs'Length of output: 109186
I consider the six findings resolved or withdrawn as applicable. 🐇 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
…solves (constructorfabric#2259) The promoted tier-3 frontend calls each saved query by id (`/v1/queries/{id}/run`) — there is no run-by-name path — so a query that lands under a fresh id on the target stand is unreachable from the FE that was promoted alongside it. Dropping the source id (the earlier design) broke exactly the use case this feature exists for. Import now PRESERVES the source id and re-homes only the tenant; export carries the id. Cross-tenant safety is unchanged — it comes from the tenant-scoped reads (`find`/`list`/`run` filter `insight_tenant_id`), not from rewriting the id. Collision/idempotency is keyed on id, not name (names are not unique): an id already present in the table is skipped, checked globally over the incoming ids because the id is the sole primary key, so an already-taken id cannot be re-homed and skipping it (rather than colliding on insert) keeps a re-import idempotent. DESIGN notes the single-target-tenant consequence and the best-effort concurrency edge. Specs, OpenAPI, generated models, and stand tests updated to match; the import test now asserts the query lands under its own id. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Closing — the design moved. Migrating saved queries doesn't integrate anything into the product FE; the FE renders metrics (by |
2 participants
What
Adds saved-query export/import to the analytics service — two thin endpoints over the existing saved-query CRUD (#1965):
GET /v1/queries/export-> the calling tenant's saved queries as a portable JSON document ({ queries: [{ name, description, sql }] }), carrying no id, tenant, or timestamps.POST /v1/queries/import-> bulk-creates from that document; returns{ imported, skipped }.Why
A tier-3 experiment is built on a dev stand: a bespoke frontend plus new saved queries authored against that stand's data. The frontend promotes to production by PR into
src/frontend, but the saved queries it depends on are otherwise stranded on the dev stand's database. Export/import lets those queries travel dev -> prod alongside the frontend.Mechanism
Portable by construction — no SQL rewriting:
silver.*/ gold), identical across stands.{tenant}parameter), never stored. Import drops any source id/tenant, generates a fresh id, and binds the importing session's tenant, so an import can never write cross-tenant.validate_single_select) — one bad statement rejects the whole document as a 400.namealready exists for the tenant is skipped, never overwritten, so a re-import is idempotent.No schema change — reuses the existing
saved_queriestable.Out of scope
Specs & tests
cfs validateis 0 errors per artifact.Closes #2259
Part of #1803
Summary by CodeRabbit