fix: workspace settings usage for change reason validation #814
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors validation logic to make change reason validation unconditional across the codebase rather than gated by a feature flag. It centralizes workspace helper functionality by moving Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/frontend/src/components/workspace_form.rs (1)
52-55: Critical bug: Signal values are swapped.The signal initializations have the variable names swapped:
enable_context_validation_rsis initialized withenable_change_reason_validationenable_change_reason_validation_rsis initialized withenable_context_validationThis will cause the UI toggles to save the opposite values to what the user selected.
🔎 Proposed fix
let (enable_context_validation_rs, enable_context_validation_ws) = - create_signal(enable_change_reason_validation); + create_signal(enable_context_validation); let (enable_change_reason_validation_rs, enable_change_reason_validation_ws) = - create_signal(enable_context_validation); + create_signal(enable_change_reason_validation);
🧹 Nitpick comments (4)
crates/context_aware_config/src/helpers.rs (1)
260-270: Consider using&SchemaNamefor type consistency.The
get_workspacefunction accepts&Stringwhile the similar function incrates/experimentation_platform/src/api/experiments/helpers.rs(lines 43-51) accepts&SchemaName. This inconsistency may cause confusion or require unnecessary type conversions at call sites. Sincevalidate_change_reasonalready receives&SchemaName, consider aligning the parameter type.🔎 Suggested change
pub fn get_workspace( - workspace_schema_name: &String, + workspace_schema_name: &SchemaName, db_conn: &mut DBConnection, ) -> superposition::Result<Workspace> { let workspace = workspaces::dsl::workspaces - .filter(workspaces::workspace_schema_name.eq(workspace_schema_name)) + .filter(workspaces::workspace_schema_name.eq(workspace_schema_name.to_string())) .get_result::<Workspace>(db_conn)?; Ok(workspace) }crates/context_aware_config/src/api/context/handlers.rs (1)
217-224: Inconsistent description handling compared toput_handler.In
put_handler(lines 82-101), when description is absent and the context doesn't exist, abad_argumenterror is returned. Here inmove_handler, thequery_descriptionerror propagates directly via?, which could expose internalNotFounderrors to the client instead of a user-friendly message.Consider aligning the error handling with
put_handlerfor consistency.🔎 Suggested change
let description = match req.description.clone() { Some(val) => val, - None => query_description( - Value::Object(req.context.clone().into_inner().into()), - &mut db_conn, - &schema_name, - )?, + None => { + let resp = query_description( + Value::Object(req.context.clone().into_inner().into()), + &mut db_conn, + &schema_name, + ); + match resp { + Err(AppError::DbError(diesel::result::Error::NotFound)) => { + return Err(bad_argument!( + "Description is required when context does not exist" + )); + } + Ok(desc) => desc, + Err(e) => return Err(e), + } + } };crates/experimentation_platform/src/api/experiments/handlers.rs (2)
1175-1183: Consider removing the duplicateget_workspacecall.The workspace settings are fetched at line 1175 for validation, then fetched again at line 1196 (shadowing the first variable). This results in an unnecessary database query and potential confusion from variable shadowing. Recommend removing the second call at line 1196 and reusing the workspace settings fetched at line 1175 throughout the handler.
🔎 Suggested refactor
Remove the duplicate fetch at line 1196:
let workspace_settings = get_workspace(&workspace_request.schema_name, &mut conn)?; fetch_and_validate_change_reason_with_function( &workspace_settings, &change_reason, &state, &workspace_request, ) .await?; let experiment: Experiment = experiments::experiments .find(exp_id) .schema_name(&workspace_request.schema_name) .get_result::<Experiment>(&mut conn)?; if !experiment.status.active() { return Err(bad_argument!( "Experiment is not active, cannot ramp a concluded experiment" )); } - let workspace_settings = get_workspace(&workspace_request.schema_name, &mut conn)?; - if !user_allowed_to_ramp( &experiment, &user, workspace_settings.allow_experiment_self_approval,Also applies to: 1196-1196
1372-1380: Consider removing the duplicateget_workspacecall.Similar to the
ramphandler, workspace settings are fetched twice: once at line 1372 for validation and again at line 1421 for later checks. This creates an unnecessary database query and variable shadowing. Recommend removing the second call at line 1421 and reusing the workspace settings from line 1372.🔎 Suggested refactor
Remove the duplicate fetch at line 1421:
let workspace_settings = get_workspace(&workspace_request.schema_name, &mut conn)?; fetch_and_validate_change_reason_with_function( &workspace_settings, &change_reason, &state, &workspace_request, ) .await?; let payload = req.into_inner(); let variants = payload.variants; // ... existing code ... - let workspace_settings = get_workspace(&workspace_request.schema_name, &mut conn)?; - /****************** Validating override_keys and variant overrides *********************/ validate_override_keys(&override_keys)?;Also applies to: 1421-1421
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
crates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/context_aware_config/src/helpers.rscrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/experimentation_platform/src/api/experiments/helpers.rscrates/frontend/src/components/workspace_form.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_types/migrations/2026-01-02-214221-0000_workspace_idx_fix/down.sqlcrates/superposition_types/migrations/2026-01-02-214221-0000_workspace_idx_fix/up.sqlcrates/superposition_types/src/api/type_templates.rscrates/superposition_types/src/api/workspace.rssuperposition.sql
🧰 Additional context used
🧬 Code graph analysis (13)
crates/superposition_types/src/api/workspace.rs (1)
crates/superposition_types/src/api.rs (2)
default_true(26-28)deserialize_option_i64(65-91)
crates/context_aware_config/src/api/variables/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(503-538)
crates/superposition/src/webhooks/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(503-538)
crates/superposition_types/src/api/type_templates.rs (1)
crates/context_aware_config/src/api/type_templates/handlers.rs (4)
diesel(67-71)diesel(121-130)diesel(153-155)type_templates(83-86)
crates/context_aware_config/src/api/type_templates/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(503-538)
crates/context_aware_config/src/api/default_config/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(503-538)
crates/context_aware_config/src/api/context/handlers.rs (2)
crates/context_aware_config/src/helpers.rs (2)
get_workspace(262-270)validate_change_reason(503-538)crates/context_aware_config/src/api/context/helpers.rs (2)
query_description(351-368)diesel(421-433)
crates/experimentation_platform/src/api/experiment_groups/handlers.rs (1)
crates/experimentation_platform/src/api/experiments/helpers.rs (1)
fetch_and_validate_change_reason_with_function(865-919)
crates/context_aware_config/src/api/dimension/handlers.rs (3)
crates/context_aware_config/src/helpers.rs (2)
get_workspace(262-270)validate_change_reason(503-538)crates/experimentation_platform/src/api/experiments/helpers.rs (1)
get_workspace(44-52)crates/superposition/src/workspace/handlers.rs (4)
get_workspace(71-84)conn(127-127)conn(166-166)conn(284-284)
crates/context_aware_config/src/api/functions/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(503-538)
crates/context_aware_config/src/helpers.rs (1)
crates/experimentation_platform/src/api/experiments/helpers.rs (1)
get_workspace(44-52)
crates/frontend/src/components/workspace_form.rs (5)
crates/superposition_sdk/src/operation/create_workspace/_create_workspace_input.rs (4)
enable_context_validation(61-63)enable_context_validation(201-204)enable_change_reason_validation(65-67)enable_change_reason_validation(214-217)crates/superposition_sdk/src/operation/create_workspace/builders.rs (2)
enable_context_validation(212-215)enable_change_reason_validation(226-229)crates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rs (4)
enable_context_validation(70-72)enable_context_validation(228-231)enable_change_reason_validation(74-76)enable_change_reason_validation(241-244)crates/superposition_sdk/src/operation/update_workspace/builders.rs (2)
enable_context_validation(231-234)enable_change_reason_validation(245-248)crates/superposition_sdk/src/types/_workspace_response.rs (4)
enable_context_validation(112-114)enable_context_validation(380-383)enable_change_reason_validation(116-118)enable_change_reason_validation(394-397)
crates/experimentation_platform/src/api/experiments/handlers.rs (1)
crates/experimentation_platform/src/api/experiments/helpers.rs (1)
fetch_and_validate_change_reason_with_function(865-919)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Provider Tests (py)
- GitHub Check: Provider Tests (js)
- GitHub Check: Provider Tests (rust)
- GitHub Check: Provider Tests (kotlin)
- GitHub Check: Check formatting
- GitHub Check: Testing
🔇 Additional comments (35)
crates/superposition_types/migrations/2026-01-02-214221-0000_workspace_idx_fix/up.sql (1)
1-4: LGTM!The migration correctly transitions from a single-column index to a composite index on
(organisation_id, workspace_name)and adds a new index onworkspace_schema_name. This aligns with the workspace-aware validation flow introduced in the PR.superposition.sql (1)
145-147: LGTM!The index changes correctly mirror the migration file, transitioning to a composite index for improved query performance.
crates/superposition_types/src/api/type_templates.rs (1)
2-3: LGTM!The conditional diesel integration for
TypeTemplateUpdateRequestis correctly implemented using feature flags. This enables the struct to be used with Diesel'sAsChangesettrait for update operations when thediesel_derivesfeature is enabled.Also applies to: 6-7, 22-23
crates/context_aware_config/src/api/dimension/handlers.rs (3)
278-284: Consistent workspace validation pattern.The update handler correctly follows the same pattern as the create handler, fetching workspace settings before validation and passing them to
validate_change_reason. The same staleness consideration applies here as in the create handler.
192-195: LGTM!The transaction correctly uses
workspace_settingsfrom the outer scope to determine if dimensions are mandatory. The.unwrap_or_default()pattern provides safe handling of optionalmandatory_dimensions.Also applies to: 415-418
74-80: This code is correct as-is and does not require changes.The workspace settings fetch outside the transaction is not a staleness concern in this codebase. Investigation shows that:
get_workspace()is the only place where workspace settings are accessed from the database- There are no UPDATE, INSERT, or mutation operations on the workspaces table anywhere in the codebase
- The workspace settings are used inside the transaction only for a read-only check:
workspace_settings.mandatory_dimensions.unwrap_or_default().contains(&inserted_dimension.dimension)- Since workspace settings cannot change between the fetch and the transaction, moving the fetch inside the transaction provides no additional consistency benefit
Likely an incorrect or invalid review comment.
crates/context_aware_config/src/api/default_config/handlers.rs (2)
76-77: LGTM - Consistent with new validation pattern.The call to
validate_change_reason(None, ...)correctly uses the updated signature. The TODO comment appropriately flags future optimization potential.
207-208: LGTM - Matches create handler pattern.Consistent application of the new validation approach with appropriate TODO for future optimization.
crates/context_aware_config/src/helpers.rs (1)
503-515: LGTM - Clean workspace-aware validation flow.The updated signature with optional workspace settings allows callers to avoid redundant DB calls when workspace is already fetched. The early return for disabled validation is correctly placed before any function lookup.
crates/context_aware_config/src/api/variables/handlers.rs (2)
16-17: LGTM - Change reason validation added to create path.The import and validation call follow the established pattern from other handlers.
Also applies to: 104-106
158-160: LGTM - Consistent validation in update path.crates/context_aware_config/src/api/type_templates/handlers.rs (2)
51-72: LGTM - Cleaner struct-based insertion with validation.The refactor to construct a
TypeTemplatestruct before insertion improves readability. Change reason validation is correctly placed before the database operation.
115-131: LGTM - Validation added to update path with simplified update.The change reason validation is correctly placed before the database update, and the simplified
set((request, ...))pattern is cleaner.crates/context_aware_config/src/api/functions/handlers.rs (3)
65-66: LGTM - Change reason validation in create path.
138-140: LGTM - Change reason validation in update (draft) path.
301-314: LGTM - Publish path now validates change reason and updates audit fields.The addition of
last_modified_byandlast_modified_atto the publish update ensures consistent audit tracking.crates/context_aware_config/src/api/context/handlers.rs (3)
82-101: LGTM - Improved description handling with NotFound error.The explicit handling of the
NotFoundcase with a clear error message improves the API ergonomics by requiring a description for new contexts.
104-105: LGTM - Validation before transaction.Placing validation outside the transaction is correct - it fails fast before starting any DB modifications.
503-504: LGTM - Efficient workspace fetch for bulk operations.Fetching workspace settings once at the start and passing
Some(&workspace_settings)to per-item validations avoids redundant DB calls. This is the intended optimization pattern.Also applies to: 516-521
crates/frontend/src/components/workspace_form.rs (2)
34-35: LGTM - Default values updated.The default values for validation flags changed from
truetofalse, which aligns with the backend changes where these are now explicitboolfields with serde defaults.
95-101: LGTM - Payload construction aligned with backend type changes.The fields are now passed as raw boolean values instead of
Some(...)wrapped, which matches the backend's expectation ofboolfields with serde defaults.crates/experimentation_platform/src/api/experiments/helpers.rs (2)
44-52: LGTM! Clean helper function for workspace retrieval.The
get_workspacehelper function provides a centralized way to fetch workspace settings, improving code reusability across the codebase.
865-873: LGTM! Improved encapsulation of validation logic.Moving the
enable_change_reason_validationcheck inside the function is a good design choice. It centralizes the validation gating logic and simplifies caller code.crates/superposition/src/workspace/handlers.rs (1)
120-123: LGTM! Simplified field assignment with serde defaults.The removal of
unwrap_orcalls is safe because the corresponding fields inCreateWorkspaceRequestwere changed fromOption<bool>toboolwith appropriate#[serde(default)]attributes. This moves the defaulting logic to the deserialization layer, which is cleaner.crates/superposition/src/webhooks/handlers.rs (2)
85-86: Consistent validation pattern across handlers.The validation approach here mirrors the
createhandler, maintaining consistency. The same optimization opportunity applies.
42-43: Consider future optimization for workspace settings.The TODO comment correctly identifies that passing
Nonecauses an extra database call to fetch workspace settings. Since this pattern already exists in dimension handlers with workspace pre-fetching, the optimization is architecturally feasible.However, for the current PR focused on consistency, this approach is acceptable.
crates/experimentation_platform/src/api/experiment_groups/handlers.rs (4)
90-96: LGTM! Consistent validation with workspace context.The refactored validation call now passes workspace settings directly, allowing the validation function to handle the
enable_change_reason_validationcheck internally. This improves consistency across the codebase.
168-174: Consistent validation pattern in update handler.This follows the same pattern as the create handler, maintaining consistency.
203-209: Consistent validation pattern in add-members handler.The validation approach is consistent with other handlers in this file.
256-262: Consistent validation pattern in remove-members handler.The validation approach is consistent with other handlers in this file.
crates/experimentation_platform/src/api/experiments/handlers.rs (5)
140-148: LGTM! Workspace settings validation implemented correctly.The workspace settings are fetched once and used consistently throughout the handler for both validation and subsequent checks.
424-432: LGTM! Validation properly integrated.Workspace settings are fetched and validation is called before proceeding with the conclude operation.
707-715: LGTM! Validation correctly added.The handler properly fetches workspace settings and validates the change reason before discarding the experiment.
1722-1730: LGTM! Validation properly implemented.The pause handler correctly fetches workspace settings and validates the change reason before pausing the experiment.
1821-1829: LGTM! Validation correctly integrated.The resume handler properly fetches workspace settings and validates the change reason before resuming the experiment.
crates/superposition_types/migrations/2026-01-02-214221-0000_workspace_idx_fix/down.sql
Outdated
Show resolved
Hide resolved
ayushjain17
force-pushed
the
fix/workspace-settings
branch
4 times, most recently
from
69a47d9 to
329b34d
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/context_aware_config/src/api/context/handlers.rs (1)
109-109: Minor: Fix typo in TODO comments.The TODO comments contain a typo: "already fetcehs" should be "already fetches".
🔎 Suggested fix
- // TODO: already fetcehs + // TODO: already fetchesAlso applies to: 549-549
crates/superposition/src/webhooks/handlers.rs (2)
85-86: Same performance concern as in thecreatehandler.This has the identical pattern and performance implication as lines 42-43. Both handlers would benefit from a shared solution that fetches workspace settings once per request lifecycle.
42-43: Consider fetching workspace settings earlier in the request lifecycle.The TODO comment correctly identifies that passing
Nonetovalidate_change_reasontriggers an internal DB call to fetch workspace settings. According to the function implementation, this DB call happens unconditionally—even whenenable_change_reason_validationis disabled in the workspace settings.To verify whether workspace settings are already fetched elsewhere in the request lifecycle, run:
#!/bin/bash # Search for workspace-related fetches in webhook request handlers rg -n --type rust -C5 'get_workspace|workspace_settings|Workspace' crates/superposition/src/webhooks/If workspace settings aren't currently fetched in the webhook request lifecycle, consider:
- Adding middleware to fetch and attach workspace settings to the request context
- Passing the workspace settings down through the handler chain
- This would eliminate the redundant DB call in both
createandupdatehandlers
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
crates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/context_aware_config/src/helpers.rscrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/experimentation_platform/src/api/experiments/helpers.rscrates/frontend/src/components/workspace_form.rscrates/service_utils/src/helpers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_types/src/api/type_templates.rscrates/superposition_types/src/api/workspace.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-03T10:25:34.298Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 814
File: crates/superposition_types/src/api/workspace.rs:70-77
Timestamp: 2026-01-03T10:25:34.298Z
Learning: The `auto_populate_control` field in `CreateWorkspaceRequest` (`crates/superposition_types/src/api/workspace.rs`) is expected to default to `true`, not `false`. This is implemented using `#[serde(default = "default_true")]`.
Applied to files:
crates/superposition_types/src/api/workspace.rscrates/frontend/src/components/workspace_form.rscrates/superposition/src/workspace/handlers.rs
🧬 Code graph analysis (13)
crates/superposition/src/webhooks/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(490-525)
crates/context_aware_config/src/api/variables/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(490-525)
crates/superposition_types/src/api/workspace.rs (1)
crates/superposition_types/src/api.rs (2)
default_true(26-28)deserialize_option_i64(65-91)
crates/frontend/src/components/workspace_form.rs (7)
crates/superposition_sdk/src/operation/create_workspace/builders.rs (2)
enable_context_validation(212-215)enable_change_reason_validation(226-229)crates/superposition_sdk/src/operation/create_workspace/_create_workspace_input.rs (4)
enable_context_validation(61-63)enable_context_validation(201-204)enable_change_reason_validation(65-67)enable_change_reason_validation(214-217)crates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rs (4)
enable_context_validation(70-72)enable_context_validation(228-231)enable_change_reason_validation(74-76)enable_change_reason_validation(241-244)crates/superposition_sdk/src/operation/create_workspace/_create_workspace_output.rs (4)
enable_context_validation(112-114)enable_context_validation(380-383)enable_change_reason_validation(116-118)enable_change_reason_validation(394-397)crates/superposition_sdk/src/operation/update_workspace/builders.rs (2)
enable_context_validation(231-234)enable_change_reason_validation(245-248)crates/superposition_sdk/src/operation/update_workspace/_update_workspace_output.rs (4)
enable_context_validation(112-114)enable_context_validation(380-383)enable_change_reason_validation(116-118)enable_change_reason_validation(394-397)crates/superposition_sdk/src/types/_workspace_response.rs (4)
enable_context_validation(112-114)enable_context_validation(380-383)enable_change_reason_validation(116-118)enable_change_reason_validation(394-397)
crates/superposition_types/src/api/type_templates.rs (1)
crates/context_aware_config/src/api/type_templates/handlers.rs (4)
diesel(67-71)diesel(121-130)diesel(153-155)type_templates(83-86)
crates/experimentation_platform/src/api/experiment_groups/handlers.rs (2)
crates/experimentation_platform/src/api/experiments/helpers.rs (1)
fetch_and_validate_change_reason_with_function(855-909)crates/context_aware_config/src/helpers.rs (4)
state(276-278)state(280-282)state(290-292)state(295-297)
crates/context_aware_config/src/api/default_config/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(490-525)
crates/context_aware_config/src/api/context/handlers.rs (2)
crates/service_utils/src/helpers.rs (2)
get_workspace(542-550)parse_config_tags(371-396)crates/context_aware_config/src/api/context/helpers.rs (2)
query_description(350-367)diesel(420-432)
crates/service_utils/src/helpers.rs (4)
crates/context_aware_config/src/api/context/helpers.rs (1)
diesel(420-432)crates/context_aware_config/src/api/dimension/handlers.rs (2)
diesel(365-374)diesel(403-412)crates/superposition/src/workspace/handlers.rs (3)
diesel(167-175)workspaces(195-197)get_workspace(71-84)crates/context_aware_config/src/api/context/operations.rs (1)
dsl(85-88)
crates/context_aware_config/src/api/context/helpers.rs (2)
crates/service_utils/src/helpers.rs (1)
get_workspace(542-550)crates/context_aware_config/src/helpers.rs (1)
calculate_context_weight(111-147)
crates/context_aware_config/src/helpers.rs (3)
crates/service_utils/src/helpers.rs (2)
generate_snowflake_id(360-369)get_workspace(542-550)crates/superposition/src/workspace/handlers.rs (4)
get_workspace(71-84)conn(127-127)conn(166-166)conn(284-284)crates/context_aware_config/src/api/dimension/handlers.rs (3)
conn(154-155)conn(348-349)conn(520-520)
crates/context_aware_config/src/api/functions/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(490-525)
crates/experimentation_platform/src/api/experiments/handlers.rs (4)
crates/service_utils/src/helpers.rs (3)
construct_request_headers(322-337)request(339-359)request(453-453)crates/frontend/src/utils.rs (2)
construct_request_headers(198-213)request(284-294)crates/experimentation_platform/src/api/experiments/helpers.rs (2)
validate_control_overrides(810-853)fetch_and_validate_change_reason_with_function(855-909)crates/service_utils/src/middlewares/tenant.rs (1)
req(78-78)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Testing
- GitHub Check: checks (aarch64-darwin)
🔇 Additional comments (33)
crates/superposition_types/src/api/type_templates.rs (3)
2-7: LGTM! Conditional imports follow Rust best practices.The conditional imports for Diesel support are correctly implemented using feature gates, ensuring that diesel dependencies are only included when the
diesel_derivesfeature is enabled.
22-23: LGTM! Diesel integration correctly implemented.The conditional
AsChangesetderivation and table mapping are properly configured. This enables the struct to be used in diesel update queries as shown in the relevant code snippet fromhandlers.rs:120-129, whererequestis used in.set((request, ...)).
27-27: No issues found. Thechange_reasonfield addition is properly integrated:
- Database schema alignment: The
type_templatestable includes achange_reasoncolumn (defined in migrations and Diesel schema)- API consumers updated: The frontend
build_update_request()function already constructsTypeTemplateUpdateRequestwith thechange_reasonfield- Backend validation: The update handler validates
change_reasonviavalidate_change_reason()before processingThe breaking change is intentional and aligns with the PR's goal of unconditional change reason validation.
crates/experimentation_platform/src/api/experiments/helpers.rs (1)
855-863: LGTM! Workspace settings parameterization improves consistency.The refactoring to accept
workspace_settingsas an explicit parameter rather than fetching it internally makes the validation flow more transparent and testable. The early return whenenable_change_reason_validationis false is an appropriate optimization.crates/context_aware_config/src/api/context/helpers.rs (1)
10-10: LGTM! Import reorganization aligns with workspace helper centralization.The import path adjustment reflects the movement of
get_workspacetoservice_utils, supporting the PR's goal of consistent workspace settings usage across modules.Also applies to: 36-36
crates/context_aware_config/src/api/variables/handlers.rs (2)
16-16: LGTM! Change reason validation integrated for variable creation.The validation hook is properly integrated with an explicit note about potential optimization. Passing
Nonefor workspace settings is acceptable as the validation function will fetch them internally when needed.Also applies to: 104-105
158-159: LGTM! Change reason validation integrated for variable updates.Consistent with the create path, the validation is properly integrated with the same optimization note.
crates/superposition/src/workspace/handlers.rs (1)
120-123: LGTM! Direct field assignment aligns with non-optional boolean transition.The removal of
.unwrap_or()defaulting in favor of direct field assignment is consistent with the PR's transition to non-optional boolean fields with serde defaults. The serde defaults inCreateWorkspaceRequestare correctly configured:auto_populate_controldefaults totruevia#[serde(default = "default_true")], whileallow_experiment_self_approval,enable_context_validation, andenable_change_reason_validationdefault tofalsevia#[serde(default)].crates/service_utils/src/helpers.rs (2)
11-11: LGTM! Import additions support the new get_workspace function.The new imports for Diesel query building, Workspace model, and schema access are properly scoped and necessary for the centralized workspace retrieval function.
Also applies to: 28-39
540-550: LGTM! Centralized workspace retrieval function is correctly implemented.The function provides a clean interface for fetching workspace settings by schema name. The TODO comment appropriately acknowledges the blocking DB call concern and suggests a potential middleware optimization. This is a reasonable trade-off for now, especially since the comment notes it might not be frequently used.
crates/context_aware_config/src/api/default_config/handlers.rs (2)
76-77: LGTM! Change reason validation correctly integrated.Passing
Nonefor workspace settings is appropriate here since the workspace isn't fetched elsewhere in this handler. The validation function will fetch it internally when needed. The TODO provides helpful context for potential future optimization.
207-208: LGTM! Consistent validation pattern.The validation call follows the same pattern as in
create_default_config, maintaining consistency across the codebase.crates/frontend/src/components/workspace_form.rs (3)
34-35: Verify that validation defaults should befalse.The default values for
enable_context_validationandenable_change_reason_validationchanged fromtruetofalse. This means validations are now OFF by default when creating new workspaces. Please confirm this behavioral change aligns with the intended UX, as it requires users to explicitly opt into validation rather than opting out.
52-55: LGTM! Signal wiring correctly matches prop names.The signal creation now properly maps each validation flag to its corresponding prop, ensuring the component state reflects the intended configuration.
95-101: LGTM! Payload construction updated to match new API types.The removal of
Some()wrappers correctly aligns with the updatedCreateWorkspaceRequeststruct where these fields changed fromOption<bool>tobool.crates/superposition_types/src/api/workspace.rs (2)
9-9: LGTM! Import addition supports auto_populate_control default.The
default_trueimport is correctly added to support the serde default attribute for theauto_populate_controlfield.
70-77: LGTM! Field type changes improve API clarity.The transition from
Option<bool>toboolwith serde defaults eliminates ambiguity and ensures these fields always have defined values. The defaults are correctly configured:
auto_populate_controldefaults totrue(as per learnings)- The validation flags default to
false(aligning with frontend defaults)The changes are backward compatible for deserialization—clients omitting these fields will receive the documented default values.
Based on learnings, auto_populate_control correctly defaults to true.
crates/context_aware_config/src/helpers.rs (2)
20-20: LGTM! Centralized workspace retrieval via service_utils.Importing
get_workspacefrom the centralized helper inservice_utilseliminates code duplication and ensures consistent workspace retrieval across the codebase.
490-525: LGTM! Workspace-aware validation correctly implemented.The updated signature and implementation deliver the core fix:
- Optimization: Accepts an optional
workspace_settingsparameter to avoid redundant DB calls when the workspace is already available in the caller's context.- Conditional validation: Checks
workspace.enable_change_reason_validationand short-circuits when validation is disabled, ensuring consistent enforcement of workspace settings.This change makes change reason validation workspace-aware as intended by the PR.
crates/context_aware_config/src/api/dimension/handlers.rs (2)
12-12: LGTM! Workspace settings correctly fetched and passed to validation.The pre-transaction fetch of
workspace_settingsviaget_workspaceensures that the change reason validation is workspace-aware. PassingSome(&workspace_settings)avoids redundant DB calls insidevalidate_change_reason.Also applies to: 74-80
278-284: Consistent validation pattern in update handler.The update handler follows the same pattern as create: fetching workspace settings before the transaction and passing them to
validate_change_reason. This ensures consistent behavior across both endpoints.crates/experimentation_platform/src/api/experiments/handlers.rs (2)
27-28: LGTM! Consistent workspace-aware change reason validation across experiment handlers.The pattern of fetching
workspace_settingsviaget_workspaceand passing it tofetch_and_validate_change_reason_with_functionis consistently applied across all state-changing endpoints (create, conclude, discard, ramp, update_overrides, pause, resume).Also applies to: 140-148
426-432: Uniform validation pattern maintained across all experiment state handlers.All handlers follow the same flow:
- Fetch workspace settings once at handler entry
- Pass to validation function which checks
enable_change_reason_validationinternally- Continue with business logic
This eliminates previous inconsistencies in how change reason validation was gated.
Also applies to: 709-715, 1177-1183, 1372-1378, 1720-1726, 1819-1825
crates/context_aware_config/src/api/type_templates/handlers.rs (2)
51-72: LGTM! Clean struct-based insertion with workspace-aware validation.Using
Nonefor workspace settings is valid sincevalidate_change_reasonwill fetch it internally. The TODO comment appropriately documents the future optimization opportunity. The consolidatedTypeTemplatestruct insertion is cleaner than field-by-field insertion.
115-131: Update handler correctly uses consolidated payload.The
validate_change_reason(None, ...)call ensures validation occurs, with the internal workspace fetch. Theset((request, ...))pattern cleanly applies the update request alongside timestamp fields.crates/context_aware_config/src/api/functions/handlers.rs (2)
65-66: LGTM! Consistent change reason validation across function endpoints.All three mutating endpoints (create, update, publish) now properly validate the change reason. The TODO comments acknowledge the optimization opportunity to pass pre-fetched workspace settings.
Also applies to: 138-139, 301-302
312-313: Good addition of last_modified tracking in publish.The publish endpoint now correctly updates
last_modified_byandlast_modified_atfields, maintaining consistent audit trail metadata.crates/experimentation_platform/src/api/experiment_groups/handlers.rs (2)
13-13: LGTM! Experiment group handlers follow the established validation pattern.The
get_workspaceimport and usage pattern aligns with the broader refactor. Workspace settings are fetched once and passed to validation for all mutating endpoints.Also applies to: 77-77, 90-96
167-174: Consistent validation across all experiment group mutation endpoints.Update, add-members, and remove-members endpoints all follow the same workspace-aware validation pattern, ensuring uniform change reason validation behavior.
Also applies to: 202-209, 255-262
crates/context_aware_config/src/api/context/handlers.rs (3)
84-105: LGTM! Proper description requirement enforcement.The logic correctly returns a
bad_argumenterror when no description is provided and the context doesn't exist, while allowing existing contexts to retain their description. The validation pattern withNoneis consistent with other handlers.
220-240: Consistent description handling in move_handler.The move handler mirrors the put_handler logic for description requirement, maintaining consistent behavior across context operations.
516-534: LGTM! Bulk operations efficiently reuses workspace settings.Workspace settings are fetched once before the transaction loop and passed to each
validate_change_reasoncall viaSome(&workspace_settings). This avoids redundant DB calls during bulk operations, which is the optimal pattern for this use case.crates/superposition/src/webhooks/handlers.rs (1)
8-8: LGTM! Import changes support the refactored validation.The added import for
validate_change_reasonand the service_utils types are appropriate for the centralized validation approach.Also applies to: 12-12
ayushjain17
force-pushed
the
fix/workspace-settings
branch
3 times, most recently
from
8ea1727 to
02994b4
Compare
| let var_name = path.into_inner(); | ||
|
|
||
| // TODO: if ever workspace settings is fetched in this request lifecycle, pass it here to avoid extra db call. | ||
| validate_change_reason(None, &req.change_reason, &mut conn, &schema_name)?; |
There was a problem hiding this comment.
We’re passing None here and then re-fetching it inside validate_change_reason. It would be cleaner to fetch the value here itself, avoiding dependencies on future changes. We can also do the check here and call the function only when needed, keeping the function purely responsible for validation.
There was a problem hiding this comment.
That in fact is a very bad practice
The exact reason why this PR was needed, as the check was separate responsibility which was missed out in most of the places causing the problem
You dont keep core inter-related logic decoupled like this
There was a problem hiding this comment.
@mahatoankitkumar have made the changes to pass the settings to the function and removed the fetch from inside
ayushjain17
force-pushed
the
fix/workspace-settings
branch
2 times, most recently
from
0d2ec39 to
828820a
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
ayushjain17
force-pushed
the
fix/workspace-settings
branch
2 times, most recently
from
5ee0772 to
f4834e4
Compare
| #[cfg(all(feature = "server", feature = "result"))] | ||
| impl FromRequest for Workspace { | ||
| type Error = Error; | ||
| type Future = std::future::Ready<Result<Self, Self::Error>>; | ||
|
|
||
| fn from_request( | ||
| req: &actix_web::HttpRequest, | ||
| _: &mut actix_web::dev::Payload, | ||
| ) -> Self::Future { | ||
| use std::future::ready; | ||
|
|
||
| let result = req.extensions().get::<Self>().cloned().ok_or_else(|| { | ||
| crate::result::AppError::UnexpectedError( | ||
| anyhow::anyhow!("Please check that the organisation id and workspace id are being properly sent") | ||
| ).into() | ||
| }); | ||
|
|
||
| ready(result) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this needed? You aren't inserting Workspace anywhere explicitly, we are inserting this data into WorkspaceContext::settings field
There was a problem hiding this comment.
yeah its not needed, I too wanted to remove it, left it for the consistency part
if we agree then yeah good to remove it
There was a problem hiding this comment.
Can you remove it and push, I'll approve
There was a problem hiding this comment.
yeah dead code, pushed by mistake
ayushjain17
force-pushed
the
fix/workspace-settings
branch
3 times, most recently
from
a3129e5 to
ed96493
Compare
ayushjain17
force-pushed
the
fix/workspace-settings
branch
from
ed96493 to
b241cd3
Compare
Problem
For change reason validation, workspace settings was not being checked consistently
Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.