fix: function attachment logic for dimension and default config #832
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 You can disable this status message by setting the WalkthroughThreads a FunctionType parameter through function lookup and validation paths so published functions are fetched and checked by explicit type (ValueValidation / ValueCompute) across backend helpers, validations, default-config handlers, and frontend function-name utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant API as Backend API
participant FuncHelper as functions::helpers
participant DB as Database
Frontend->>API: submit config / select function (includes FunctionType)
API->>FuncHelper: validate_fn_published(name, FunctionType)
FuncHelper->>DB: query published functions by name + FunctionType
DB-->>FuncHelper: FunctionInfo (with function_type)
FuncHelper-->>API: validation result (error if type mismatch)
API-->>Frontend: success / validation error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
crates/frontend/src/components/default_config_form.rs (1)
439-459: FunctionType-filtered dropdown options look correct; consider avoidingconcat()allocations.Filtering by
FunctionType::ValueValidation/FunctionType::ValueComputematches the PR objective and prevents wrong-type attachments. If you want to avoid intermediate allocations, you can build the vectors via iterator chaining.Proposed refactor (optional)
- let validation_function_names: Vec<FunctionsName> = [ - vec!["None".to_string()], - functions - .iter() - .filter(|ele| { - ele.function_type == FunctionType::ValueValidation - }) - .map(|ele| ele.function_name.clone()) - .collect(), - ] - .concat(); - let value_compute_function_names: Vec<FunctionsName> = [ - vec!["None".to_string()], - functions - .iter() - .filter(|ele| ele.function_type == FunctionType::ValueCompute) - .map(|ele| ele.function_name.clone()) - .collect(), - ] - .concat(); + let validation_function_names: Vec<FunctionsName> = + std::iter::once("None".to_string()) + .chain( + functions + .iter() + .filter(|f| f.function_type == FunctionType::ValueValidation) + .map(|f| f.function_name.clone()), + ) + .collect(); + let value_compute_function_names: Vec<FunctionsName> = + std::iter::once("None".to_string()) + .chain( + functions + .iter() + .filter(|f| f.function_type == FunctionType::ValueCompute) + .map(|f| f.function_name.clone()), + ) + .collect();crates/frontend/src/components/dimension_form.rs (1)
529-549: FunctionType-filtered dropdown options look correct; optional micro-refactor to avoidconcat().Same as default-config: the filtering logic is correct and matches the backend’s FunctionType-aware behavior. If desired, you can switch to
iter::once(...).chain(...).collect()to avoid intermediateVecallocations (see similar suggestion indefault_config_form.rs).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/validations.rscrates/context_aware_config/src/api/functions/helpers.rscrates/context_aware_config/src/helpers.rscrates/frontend/src/components/default_config_form.rscrates/frontend/src/components/dimension_form.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-07T20:38:53.153Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
Applied to files:
crates/context_aware_config/src/api/dimension/validations.rscrates/frontend/src/components/default_config_form.rscrates/frontend/src/components/dimension_form.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/helpers.rscrates/context_aware_config/src/api/functions/helpers.rs
📚 Learning: 2026-01-05T12:37:05.828Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 815
File: crates/frontend/src/components/datetime.rs:70-95
Timestamp: 2026-01-05T12:37:05.828Z
Learning: Documentation should reflect build-target dependent behavior. For functions that differ between wasm32 and non-wasm32 (e.g., datetime parsing/normalization), explicitly document the intent and the platform-specific logic in the code and/or a nearby comment. Prefer using cfg attributes to clearly separate implementations for wasm32 vs non-wasm32 and add tests that cover both builds. In this case, the function should be noted as: wasm32 -> local midnight interpreted then converted to UTC; non-wasm32 (SSR) -> UTC midnight directly. Ensure any related behavior is consistent with the SSR/CSR architecture and that downstream code relies on the clarified semantics.
Applied to files:
crates/frontend/src/components/default_config_form.rscrates/frontend/src/components/dimension_form.rs
🧬 Code graph analysis (3)
crates/context_aware_config/src/api/dimension/validations.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (1)
get_published_function_code(26-42)
crates/context_aware_config/src/api/default_config/handlers.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (1)
get_published_function_code(26-42)
crates/context_aware_config/src/api/functions/helpers.rs (1)
crates/context_aware_config/src/api/functions/handlers.rs (1)
functions(137-141)
🔇 Additional comments (16)
crates/context_aware_config/src/api/dimension/validations.rs (4)
10-17: Import update for FunctionType is consistent with the new validation contract.No issues here.
243-259: Type-scoped publication check is the right fix for “wrong function type attached”.
check_fn_publishednow enforces(name, type)when checking published code, which closes the gap described in the PR objectives.
290-300: Validation function check now correctly enforces ValueValidation type.This prevents “compute fn attached as validation fn” (and vice versa).
261-289: This concern is not applicable—FunctionTypederivesCopy.The
FunctionTypeenum explicitly derivesCopy(along withClone,Debug,PartialEq,Deserialize,Serialize,Default, and strum macros). Since it implementsCopy, the pattern of bindingfn_typeonce and reusing it across multiple match arms is valid and idiomatic Rust. No changes are needed.crates/context_aware_config/src/api/functions/helpers.rs (3)
1-3: Diesel imports updated appropriately for composed boolean predicates.Looks fine.
26-42: Good: published function lookup is now constrained by(name, type).This is the core behavioral fix (prevents retrieving a different-type function with the same name).
44-61: Good: bulk lookup now also enforces function type.Same benefit as the single-function path; avoids mixed-type maps when names overlap. All callers properly updated to pass FunctionType.
crates/context_aware_config/src/helpers.rs (1)
398-408: Good: remote cohort evaluation now explicitly fetches ValueCompute functions.This aligns the runtime lookup with the semantics of
data.value_compute_function_nameand prevents accidentally resolving a same-named function of a different type.crates/context_aware_config/src/api/context/helpers.rs (3)
155-160: LGTM! Correct function type for dimension validation.The addition of
FunctionType::ValueValidationcorrectly ensures that only value validation functions are fetched when validating dimension values. This prevents accidentally using a compute function where a validation function is expected.
255-260: LGTM! Correct function type for override validation.The addition of
FunctionType::ValueValidationensures that validation functions from default configs are correctly fetched by type. This maintains consistency with the dimension validation logic.
290-321: LGTM! Clean signature update with proper propagation.The function signature is correctly updated to accept and propagate the
function_typeparameter. The implementation maintains the existing logic while adding type-based filtering at the database query level.crates/context_aware_config/src/api/default_config/handlers.rs (5)
32-32: LGTM! Required import for function type validation.The import of
FunctionTypeis correctly added to support the function type parameter threading.
130-135: LGTM! Correct validation of compute function on creation.The create handler correctly validates that the compute function exists and is published by passing
FunctionType::ValueCompute. This prevents attaching non-existent or wrong-type functions to default configs.
251-256: LGTM! Consistent validation in update handler.The update handler correctly validates the compute function type when updating
value_compute_function_name. The conditional validation (only when provided) is appropriate for partial updates.
294-314: LGTM! Well-refactored validation function.The signature update makes
validate_fn_publishedreusable for different function types while maintaining clear error messages. The function now ensures type-safe validation of function existence and publication status.
328-333: LGTM! Correct function type for validation path.The call to
get_published_function_codecorrectly specifiesFunctionType::ValueValidationfor fetching value validation functions. This ensures that only validation functions can be used for validating default config values.
ayushjain17
force-pushed
the
fix/function/attachment
branch
from
db4ab28 to
bd22b70
Compare
ayushjain17
force-pushed
the
fix/function/attachment
branch
from
bd22b70 to
e627507
Compare
ayushjain17
force-pushed
the
fix/function/attachment
branch
2 times, most recently
from
dd1117c to
549786f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/context_aware_config/src/api/functions/helpers.rs (2)
25-45: Improve type-mismatch error: include expected + actual.
This will cut debugging time when a function name is reused incorrectly.Proposed tweak
@@ - if function.function_type != f_type { + if function.function_type != f_type { return Err(validation_error!( - "Function type mismatch for function: {}", - f_name + "Function type mismatch for function: {} (expected: {:?}, found: {:?})", + f_name, + f_type, + function.function_type, )); }
47-66: Nice guardrail: bulk fetch now rejects any returned function with the wrong type.
Optional follow-up: consider also detecting and erroring on missing function names (today they’ll just be absent from the returned vector).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/validations.rscrates/context_aware_config/src/api/functions/helpers.rscrates/context_aware_config/src/api/functions/types.rscrates/context_aware_config/src/helpers.rscrates/frontend/src/components/default_config_form.rscrates/frontend/src/components/dimension_form.rscrates/frontend/src/types.rscrates/frontend/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/frontend/src/components/default_config_form.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-07T20:38:53.153Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
Applied to files:
crates/frontend/src/types.rscrates/context_aware_config/src/helpers.rscrates/context_aware_config/src/api/functions/helpers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/frontend/src/components/dimension_form.rscrates/frontend/src/utils.rscrates/context_aware_config/src/api/dimension/validations.rscrates/context_aware_config/src/api/functions/types.rs
📚 Learning: 2026-01-05T12:37:05.828Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 815
File: crates/frontend/src/components/datetime.rs:70-95
Timestamp: 2026-01-05T12:37:05.828Z
Learning: Documentation should reflect build-target dependent behavior. For functions that differ between wasm32 and non-wasm32 (e.g., datetime parsing/normalization), explicitly document the intent and the platform-specific logic in the code and/or a nearby comment. Prefer using cfg attributes to clearly separate implementations for wasm32 vs non-wasm32 and add tests that cover both builds. In this case, the function should be noted as: wasm32 -> local midnight interpreted then converted to UTC; non-wasm32 (SSR) -> UTC midnight directly. Ensure any related behavior is consistent with the SSR/CSR architecture and that downstream code relies on the clarified semantics.
Applied to files:
crates/frontend/src/components/dimension_form.rs
🧬 Code graph analysis (4)
crates/context_aware_config/src/api/default_config/handlers.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (1)
get_published_function_code(25-45)
crates/frontend/src/components/dimension_form.rs (1)
crates/frontend/src/utils.rs (2)
set_function(328-336)get_fn_names_by_type(424-436)
crates/context_aware_config/src/api/dimension/validations.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (1)
get_published_function_code(25-45)
crates/context_aware_config/src/api/functions/types.rs (1)
crates/context_aware_config/src/api/functions/handlers.rs (1)
functions(137-141)
🔇 Additional comments (23)
crates/context_aware_config/src/api/functions/types.rs (2)
1-5: AddingFunctionTypeto the CAC model import is consistent with the type-aware validation change.
7-14: No issues found. AllFunctionInfoqueries properly useFunctionInfo::as_select()(helpers.rs lines 33, 55, 104), which ensuresfunction_typeis compiled into the column selection. All destructuring patterns use..catch-all, making them forward-compatible with new fields.crates/context_aware_config/src/api/dimension/validations.rs (3)
10-17:FunctionTypeimport aligns with the new typed function validation.
243-259: Good:check_fn_publishednow enforces published-by-type viaget_published_function_code.
This makes the type mismatch fail fast (instead of silently accepting a wrong function).
261-300: Good: compute vs validation paths are now explicitly typed.
ValueComputeandValueValidationare correctly separated at call sites.crates/context_aware_config/src/api/functions/helpers.rs (1)
1-10: Importingvalidation_erroris appropriate for typed validation failures.crates/context_aware_config/src/helpers.rs (1)
398-407: Good: remote cohort value compute now fetches published code withFunctionType::ValueCompute.crates/context_aware_config/src/api/context/helpers.rs (3)
155-160: Good: dimension validation functions are now fetched/validated asValueValidation.
255-260: Good: default-config validation functions are now fetched/validated asValueValidation.
290-321:get_functions_mapsignature change is clean; passingfunction_typedown is the right abstraction boundary.crates/context_aware_config/src/api/default_config/handlers.rs (5)
32-32: LGTM: FunctionType import added.The import correctly adds
FunctionTypeto support function type validation in this module.
130-135: LGTM: ValueCompute function validation added.The validation correctly uses
FunctionType::ValueComputeto ensure the attached function is of the correct type for computing values in the create handler.
251-256: LGTM: ValueCompute function validation added to update handler.Consistent with the create handler, the update path now validates that the value compute function is of type
ValueCompute.
294-314: LGTM: Function signature updated with type parameter.The
validate_fn_publishedfunction now correctly acceptsf_type: FunctionTypeand passes it toget_published_function_code, enabling type-specific function lookup and validation. The error handling remains appropriate.
328-333: LGTM: ValueValidation type used correctly.The call to
get_published_function_codecorrectly specifiesFunctionType::ValueValidationfor validation functions attached to default config values.crates/frontend/src/types.rs (1)
145-153: LGTM: Type alias renamed for consistency.The rename from
FunctionsNametoFunctionName(singular) is a sensible naming improvement that aligns with Rust naming conventions.crates/frontend/src/components/dimension_form.rs (3)
24-44: LGTM: Imports updated for type-safe function handling.The imports correctly add
FunctionNameandget_fn_names_by_typeto support the new function-type-aware dropdown population logic.
131-137: LGTM: Closure parameter types updated.Both closure signatures correctly use
FunctionNameinstead of the oldFunctionsName, maintaining consistency with the renamed type.
532-539: LGTM: Function lists now filtered by type.The new logic correctly uses
get_fn_names_by_typeto populate separate dropdown lists forValueValidationandValueComputefunctions, ensuring users can only select functions of the appropriate type for each purpose.crates/frontend/src/utils.rs (4)
11-14: LGTM: Imports extended for function type handling.The imports correctly add
FunctionandFunctionTypefromsuperposition_typesto support the new type-aware function filtering logic.
23-23: LGTM: FunctionName import updated.The import correctly references the renamed
FunctionNametype.
328-336: LGTM: Function signature updated.The
set_functionsignature correctly usesFunctionNameinstead ofFunctionsName, maintaining consistency with the type rename.
424-436: LGTM: Type-aware function name helper added.The new
get_fn_names_by_typehelper correctly filters functions byFunctionTypeand prepends "None" as the first option, enabling type-safe function selection in the UI. The implementation is clean and efficient.
ayushjain17
force-pushed
the
fix/function/attachment
branch
from
549786f to
5aeb236
Compare
ayushjain17
force-pushed
the
fix/function/attachment
branch
from
5aeb236 to
43af0ad
Compare
ayushjain17
force-pushed
the
fix/function/attachment
branch
from
43af0ad to
6a84cfd
Compare
ayushjain17
added
bug
Problem
No validation on the type of function given in input for validation or compute function attachment to default config and dimension
Probably needs some refactoring in all these logic
Summary by CodeRabbit
Refactor
Bug Fixes / Validation
✏️ Tip: You can customize this high-level summary in your review settings.