Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a "Copy Dimensions Between Workspaces" feature alongside a refactoring of dimension CRUD logic. New type definitions define request/response structures for the copy operation. A new backend copy handler validates entities, performs authorization checks, executes dimension copying in transactions, persists config versions to Redis, and triggers webhooks. Existing dimension creation/update logic is extracted into a dedicated operations module and refactored from handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Copy Handler
participant DB as Database
participant Redis
participant Webhook as Webhook Service
Client->>Server: POST /copy-to (CopyToRequest)
Server->>Server: Validate entity type & workspace
Server->>DB: Query source dimensions from workspace
Server->>DB: Lookup target workspace by name
Server->>Server: Per-dimension authorization check
Server->>DB: Begin transaction
Server->>DB: Query existing target dimensions
Server->>Server: Compute positions (append selected)
Server->>DB: Create new dimensions (with audit fields)
Server->>DB: Update existing dimensions (skip_existing mode)
Server->>DB: Commit transaction
Server->>Redis: Store config version (if changes made)
Server->>Webhook: POST webhook to target workspace
Webhook-->>Server: Webhook response/failure
Server->>Client: Return CopyToResponse (with XConfigVersion header)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
sauraww
force-pushed
the
copyTosemantics
branch
from
46f5a76 to
beccf73
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “copy-to-workspace” capability for dimensions, exposing a backend /copy-to endpoint and a frontend UI flow to trigger it, while also refactoring dimension create/update logic into shared operations.
Changes:
- Added shared API request/response types for copy operations (
superposition_types::api::copy_to), and wired the module export. - Implemented a new backend
/copy-toendpoint that copies dimensions between workspaces (with skip/overwrite behavior and webhook/config-version integration). - Updated the Dimensions frontend page to support selecting rows and initiating copy-to-workspace, and refactored dimension handlers by extracting DB operations.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition_types/src/api/copy_to_semantics.rs | Adds copy-to semantics types (currently duplicated/unreferenced). |
| crates/superposition_types/src/api/copy_to.rs | Defines copy-to API types consumed by backend + frontend. |
| crates/superposition_types/src/api.rs | Exports the new copy_to API module. |
| crates/superposition/src/main.rs | Registers /copy-to scope in the server routing. |
| crates/frontend/src/pages/dimensions.rs | Adds UI for selecting dimensions + copying to another workspace. |
| crates/frontend/src/api.rs | Adds frontend client wrapper to call POST /copy-to. |
| crates/context_aware_config/src/api/dimension/operations.rs | Extracts dimension create/update helpers used by multiple handlers. |
| crates/context_aware_config/src/api/dimension/handlers.rs | Refactors handlers to use extracted operations. |
| crates/context_aware_config/src/api/dimension.rs | Exposes the new operations module internally. |
| crates/context_aware_config/src/api/copy_to/handlers.rs | Implements the backend copy-to endpoint for dimensions. |
| crates/context_aware_config/src/api/copy_to.rs | Exposes copy-to endpoints module. |
| crates/context_aware_config/src/api.rs | Exports the new copy_to API module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .clone() | ||
| .unwrap_or_default() | ||
| .contains(&dimension_name.to_string()) |
There was a problem hiding this comment.
is_mandatory_dimension currently clones mandatory_dimensions and allocates a new String on every call (clone().unwrap_or_default().contains(&dimension_name.to_string())). This can get expensive in hot paths like bulk copy. Consider borrowing instead (e.g., as_deref()/as_ref() and compare against &str) to avoid cloning and repeated allocations.
| .clone() | |
| .unwrap_or_default() | |
| .contains(&dimension_name.to_string()) | |
| .as_ref() | |
| .is_some_and(|mandatory_dimensions| { | |
| mandatory_dimensions.iter().any(|mandatory_dimension| mandatory_dimension == dimension_name) | |
| }) |
| prop:checked=move || { | ||
| selected_dimensions_rws | ||
| .get() | ||
| .contains(&checked_dimension_name) | ||
| } |
There was a problem hiding this comment.
selected_dimensions_rws is a Vec<String> and the checkbox rendering does a linear contains(...) lookup per row. With many dimensions this becomes O(n²) work during rendering/interaction. Consider tracking selection as a HashSet<String> (and converting to a Vec only when building the CopyToRequest) to keep lookups O(1).
| Ok(()) | ||
| } | ||
|
|
||
| #[authorized(action = "update")] |
There was a problem hiding this comment.
#[authorized(action = "update")] causes the request extractor to require global Dimension:update permission (with no attributes) before any per-dimension checks run. This can unnecessarily block users who have create (or attribute-scoped) permissions but not global update, even when the copy operation only creates new rows or skips existing ones. Consider using a dedicated authz action for this endpoint (e.g. copy_to) and relying on the explicit per-dimension create/update checks, or dynamically enforcing the appropriate coarse permission based on skip_existing/target existence.
| #[authorized(action = "update")] |
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use crate::database::models::ChangeReason; | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum CopyEntityType { | ||
| Dimensions, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum RowSelectionMode { | ||
| All, | ||
| Selected, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum CopyResultStatus { | ||
| Copied, | ||
| Skipped, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct CopyToSemanticsRequest { | ||
| pub entity_type: CopyEntityType, | ||
| pub target_workspace: String, | ||
| pub selection_mode: RowSelectionMode, | ||
| #[serde(default)] | ||
| pub selected_rows: Vec<String>, | ||
| #[serde(default)] | ||
| pub skip_existing: bool, | ||
| pub change_reason: ChangeReason, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct CopyToSemanticsResult { | ||
| pub row_identifier: String, | ||
| pub status: CopyResultStatus, | ||
| pub message: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct CopyToSemanticsResponse { | ||
| pub entity_type: CopyEntityType, | ||
| pub source_workspace: String, | ||
| pub target_workspace: String, | ||
| pub requested_count: usize, | ||
| pub copied_count: usize, | ||
| pub skipped_count: usize, | ||
| pub failed_count: usize, | ||
| pub results: Vec<CopyToSemanticsResult>, | ||
| } |
There was a problem hiding this comment.
copy_to_semantics duplicates the types in api/copy_to.rs but the module is not exported from crates/superposition_types/src/api.rs and a repo-wide search shows no references to it. This looks like dead/unused API surface and risks the two copies drifting out of sync. Consider removing this file, or exporting and reusing it as the single source of truth (e.g., re-export from api::copy_to/rename accordingly) so there’s only one definition of these request/response types.
| use serde::{Deserialize, Serialize}; | |
| use crate::database::models::ChangeReason; | |
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | |
| #[serde(rename_all = "snake_case")] | |
| pub enum CopyEntityType { | |
| Dimensions, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | |
| #[serde(rename_all = "snake_case")] | |
| pub enum RowSelectionMode { | |
| All, | |
| Selected, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | |
| #[serde(rename_all = "snake_case")] | |
| pub enum CopyResultStatus { | |
| Copied, | |
| Skipped, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| pub struct CopyToSemanticsRequest { | |
| pub entity_type: CopyEntityType, | |
| pub target_workspace: String, | |
| pub selection_mode: RowSelectionMode, | |
| #[serde(default)] | |
| pub selected_rows: Vec<String>, | |
| #[serde(default)] | |
| pub skip_existing: bool, | |
| pub change_reason: ChangeReason, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| pub struct CopyToSemanticsResult { | |
| pub row_identifier: String, | |
| pub status: CopyResultStatus, | |
| pub message: Option<String>, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| pub struct CopyToSemanticsResponse { | |
| pub entity_type: CopyEntityType, | |
| pub source_workspace: String, | |
| pub target_workspace: String, | |
| pub requested_count: usize, | |
| pub copied_count: usize, | |
| pub skipped_count: usize, | |
| pub failed_count: usize, | |
| pub results: Vec<CopyToSemanticsResult>, | |
| } | |
| pub use super::copy_to::{CopyEntityType, CopyResultStatus, RowSelectionMode}; | |
| pub type CopyToSemanticsRequest = super::copy_to::CopyToRequest; | |
| pub type CopyToSemanticsResult = super::copy_to::CopyToResult; | |
| pub type CopyToSemanticsResponse = super::copy_to::CopyToResponse; |
| &user, | ||
| source_dimension.dimension.clone(), | ||
| UpdateRequest { | ||
| position: Some(source_dimension.position), |
There was a problem hiding this comment.
When selection_mode is Selected, using the source dimension’s position for updates can fail validation in the target workspace (e.g., selecting a single dimension with a high source position while the target has fewer dimensions will trip validate_dimension_position). Consider not forcing position on overwrite in Selected mode (keep the existing target position), or remap selected dimensions to valid contiguous positions in the target (e.g., append in the same relative order).
| position: Some(source_dimension.position), | |
| position: None, |
| CreateRequest { | ||
| dimension: DimensionName::try_from(source_dimension.dimension.clone()) | ||
| .map_err(|err| bad_argument!(err))?, | ||
| position: source_dimension.position, | ||
| schema: source_dimension.schema.clone(), |
There was a problem hiding this comment.
When selection_mode is Selected, creating new dimensions with position: source_dimension.position can be invalid in the target workspace if the selection is sparse (same validation issue as the update path). Consider remapping positions for selected rows (e.g., append them in order after the current last position) or otherwise defining/validating the positioning semantics for partial copies.
| let _ = is_mandatory_dimension( | ||
| &target_workspace_context, | ||
| &updated_dimension.dimension, | ||
| ); |
There was a problem hiding this comment.
These is_mandatory_dimension(...) calls discard the return value and have no side effects, so they add noise and extra work during a copy without affecting the response. Consider removing them, or (if mandatory-ness should influence behavior) incorporate the boolean into the response/result messaging.
sauraww
force-pushed
the
copyTosemantics
branch
2 times, most recently
from
acbcfd6 to
daf5f8d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
crates/context_aware_config/src/api/dimension/operations.rs (2)
181-181: Redundantuse dimensions::dsl;(already glob-imported).
dimensions::dsl::*is glob-imported at the top of the file (line 12), sodsl::dimension,dsl::dimensions,dsl::last_modified_at, etc. used below already resolve through the glob. The localuse dimensions::dsl;only re-introduces the parent module path; you could either drop it and use the glob names directly, or drop the glob and usedsl::qualification consistently — currently both styles are mixed in this function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/dimension/operations.rs` at line 181, The file has a redundant import "use dimensions::dsl;" because "use dimensions::dsl::*" is already glob-imported; remove the extra "use dimensions::dsl;" and make the function consistently use the existing glob-imported symbols (e.g., dimension, dimensions, last_modified_at, etc.) or alternatively remove the glob and qualify all uses with dsl:: (e.g., dsl::dimension, dsl::dimensions) — pick one style and update the function that currently mixes both so references to dimension-related symbols resolve without the redundant import.
27-37: Avoid unnecessary.clone(); useas_ref()instead.
mandatory_dimensions.clone().unwrap_or_default()allocates a newVec<String>on every call solely to drop it after one.contains()check. Useas_ref().is_some_and(...)to avoid the allocation.♻️ Proposed fix
pub fn is_mandatory_dimension( workspace_context: &WorkspaceContext, dimension_name: &str, ) -> bool { workspace_context .settings .mandatory_dimensions - .clone() - .unwrap_or_default() - .contains(&dimension_name.to_string()) + .as_ref() + .is_some_and(|dims| dims.iter().any(|d| d == dimension_name)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/dimension/operations.rs` around lines 27 - 37, The function is_mandatory_dimension is allocating unnecessarily by calling settings.mandatory_dimensions.clone().unwrap_or_default() just to call contains; change it to borrow the Option instead (use settings.mandatory_dimensions.as_ref()) and check the contained Vec without cloning, e.g. use as_ref().is_some_and(|v| v.contains(&dimension_name.to_string())) or as_ref().map_or(false, |v| v.contains(&dimension_name.to_string())), so you avoid allocating a new Vec<String> on each call while still returning the correct bool.crates/superposition_types/src/api.rs (1)
3-3: Module ordering nit.
copy_tois inserted betweenauthzandconfig, breaking the alphabetical order with the rest of the list (config<copy_to). Move it afterconfigfor consistency.♻️ Proposed fix
pub mod audit_log; pub mod authz; -pub mod copy_to; pub mod config; pub mod context; +pub mod copy_to; pub mod default_config;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/api.rs` at line 3, Module ordering is out of alphabetical order: the pub mod copy_to; declaration is placed between authz and config; move the pub mod copy_to; line so it appears after the pub mod config; declaration to restore alphabetical ordering with the other modules (look for the module list in this file and relocate the copy_to module declaration accordingly).crates/superposition_types/src/api/copy_to.rs (1)
26-35: Consider validatingselected_rowsconsistency againstselection_modeat the request layer.When
selection_mode = All,selected_rowsis silently ignored; whenSelected, an emptyselected_rowsis accepted by deserialization and silently becomes a no-op. Either reject these mismatches in the handler or document the precedence. A typedWorkspacenewtype fortarget_workspace(matching how other modules treat workspace identifiers) would also strengthen the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/api/copy_to.rs` around lines 26 - 35, CopyToRequest currently allows inconsistent combinations of selection_mode and selected_rows and uses a plain String for target_workspace; validate and tighten the request: in the CopyToRequest handling/validation (or implement a constructor/try_from) enforce that when RowSelectionMode::Selected then selected_rows is non-empty (return an error when empty) and when RowSelectionMode::All then selected_rows must be empty or ignored explicitly (return an error if non-empty), and replace the plain String target_workspace with the crate's Workspace newtype (or add a Workspace field and convert/validate the incoming String) to match other modules; update any deserialization entrypoint to perform these checks and return clear validation errors.crates/context_aware_config/src/api/dimension/handlers.rs (1)
188-211: Avoid double-cloningupdate_req.
update_reqis cloned on line 197 to pass intoupdate_dimension_entry, whileupdate_change_reasonis also separately cloned on line 188. Since the outer scope only usesupdate_reqonce (inside the closure), you can move it directly and clone onlychange_reason— saving aUpdateRequestclone per request.♻️ Suggested simplification
- let update_change_reason = update_req.change_reason.clone(); + let update_change_reason = update_req.change_reason.clone(); @@ - let result = update_dimension_entry( - transaction_conn, - &workspace_context, - &user, - name.clone(), - update_req.clone(), - )?; + let result = update_dimension_entry( + transaction_conn, + &workspace_context, + &user, + name.clone(), + update_req, + )?;Note: this only works because
Connection::transactiontakesFnOnce; if the closure must becomeFnMutlater, the clone has to be re-introduced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/dimension/handlers.rs` around lines 188 - 211, The code is cloning update_req twice; instead move update_req into the transaction closure and only clone its change_reason (create update_change_reason from update_req.change_reason.clone() inside the closure) so update_dimension_entry receives the moved update_req without an extra clone; adjust references to update_change_reason so add_config_version receives the cloned change_reason and keep calls to update_dimension_entry, is_mandatory_dimension, and add_config_version unchanged otherwise.crates/frontend/src/pages/dimensions.rs (3)
300-307:prop:valueis set non-reactively here.
prop:value=change_reason_rws.get()evaluates eagerly at view construction, so the input is bound to the initial value only. It happens to "work" becauseon:inputkeeps the signal in sync with user input, but if you ever want to reset/preset the input from elsewhere (e.g. clearing after a successful copy as suggested above), the input won't update. Wrap in a closure to make it reactive:♻️ Suggested change
- prop:value=change_reason_rws.get() + prop:value=move || change_reason_rws.get()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/dimensions.rs` around lines 300 - 307, The input's value is set non-reactively via prop:value=change_reason_rws.get(), so it won't update when the signal changes elsewhere; change prop:value to a reactive closure (e.g., prop:value=move || change_reason_rws.get()) so the input's displayed value subscribes to the signal, keeping it in sync with changes made by other code; keep the existing on:input handler that calls event_target_value and change_reason_rws.set(...) intact.
192-218: Reset the copy form state on success.After a successful copy,
target_workspace_rws,change_reason_rws, andselected_dimensions_rwsretain their previous values, so an immediate second click will silently re-submit the same payload (especially risky withskip_existing=false). Consider clearing these signals (and optionallycopy_all_rwsback to its default) inside theOk(_)branch.♻️ Suggested change
match result { Ok(response) => { + target_workspace_rws.set(String::new()); + change_reason_rws.set(String::new()); + selected_dimensions_rws.set(vec![]); enqueue_alert( format!( "Copied {} dimension(s), skipped {}.", response.copied_count, response.skipped_count ), AlertType::Success, 5000, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/dimensions.rs` around lines 192 - 218, After a successful copy (inside the Ok(response) branch of the spawn_local handler), reset the relevant form signals so the UI won't re-submit the same payload: call target_workspace_rws.set(None) (or its default), change_reason_rws.set(String::new() or None as appropriate), selected_dimensions_rws.set(Vec::new()), and optionally reset copy_all_rws.set(false) and any other related signals; place these .set(...) calls immediately after the enqueue_alert in the Ok arm so the form is cleared on success.
67-148:create_memodoes not actually track any reactive dependencies here.The memo body only reads
selected_dimensions_rwsandcopy_all_rwsvia shadowed copies and embeds them insideprop:checked/prop:disabledclosures — no.get()is invoked at the top level of the memo. As a result, the memo runs once and never invalidates, so wrapping the column vec increate_memoadds overhead and signals an intent (re-derivation on selection changes) that doesn't happen. The reactivity you actually want is already implemented inside the per-row closures.Consider using a plain
let table_columns = vec and dropping the.get()at the call site (Line 381):♻️ Suggested simplification
- let table_columns = create_memo(move |_| { - let selected_dimensions_rws = selected_dimensions_rws; - let copy_all_rws = copy_all_rws; - vec![ + let table_columns = vec![ Column::new( ... ), ... - ] - }); + ];- columns=table_columns.get() + columns=table_columns.clone()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/dimensions.rs` around lines 67 - 148, The create_memo wrapper around table_columns is unnecessary because the memo body never calls .get() on selected_dimensions_rws or copy_all_rws at the top level (reactivity is inside the per-row closures), so replace the create_memo(...) that defines table_columns with a plain let table_columns = vec![ ... ] (or store_value/Rc if you need heap allocation) and update any call site that currently does table_columns.get() to use table_columns directly; key symbols to locate are create_memo, table_columns, selected_dimensions_rws, copy_all_rws, and the place where table_columns.get() is invoked.crates/context_aware_config/src/api/copy_to/handlers.rs (2)
398-413:failed_countis structurally always0; document or remove.Because the per-row creates/updates run inside a single Diesel transaction, any individual failure aborts the transaction and the handler returns
Err(...)— meaning thisCopyToResponseis only ever produced when no rows failed. Either:
- drop
failed_countfrom the response shape (and the corresponding type), or- switch to per-row error capture (push
CopyResultStatus::Failedinstead of bubbling errors out of the closure) so the field can carry meaningful information.As-is, the field is misleading to API consumers who may write logic against
failed_count > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/copy_to/handlers.rs` around lines 398 - 413, The response builder sets failed_count to 0 which is always true because per-row creates/updates run inside a single Diesel transaction that aborts on any error; update the implementation so the API shape is accurate by either (A) removing failed_count from CopyToResponse and its corresponding type/usages (drop the field and adjust callers that read failed_count), or (B) switching to per-row error capture inside the create/update loop so errors are collected into results as CopyResultStatus::Failed (stop bubbling the error out of the transaction closure, record failures in results and compute failed_count from results) — modify the code that populates results and the CopyToResponse construction accordingly (refer to CopyToResponse, failed_count, results, and CopyResultStatus::Failed).
415-441: Unnecessary clones when computing the webhook action.
CopyOperation::webhook_action(&self)only borrows, so the.clone()calls on Lines 417 and 422 are redundant. Also,actions.is_empty()is implicitly handled byconfig_version.is_some(), but theactions.len() == 1branch is fine as-is.♻️ Suggested simplification
- let webhook_action = if actions.len() == 1 { - actions[0].clone().webhook_action() - } else { - Action::Batch( - actions - .iter() - .map(|action| action.clone().webhook_action()) - .collect(), - ) - }; + let webhook_action = if actions.len() == 1 { + actions[0].webhook_action() + } else { + Action::Batch(actions.iter().map(CopyOperation::webhook_action).collect()) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/copy_to/handlers.rs` around lines 415 - 441, Remove the unnecessary .clone() calls when building webhook_action since CopyOperation::webhook_action(&self) only borrows; instead pass references or map to borrowed webhook_action values: replace actions[0].clone().webhook_action() with actions[0].webhook_action() (or &actions[0].webhook_action() as appropriate) and in the iterator drop action.clone() and call action.webhook_action() directly before collecting into Action::Batch; keep the existing branching that checks actions.len() == 1 and leave execute_webhook_call/WebhookData/config_version logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/context_aware_config/src/api/copy_to/handlers.rs`:
- Around line 279-371: The code computes initial_selected_create_position
outside the transaction and reuses source positions in RowSelectionMode::All,
which can cause TOCTOU collisions and unintended position overwrites; move the
count/load that produces initial_selected_create_position into the same
transaction closure (or compute next position as MAX(position)+1 inside the
transaction when handling Selected mode) so next_selected_create_position is
based on the transaction snapshot, and add a pre-validation step inside the
transaction before calling update_dimension_entry or create_dimension_entry to
detect positional conflicts when RowSelectionMode::All (or explicitly document
that All will replace target positions) — update references:
initial_selected_create_position, next_selected_create_position,
RowSelectionMode::All, create_dimension_entry, and update_dimension_entry.
- Around line 443-450: Add a short comment above the http_resp construction
(around the webhook_status branch and the use of StatusCode::from_u16(512) /
HttpResponse::Ok()) explaining the project convention: HTTP 512 is intentionally
used to denote "mutation committed (data persisted) but webhook/notification
delivery failed", so clients should treat 512 as a successful write with a
delivery error rather than a transactional failure. Ensure the comment mentions
that DB mutations are committed before webhook execution and that 512 signals
persisted state plus notification failure for maintainability and
cross-references to similar handlers using the same convention.
In `@crates/context_aware_config/src/api/dimension/operations.rs`:
- Around line 154-171: The ForeignKeyViolation handler in the
insert-of-dimensions block currently always blames
create_req.value_validation_function_name; update the map_err for
diesel::result::Error::DatabaseError to inspect the DB error's constraint name
(from the returned DatabaseErrorInformation) and map it to the correct column
(e.g., match constraint name for the validation function vs compute function)
before returning bad_argument!, falling back to a generic FK error message
mentioning the constraint if the name is unknown; reference the insert flow
using diesel::insert_into(dimensions::table) -> .get_result(conn) and the
create_req fields value_validation_function_name and value_compute_function_name
when implementing the mapping.
- Around line 248-296: The parking-and-reorder block uses (num_rows + 100) as
i32 which narrows i64 implicitly and never explicitly writes the final
new_position into the row (it relies on a later AsChangeset); fix by converting
defensively with i32::try_from(num_rows + 100).map_err(|_|
bad_argument!("position out of range"))? when setting the temporary parked
value, and after the shifting updates add an explicit
set(dimensions::position.eq(i32::try_from(new_position).map_err(|_|
bad_argument!("position out of range"))?)) (or otherwise convert new_position
safely) in the diesel::update that targets the moved dimension so the final
position is deterministically stored rather than relying on the trailing
AsChangeset.
In `@crates/superposition_types/src/api/copy_to.rs`:
- Around line 18-23: Add a Failed variant to the CopyResultStatus enum so
per-row failures can be represented (i.e., change enum CopyResultStatus {
Copied, Skipped } to include Failed) and keep serde(rename_all = "snake_case")
so it serializes as "failed"; update any code that constructs or pattern-matches
on CopyResultStatus (e.g., places that populate the results vector in
CopyToResponse or compute failed_count) to use CopyResultStatus::Failed and
supply the optional message when available so the results vector and
failed_count remain consistent.
In `@crates/superposition/src/main.rs`:
- Around line 259-263: The handler for /copy-to currently only authorizes the
source workspace via OrgWorkspaceMiddlewareFactory and the existing
authorize_copy() call (scoped by _auth_z), but never verifies permissions on the
target workspace; after you resolve target_workspace_context (the context built
for the workspace looked up with the source org filter) and before calling
create_dimension_entry() or update_dimension_entry(), perform an explicit
authorization check on that target context (e.g., invoke authorize_copy() or the
same authorization routine against target_workspace_context for the "create" and
"update" actions) so the caller is permitted to modify the target workspace;
update the handler logic that follows target_workspace_context acquisition to
short-circuit on failed auth and only proceed to
create_dimension_entry()/update_dimension_entry() when target authorization
succeeds.
---
Nitpick comments:
In `@crates/context_aware_config/src/api/copy_to/handlers.rs`:
- Around line 398-413: The response builder sets failed_count to 0 which is
always true because per-row creates/updates run inside a single Diesel
transaction that aborts on any error; update the implementation so the API shape
is accurate by either (A) removing failed_count from CopyToResponse and its
corresponding type/usages (drop the field and adjust callers that read
failed_count), or (B) switching to per-row error capture inside the
create/update loop so errors are collected into results as
CopyResultStatus::Failed (stop bubbling the error out of the transaction
closure, record failures in results and compute failed_count from results) —
modify the code that populates results and the CopyToResponse construction
accordingly (refer to CopyToResponse, failed_count, results, and
CopyResultStatus::Failed).
- Around line 415-441: Remove the unnecessary .clone() calls when building
webhook_action since CopyOperation::webhook_action(&self) only borrows; instead
pass references or map to borrowed webhook_action values: replace
actions[0].clone().webhook_action() with actions[0].webhook_action() (or
&actions[0].webhook_action() as appropriate) and in the iterator drop
action.clone() and call action.webhook_action() directly before collecting into
Action::Batch; keep the existing branching that checks actions.len() == 1 and
leave execute_webhook_call/WebhookData/config_version logic unchanged.
In `@crates/context_aware_config/src/api/dimension/handlers.rs`:
- Around line 188-211: The code is cloning update_req twice; instead move
update_req into the transaction closure and only clone its change_reason (create
update_change_reason from update_req.change_reason.clone() inside the closure)
so update_dimension_entry receives the moved update_req without an extra clone;
adjust references to update_change_reason so add_config_version receives the
cloned change_reason and keep calls to update_dimension_entry,
is_mandatory_dimension, and add_config_version unchanged otherwise.
In `@crates/context_aware_config/src/api/dimension/operations.rs`:
- Line 181: The file has a redundant import "use dimensions::dsl;" because "use
dimensions::dsl::*" is already glob-imported; remove the extra "use
dimensions::dsl;" and make the function consistently use the existing
glob-imported symbols (e.g., dimension, dimensions, last_modified_at, etc.) or
alternatively remove the glob and qualify all uses with dsl:: (e.g.,
dsl::dimension, dsl::dimensions) — pick one style and update the function that
currently mixes both so references to dimension-related symbols resolve without
the redundant import.
- Around line 27-37: The function is_mandatory_dimension is allocating
unnecessarily by calling
settings.mandatory_dimensions.clone().unwrap_or_default() just to call contains;
change it to borrow the Option instead (use
settings.mandatory_dimensions.as_ref()) and check the contained Vec without
cloning, e.g. use as_ref().is_some_and(|v|
v.contains(&dimension_name.to_string())) or as_ref().map_or(false, |v|
v.contains(&dimension_name.to_string())), so you avoid allocating a new
Vec<String> on each call while still returning the correct bool.
In `@crates/frontend/src/pages/dimensions.rs`:
- Around line 300-307: The input's value is set non-reactively via
prop:value=change_reason_rws.get(), so it won't update when the signal changes
elsewhere; change prop:value to a reactive closure (e.g., prop:value=move ||
change_reason_rws.get()) so the input's displayed value subscribes to the
signal, keeping it in sync with changes made by other code; keep the existing
on:input handler that calls event_target_value and change_reason_rws.set(...)
intact.
- Around line 192-218: After a successful copy (inside the Ok(response) branch
of the spawn_local handler), reset the relevant form signals so the UI won't
re-submit the same payload: call target_workspace_rws.set(None) (or its
default), change_reason_rws.set(String::new() or None as appropriate),
selected_dimensions_rws.set(Vec::new()), and optionally reset
copy_all_rws.set(false) and any other related signals; place these .set(...)
calls immediately after the enqueue_alert in the Ok arm so the form is cleared
on success.
- Around line 67-148: The create_memo wrapper around table_columns is
unnecessary because the memo body never calls .get() on selected_dimensions_rws
or copy_all_rws at the top level (reactivity is inside the per-row closures), so
replace the create_memo(...) that defines table_columns with a plain let
table_columns = vec![ ... ] (or store_value/Rc if you need heap allocation) and
update any call site that currently does table_columns.get() to use
table_columns directly; key symbols to locate are create_memo, table_columns,
selected_dimensions_rws, copy_all_rws, and the place where table_columns.get()
is invoked.
In `@crates/superposition_types/src/api.rs`:
- Line 3: Module ordering is out of alphabetical order: the pub mod copy_to;
declaration is placed between authz and config; move the pub mod copy_to; line
so it appears after the pub mod config; declaration to restore alphabetical
ordering with the other modules (look for the module list in this file and
relocate the copy_to module declaration accordingly).
In `@crates/superposition_types/src/api/copy_to.rs`:
- Around line 26-35: CopyToRequest currently allows inconsistent combinations of
selection_mode and selected_rows and uses a plain String for target_workspace;
validate and tighten the request: in the CopyToRequest handling/validation (or
implement a constructor/try_from) enforce that when RowSelectionMode::Selected
then selected_rows is non-empty (return an error when empty) and when
RowSelectionMode::All then selected_rows must be empty or ignored explicitly
(return an error if non-empty), and replace the plain String target_workspace
with the crate's Workspace newtype (or add a Workspace field and
convert/validate the incoming String) to match other modules; update any
deserialization entrypoint to perform these checks and return clear validation
errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81328f10-0017-46ce-8aaf-71a79ce23c51
📒 Files selected for processing (11)
crates/context_aware_config/src/api.rscrates/context_aware_config/src/api/copy_to.rscrates/context_aware_config/src/api/copy_to/handlers.rscrates/context_aware_config/src/api/dimension.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/dimension/operations.rscrates/frontend/src/api.rscrates/frontend/src/pages/dimensions.rscrates/superposition/src/main.rscrates/superposition_types/src/api.rscrates/superposition_types/src/api/copy_to.rs
| #[authorized(action = "update")] | ||
| #[post("")] | ||
| async fn copy_to_handler( | ||
| workspace_context: WorkspaceContext, | ||
| state: Data<AppState>, | ||
| custom_headers: CustomHeaders, | ||
| req: Json<CopyToRequest>, | ||
| user: User, | ||
| db_conn: DbConnection, | ||
| ) -> superposition::Result<HttpResponse> { | ||
| let DbConnection(mut conn) = db_conn; | ||
| let req = req.into_inner(); | ||
|
|
||
| if req.entity_type != CopyEntityType::Dimensions { | ||
| return Err(bad_argument!("Unsupported entity_type")); | ||
| } | ||
|
|
||
| if req.target_workspace == workspace_context.workspace_id.0 { | ||
| return Err(bad_argument!( | ||
| "Source and target workspaces must be different" | ||
| )); | ||
| } | ||
|
|
||
| let selected_rows = dedupe_selected_rows(req.selected_rows); | ||
| if matches!(req.selection_mode, RowSelectionMode::Selected) | ||
| && selected_rows.is_empty() | ||
| { | ||
| return Err(bad_argument!( | ||
| "selected_rows must be provided when selection_mode is selected" | ||
| )); | ||
| } | ||
|
|
||
| let source_dimensions = get_source_dimensions( | ||
| &mut conn, | ||
| &workspace_context, | ||
| &req.selection_mode, | ||
| &selected_rows, | ||
| )?; | ||
| let target_workspace_context = get_target_workspace_context( | ||
| &mut conn, | ||
| &workspace_context, | ||
| &req.target_workspace, | ||
| )?; | ||
|
|
||
| validate_change_reason( | ||
| &target_workspace_context, | ||
| &req.change_reason, | ||
| &mut conn, | ||
| &state.master_encryption_key, | ||
| ) | ||
| .await?; | ||
|
|
||
| let target_dimension_names = dimensions::dsl::dimensions | ||
| .select(dimensions::dsl::dimension) | ||
| .schema_name(&target_workspace_context.schema_name) | ||
| .load::<String>(&mut conn) | ||
| .map_err(|err| { | ||
| log::error!("failed to fetch target dimension names with error: {err}"); | ||
| db_error!(err) | ||
| })? | ||
| .into_iter() | ||
| .collect::<HashSet<_>>(); | ||
|
|
||
| authorize_copy( | ||
| &_auth_z, | ||
| &source_dimensions, | ||
| &target_dimension_names, | ||
| req.skip_existing, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the `authorized` proc-macro to confirm what scope `_auth_z` resolves against.
fd -t f 'authorized' crates/superposition_derives crates/superposition_macros 2>/dev/null
rg -nP -C5 'fn\s+authorized|#\[proc_macro' crates/superposition_derives crates/superposition_macros
# Inspect AuthZ::action_authorized to see how it determines scope (workspace_context vs explicit arg).
ast-grep --pattern $'impl$_ AuthZ$_ {
$$$
pub async fn action_authorized($$$) $$$ {
$$$
}
$$$
}'
rg -nP -C8 'action_authorized' --type=rustRepository: juspay/superposition
Length of output: 30545
🏁 Script executed:
sed -n '442,560p' crates/superposition_derives/src/lib.rsRepository: juspay/superposition
Length of output: 4145
🏁 Script executed:
# Also search for how AuthZ is created from workspace_context
rg -A 10 "impl.*FromRequest.*for.*AuthZ" --type=rustRepository: juspay/superposition
Length of output: 2969
🏁 Script executed:
# Look for any explicit auth scope handling in copy_to handlers or similar cross-workspace operations
rg -B 5 -A 10 "target_workspace_context" crates/context_aware_config/src/api/copy_to/handlers.rs | head -100Repository: juspay/superposition
Length of output: 3456
🏁 Script executed:
# Find where AuthZHandler is set in request extensions
rg -B 10 -A 10 "extensions.*insert.*AuthZHandler" --type=rust | head -80Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look at the AuthZHandler struct and how it's created
rg -B 5 -A 15 "struct AuthZHandler" --type=rustRepository: juspay/superposition
Length of output: 1657
🏁 Script executed:
# Search for middleware that sets up the auth_z in request extensions
fd -type f -name "*.rs" | xargs rg -l "AuthZHandler\|AuthZMiddleware" | head -10Repository: juspay/superposition
Length of output: 235
🏁 Script executed:
# Find middleware setup and where workspace_context is passed to auth handlers
rg -B 5 -A 15 "CasbinPolicyEngine\|impl.*Authorizer" --type=rust | head -150Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for how workspace context flows into authorization checks
rg -B 5 -A 10 "enforce.*workspace\|workspace.*schema" crates/service_utils/src/middlewares/auth_z.rs | head -100Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there's domain/workspace scoping in the casbin enforce call
rg -B 3 -A 8 "\.enforce\(" --type=rust | head -100Repository: juspay/superposition
Length of output: 1119
🏁 Script executed:
# Look at the full action_authorized implementation and how domain is captured
sed -n '55,82p' crates/service_utils/src/middlewares/auth_z.rsRepository: juspay/superposition
Length of output: 771
🏁 Script executed:
# Find where domain comes from - look at AuthZ::new or struct initialization
rg -B 10 -A 5 "pub fn new.*domain\|fn from_request" crates/service_utils/src/middlewares/auth_z.rs | head -80Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check what domain and workspace_context are and how they relate
rg -B 2 "domain\:" crates/service_utils/src/middlewares/auth_z.rs | head -50Repository: juspay/superposition
Length of output: 145
🏁 Script executed:
# Look at the full AuthZ struct definition
rg -B 5 -A 20 "pub struct AuthZ" crates/service_utils/src/middlewares/auth_z.rsRepository: juspay/superposition
Length of output: 2753
🏁 Script executed:
# Find where AuthZ is initialized in FromRequest
rg -A 50 "impl.*FromRequest.*for.*AuthZ<A>" crates/service_utils/src/middlewares/auth_z.rs | head -80Repository: juspay/superposition
Length of output: 1911
🏁 Script executed:
# Check how domain/workspace_context flows into the AuthZ creation
rg "WorkspaceContext\|AuthZDomain\|domain =" crates/service_utils/src/middlewares/auth_z.rs | head -30Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Find where AuthZDomain is inserted into request extensions
rg -B 10 -A 5 "extensions.*insert.*AuthZDomain\|AuthZDomain.*insert" --type=rustRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for middleware that sets up AuthZDomain based on workspace
rg -B 10 -A 10 "schema_name\|workspace.*domain" crates/service_utils/src/middlewares/ --type=rust | head -120Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check how workspace_context relates to AuthZDomain in actual handlers
rg -B 3 "workspace_context\|WorkspaceContext" crates/context_aware_config/src/api/copy_to/handlers.rs | head -50Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Find all files that insert things into request extensions
rg "extensions_mut.*insert\|extensions.*insert" --type=rust | head -50Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look at copy_to module to see the full context and what gets passed
rg -B 20 "async fn copy_to_handler" crates/context_aware_config/src/api/copy_to/handlers.rs | head -80Repository: juspay/superposition
Length of output: 556
🏁 Script executed:
# Search for where workspace schema is set as domain in authorization
rg "schema_name.*AuthZDomain\|AuthZDomain.*schema" --type=rustRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Search more broadly for where AuthZDomain is created
rg "AuthZDomain" --type=rust | grep -v "test\|impl\|struct\|pub fn\|use"Repository: juspay/superposition
Length of output: 2395
🏁 Script executed:
# Look at the router/app setup to see how middleware is configured
fd -type f -name "*.rs" -path "*/src/main.rs" -o -path "*/src/lib.rs" | xargs cat | head -300Repository: juspay/superposition
Length of output: 235
🏁 Script executed:
# Search for where schema_name is extracted and used
rg "schema_name.*extensions\|insert.*schema" --type=rust | head -50Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look at WorkspaceContext to understand what it contains
rg -B 3 -A 15 "pub struct WorkspaceContext\|pub struct.*Context.*workspace" --type=rust | head -100Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Found it! Check the workspace_context middleware that creates AuthZDomain
cat crates/service_utils/src/middlewares/workspace_context.rs | head -200Repository: juspay/superposition
Length of output: 6798
🏁 Script executed:
# Verify that AuthZDomain is created from source workspace schema
rg -B 5 -A 5 "schema_name.*AuthZDomain\|AuthZDomain.*new" crates/service_utils/src/middlewares/workspace_context.rsRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Show the full authorize_copy function to confirm it uses the source-scoped _auth_z
sed -n '175,204p' crates/context_aware_config/src/api/copy_to/handlers.rsRepository: juspay/superposition
Length of output: 931
🏁 Script executed:
# Check if there's any mechanism to create a new AuthZ for target workspace in the handler
rg -A 30 "fn copy_to_handler" crates/context_aware_config/src/api/copy_to/handlers.rs | grep -i "auth\|target"Repository: juspay/superposition
Length of output: 191
🏁 Script executed:
# Final verification: check if there's any code that creates or uses auth_z for target workspace
rg "target_workspace.*auth\|AuthZ.*target" crates/context_aware_config/src/api/copy_to/handlers.rsRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Verify the complete flow: what happens after authorize_copy - do actual writes check auth again?
sed -n '270,350p' crates/context_aware_config/src/api/copy_to/handlers.rsRepository: juspay/superposition
Length of output: 3785
Authorization is checked against the source workspace, not the target where writes happen.
The #[authorized(action = "update")] macro injects _auth_z parameterized by the request's workspace context — i.e., the source workspace. The middleware (workspace_context.rs) creates AuthZDomain from the source workspace schema and stores it in request extensions. All permission checks in authorize_copy (line 271) call _auth_z.action_authorized(...) against this source domain. However, the actual mutations (update_dimension_entry, create_dimension_entry, add_config_version) execute against target_workspace_context (lines 309–321, 338–356, 376–382).
This means a principal with update and per-dimension create/update on the source workspace can modify dimensions on any target workspace in the same organization, even without permissions there. The endpoint lacks authorization enforcement on the target resource scope.
Rework authorize_copy to enforce permissions against the target workspace's schema/domain, and reconsider whether the top-level #[authorized(action = "update")] gate is appropriate for an operation that creates/modifies resources on a different workspace.
| let initial_selected_create_position = u32::try_from(target_dimension_names.len()) | ||
| .map_err(|_| { | ||
| bad_argument!("Too many target dimensions to append selected rows") | ||
| })?; | ||
| let (results, actions, config_version) = conn | ||
| .transaction::<_, superposition::AppError, _>(|transaction_conn| { | ||
| let mut results = Vec::new(); | ||
| let mut actions = Vec::new(); | ||
| let mut next_selected_create_position = initial_selected_create_position; | ||
|
|
||
| for source_dimension in &source_dimensions { | ||
| if target_dimension_names.contains(&source_dimension.dimension) { | ||
| if req.skip_existing { | ||
| results.push(CopyToResult { | ||
| row_identifier: source_dimension.dimension.clone(), | ||
| status: CopyResultStatus::Skipped, | ||
| message: Some(String::from( | ||
| "Dimension already exists in target workspace", | ||
| )), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| update_dimension_entry( | ||
| transaction_conn, | ||
| &target_workspace_context, | ||
| &user, | ||
| source_dimension.dimension.clone(), | ||
| UpdateRequest { | ||
| position: match req.selection_mode { | ||
| RowSelectionMode::All => Some(source_dimension.position), | ||
| RowSelectionMode::Selected => None, | ||
| }, | ||
| schema: Some(source_dimension.schema.clone()), | ||
| value_validation_function_name: Some( | ||
| source_dimension.value_validation_function_name.clone(), | ||
| ), | ||
| value_compute_function_name: Some( | ||
| source_dimension.value_compute_function_name.clone(), | ||
| ), | ||
| description: Some(source_dimension.description.clone()), | ||
| change_reason: req.change_reason.clone(), | ||
| }, | ||
| )?; | ||
| actions.push(CopyOperation::Update); | ||
| results.push(CopyToResult { | ||
| row_identifier: source_dimension.dimension.clone(), | ||
| status: CopyResultStatus::Copied, | ||
| message: Some(String::from( | ||
| "Dimension updated in target workspace", | ||
| )), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| let create_position = match req.selection_mode { | ||
| RowSelectionMode::All => source_dimension.position, | ||
| RowSelectionMode::Selected => { | ||
| let append_position = next_selected_create_position.into(); | ||
| next_selected_create_position += 1; | ||
| append_position | ||
| } | ||
| }; | ||
|
|
||
| create_dimension_entry( | ||
| transaction_conn, | ||
| &target_workspace_context, | ||
| &user, | ||
| CreateRequest { | ||
| dimension: DimensionName::try_from( | ||
| source_dimension.dimension.clone(), | ||
| ) | ||
| .map_err(|err| bad_argument!(err))?, | ||
| position: create_position, | ||
| schema: source_dimension.schema.clone(), | ||
| value_validation_function_name: source_dimension | ||
| .value_validation_function_name | ||
| .clone(), | ||
| description: source_dimension.description.clone(), | ||
| change_reason: req.change_reason.clone(), | ||
| value_compute_function_name: source_dimension | ||
| .value_compute_function_name | ||
| .clone(), | ||
| dimension_type: source_dimension.dimension_type.clone(), | ||
| }, | ||
| )?; | ||
| actions.push(CopyOperation::Create); | ||
| results.push(CopyToResult { | ||
| row_identifier: source_dimension.dimension.clone(), | ||
| status: CopyResultStatus::Copied, | ||
| message: Some(String::from("Dimension created in target workspace")), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Position handling in RowSelectionMode::All can collide with existing target dimensions, and the initial_selected_create_position count is computed outside the transaction.
Two related concerns in this block:
-
Position collisions (All mode creates). For
RowSelectionMode::All,create_position = source_dimension.position(Line 335) reuses the source's position verbatim. If the target already contains other dimensions (not present in source) occupying the same position, the create will either fail invalidate_dimension_positionor shift target dimensions in surprising ways. The same applies to updates at Line 309 whereposition = Some(source_dimension.position). Worth either documenting this as "All mode replaces target positions" or pre-validating positional layout against the target before entering the transaction so users get a clean error rather than a partial transaction abort. -
TOCTOU on
initial_selected_create_position. It's computed at Lines 259–268 / 279–282 from aloadoutside the transaction, then used as the starting append position inside the transaction at Line 287. A concurrent insert into the target schema'sdimensionsbetween these two points can cause a position collision when the first Selected-mode create runs. Move thecount()/load inside the transaction (or, better, compute the next position viaMAX(position) + 1at create time) so it shares the transaction's snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/copy_to/handlers.rs` around lines 279 -
371, The code computes initial_selected_create_position outside the transaction
and reuses source positions in RowSelectionMode::All, which can cause TOCTOU
collisions and unintended position overwrites; move the count/load that produces
initial_selected_create_position into the same transaction closure (or compute
next position as MAX(position)+1 inside the transaction when handling Selected
mode) so next_selected_create_position is based on the transaction snapshot, and
add a pre-validation step inside the transaction before calling
update_dimension_entry or create_dimension_entry to detect positional conflicts
when RowSelectionMode::All (or explicitly document that All will replace target
positions) — update references: initial_selected_create_position,
next_selected_create_position, RowSelectionMode::All, create_dimension_entry,
and update_dimension_entry.
| let mut http_resp = if webhook_status { | ||
| HttpResponse::Ok() | ||
| } else { | ||
| HttpResponse::build( | ||
| actix_web::http::StatusCode::from_u16(512) | ||
| .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other handlers using 512 to validate it's an established convention here.
rg -nP -C3 'StatusCode::from_u16\(\s*512\s*\)|status\(\s*512\s*\)' --type=rustRepository: juspay/superposition
Length of output: 13618
🏁 Script executed:
# Search for comments or documentation about HTTP 512 or webhook failures
rg -i "512|webhook.*fail|mutation.*persist|transact" -B2 -A2 crates/context_aware_config/src/api/copy_to/handlers.rs | head -100Repository: juspay/superposition
Length of output: 1012
🏁 Script executed:
# Check if there's any documentation in README or other docs
find . -name "*.md" -o -name "*.txt" | xargs grep -l "512\|webhook" 2>/dev/null | head -10Repository: juspay/superposition
Length of output: 543
🏁 Script executed:
# Look at the full context of the copy_to handler to see if mutations are committed before the response
rg -B20 "StatusCode::from_u16\(512\)" crates/context_aware_config/src/api/copy_to/handlers.rs | grep -A20 "async fn" | head -50Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for documentation about this 512 convention in the codebase or docs
find . -type f \( -name "*.md" -o -name "*.rs" \) -exec grep -l "512\|unassigned.*status\|custom.*status" {} \; | head -20Repository: juspay/superposition
Length of output: 783
🏁 Script executed:
# Check if there's a comment or documentation in the actual handlers about this
rg "512" -B5 crates/context_aware_config/src/api/copy_to/handlers.rs | head -80Repository: juspay/superposition
Length of output: 233
🏁 Script executed:
# Look at the broader transaction context for copy_to to confirm mutations are committed
sed -n '400,460p' crates/context_aware_config/src/api/copy_to/handlers.rsRepository: juspay/superposition
Length of output: 1926
Add documentation explaining HTTP 512 convention for webhook failures.
The codebase consistently uses HTTP 512 (an unassigned status code) to signal that mutations have succeeded but webhook notification failed. While this convention is established across 25+ usages in dimension, default_config, context, and other handlers, it lacks explicit documentation. Since database mutations are committed within the transaction before webhook execution occurs, a 512 response correctly indicates the server-side operation succeeded. Add a comment explaining this semantics so clients and future maintainers understand that 512 means "data persisted but notification delivery failed."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/copy_to/handlers.rs` around lines 443 -
450, Add a short comment above the http_resp construction (around the
webhook_status branch and the use of StatusCode::from_u16(512) /
HttpResponse::Ok()) explaining the project convention: HTTP 512 is intentionally
used to denote "mutation committed (data persisted) but webhook/notification
delivery failed", so clients should treat 512 as a successful write with a
delivery error rather than a transactional failure. Ensure the comment mentions
that DB mutations are committed before webhook execution and that 512 signals
persisted state plus notification failure for maintainability and
cross-references to similar handlers using the same convention.
| diesel::insert_into(dimensions::table) | ||
| .values(&dimension_data) | ||
| .returning(Dimension::as_returning()) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .get_result(conn) | ||
| .map_err(|err| match err { | ||
| diesel::result::Error::DatabaseError( | ||
| diesel::result::DatabaseErrorKind::ForeignKeyViolation, | ||
| _, | ||
| ) => bad_argument!( | ||
| "Function {} doesn't exists", | ||
| create_req.value_validation_function_name.unwrap_or_default() | ||
| ), | ||
| _ => { | ||
| log::error!("Dimension create failed with error: {err}"); | ||
| db_error!(err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Misleading FK violation error message.
The mapping unconditionally blames value_validation_function_name, but a ForeignKeyViolation here can also stem from value_compute_function_name (or any other future FK on the dimensions table). Either inspect the error's constraint name to choose the right field, or rephrase to a generic message that doesn't pin blame on one column.
♻️ Suggested fix
- ) => bad_argument!(
- "Function {} doesn't exists",
- create_req.value_validation_function_name.unwrap_or_default()
- ),
+ ) => bad_argument!(
+ "Referenced function does not exist (validation_fn={:?}, compute_fn={:?})",
+ create_req.value_validation_function_name,
+ create_req.value_compute_function_name,
+ ),📝 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.
| diesel::insert_into(dimensions::table) | |
| .values(&dimension_data) | |
| .returning(Dimension::as_returning()) | |
| .schema_name(&workspace_context.schema_name) | |
| .get_result(conn) | |
| .map_err(|err| match err { | |
| diesel::result::Error::DatabaseError( | |
| diesel::result::DatabaseErrorKind::ForeignKeyViolation, | |
| _, | |
| ) => bad_argument!( | |
| "Function {} doesn't exists", | |
| create_req.value_validation_function_name.unwrap_or_default() | |
| ), | |
| _ => { | |
| log::error!("Dimension create failed with error: {err}"); | |
| db_error!(err) | |
| } | |
| }) | |
| diesel::insert_into(dimensions::table) | |
| .values(&dimension_data) | |
| .returning(Dimension::as_returning()) | |
| .schema_name(&workspace_context.schema_name) | |
| .get_result(conn) | |
| .map_err(|err| match err { | |
| diesel::result::Error::DatabaseError( | |
| diesel::result::DatabaseErrorKind::ForeignKeyViolation, | |
| _, | |
| ) => bad_argument!( | |
| "Referenced function does not exist (validation_fn={:?}, compute_fn={:?})", | |
| create_req.value_validation_function_name, | |
| create_req.value_compute_function_name, | |
| ), | |
| _ => { | |
| log::error!("Dimension create failed with error: {err}"); | |
| db_error!(err) | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/dimension/operations.rs` around lines 154
- 171, The ForeignKeyViolation handler in the insert-of-dimensions block
currently always blames create_req.value_validation_function_name; update the
map_err for diesel::result::Error::DatabaseError to inspect the DB error's
constraint name (from the returned DatabaseErrorInformation) and map it to the
correct column (e.g., match constraint name for the validation function vs
compute function) before returning bad_argument!, falling back to a generic FK
error message mentioning the constraint if the name is unknown; reference the
insert flow using diesel::insert_into(dimensions::table) -> .get_result(conn)
and the create_req fields value_validation_function_name and
value_compute_function_name when implementing the mapping.
| if let Some(position_val) = update_req.position { | ||
| let new_position = position_val; | ||
| validate_dimension_position( | ||
| DimensionName::try_from(name.clone()).map_err(|err| bad_argument!(err))?, | ||
| position_val, | ||
| num_rows - 1, | ||
| )?; | ||
| validate_position_wrt_dependency( | ||
| &name, | ||
| &position_val, | ||
| conn, | ||
| &workspace_context.schema_name, | ||
| )?; | ||
| let previous_position = dimension_data.position; | ||
|
|
||
| diesel::update(dimensions) | ||
| .filter(dsl::dimension.eq(&name)) | ||
| .set(( | ||
| dsl::last_modified_at.eq(Utc::now()), | ||
| dsl::last_modified_by.eq(user.get_email()), | ||
| dimensions::position.eq((num_rows + 100) as i32), | ||
| )) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .execute(conn)?; | ||
|
|
||
| if previous_position < new_position { | ||
| diesel::update(dsl::dimensions) | ||
| .filter(dimensions::position.gt(previous_position)) | ||
| .filter(dimensions::position.le(&new_position)) | ||
| .set(( | ||
| dsl::last_modified_at.eq(Utc::now()), | ||
| dsl::last_modified_by.eq(user.get_email()), | ||
| dimensions::position.eq(dimensions::position - 1), | ||
| )) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .execute(conn)?; | ||
| } else { | ||
| diesel::update(dsl::dimensions) | ||
| .filter(dimensions::position.lt(previous_position)) | ||
| .filter(dimensions::position.ge(&new_position)) | ||
| .set(( | ||
| dsl::last_modified_at.eq(Utc::now()), | ||
| dsl::last_modified_by.eq(user.get_email()), | ||
| dimensions::position.eq(dimensions::position + 1), | ||
| )) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .execute(conn)?; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm UpdateRequest derives AsChangeset and includes a `position` column mapping.
fd -e rs | xargs rg -nP -C3 '\bstruct\s+UpdateRequest\b'
ast-grep --pattern $'#[derive($$$AsChangeset$$$)]
$$$
pub struct UpdateRequest {
$$$
}'Repository: juspay/superposition
Length of output: 1947
🏁 Script executed:
cat -n crates/context_aware_config/src/api/dimension/operations.rs | sed -n '240,320p'Repository: juspay/superposition
Length of output: 3158
🏁 Script executed:
cat -n crates/context_aware_config/src/api/dimension/operations.rs | sed -n '180,250p'Repository: juspay/superposition
Length of output: 3195
Position-reorder logic: confirm bounds and final position assignment.
Two things worth double-checking here:
- The dimension is parked at
(num_rows + 100) as i32. Sincenum_rowsisi64(from line 191), this is an implicit narrowing cast; an expliciti32::try_from(num_rows + 100)or guarding would be more defensive. - The dimension's final position (
new_position) is never explicitly set inside thisif let Some(position_val)block—it's applied via the trailingupdate_reqat lines 298–308 throughAsChangeset. WhileUpdateRequestdoes correctly include apositionfield and derivesAsChangeset, the code flow is implicit and relies on this mechanism. Consider adding an explicitset(dimensions::position.eq(new_position))here for clarity and safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/dimension/operations.rs` around lines 248
- 296, The parking-and-reorder block uses (num_rows + 100) as i32 which narrows
i64 implicitly and never explicitly writes the final new_position into the row
(it relies on a later AsChangeset); fix by converting defensively with
i32::try_from(num_rows + 100).map_err(|_| bad_argument!("position out of
range"))? when setting the temporary parked value, and after the shifting
updates add an explicit
set(dimensions::position.eq(i32::try_from(new_position).map_err(|_|
bad_argument!("position out of range"))?)) (or otherwise convert new_position
safely) in the diesel::update that targets the moved dimension so the final
position is deterministically stored rather than relying on the trailing
AsChangeset.
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum CopyResultStatus { | ||
| Copied, | ||
| Skipped, | ||
| } |
There was a problem hiding this comment.
CopyResultStatus is missing a Failed variant despite failed_count field.
CopyToResponse exposes failed_count (line 52), but CopyResultStatus only has Copied and Skipped. There is no way to surface a per-row failure in the results vector with an accurate status, so callers either lose the failure granularity or have to misuse Skipped. Consider adding a Failed variant so per-row outcomes (and the optional message) can faithfully reflect the failed_count.
♻️ Proposed fix
pub enum CopyResultStatus {
Copied,
Skipped,
+ Failed,
}Also applies to: 44-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_types/src/api/copy_to.rs` around lines 18 - 23, Add a
Failed variant to the CopyResultStatus enum so per-row failures can be
represented (i.e., change enum CopyResultStatus { Copied, Skipped } to include
Failed) and keep serde(rename_all = "snake_case") so it serializes as "failed";
update any code that constructs or pattern-matches on CopyResultStatus (e.g.,
places that populate the results vector in CopyToResponse or compute
failed_count) to use CopyResultStatus::Failed and supply the optional message
when available so the results vector and failed_count remain consistent.
| .service( | ||
| scope("/copy-to") | ||
| .wrap(OrgWorkspaceMiddlewareFactory::new(true, true)) | ||
| .service(copy_to::endpoints()), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i copy_to -e rs | xargs rg -nP -C3 'target_workspace|authoriz|authz|check_permission'Repository: juspay/superposition
Length of output: 1223
🏁 Script executed:
fd -i copy_to -e rs -type fRepository: juspay/superposition
Length of output: 235
🏁 Script executed:
fd copy_to -e rsRepository: juspay/superposition
Length of output: 157
🏁 Script executed:
cat -n crates/context_aware_config/src/api/copy_to.rsRepository: juspay/superposition
Length of output: 129
🏁 Script executed:
fd handlers crates/context_aware_config/src/api/ -type fRepository: juspay/superposition
Length of output: 235
🏁 Script executed:
ls -la crates/context_aware_config/src/api/Repository: juspay/superposition
Length of output: 1388
🏁 Script executed:
ls -la crates/context_aware_config/src/api/copy_to/Repository: juspay/superposition
Length of output: 243
🏁 Script executed:
cat -n crates/context_aware_config/src/api/copy_to/handlers.rsRepository: juspay/superposition
Length of output: 18579
🏁 Script executed:
rg -n "authorized\(" crates/superposition_derives -A 5 -B 2Repository: juspay/superposition
Length of output: 2466
🏁 Script executed:
sed -n '443,550p' crates/superposition_derives/src/lib.rsRepository: juspay/superposition
Length of output: 3781
🏁 Script executed:
rg -n "fn create_dimension_entry|fn update_dimension_entry" crates/context_aware_config/src/api/dimension/Repository: juspay/superposition
Length of output: 253
🏁 Script executed:
sed -n '39,120p' crates/context_aware_config/src/api/dimension/operations.rsRepository: juspay/superposition
Length of output: 2966
🏁 Script executed:
sed -n '174,250p' crates/context_aware_config/src/api/dimension/operations.rsRepository: juspay/superposition
Length of output: 2866
Target-workspace authorization is not explicitly checked.
Mounting /copy-to under OrgWorkspaceMiddlewareFactory::new(true, true) correctly authorizes the source (path-bound) workspace. The organization check for target_workspace is explicit at lines 91-109—the target workspace is queried with a filter matching the source workspace's organization. However, the handler does not verify that the caller has permissions on the target workspace before modifying it.
The authorize_copy() call at line 270 checks "create" and "update" actions, but this is scoped to the source workspace context (via _auth_z injected by #[authorized(action = "update")]). The subsequent calls to create_dimension_entry() and update_dimension_entry() assume the caller is already authorized for the target workspace, but no such check occurs. Add an explicit authorization check on target_workspace_context to verify the user has the necessary permissions before proceeding with copy operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition/src/main.rs` around lines 259 - 263, The handler for
/copy-to currently only authorizes the source workspace via
OrgWorkspaceMiddlewareFactory and the existing authorize_copy() call (scoped by
_auth_z), but never verifies permissions on the target workspace; after you
resolve target_workspace_context (the context built for the workspace looked up
with the source org filter) and before calling create_dimension_entry() or
update_dimension_entry(), perform an explicit authorization check on that target
context (e.g., invoke authorize_copy() or the same authorization routine against
target_workspace_context for the "create" and "update" actions) so the caller is
permitted to modify the target workspace; update the handler logic that follows
target_workspace_context acquisition to short-circuit on failed auth and only
proceed to create_dimension_entry()/update_dimension_entry() when target
authorization succeeds.
daf5f8d to
0559e75
Compare
sauraww
force-pushed
the
copyTosemantics
branch
from
0559e75 to
d5cd98d
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit