fix: exp and config partial apply #831
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 WalkthroughAdds a new dimensions JSON parameter to experiment query APIs across Haskell FFI and Rust layers, and refactors local-cohorts evaluation to support a skip-unresolved mode used during dimension-based evaluation. Changes
Sequence Diagram(s)sequenceDiagram
participant Hs as Haskell Client
participant FFI as C FFI
participant RustIF as Rust Interface
participant Logic as superposition_types::logic
participant Core as superposition_core
Hs->>FFI: call c_get_satisfied_experiments(client, dimensions, context, prefix)
FFI->>RustIF: forward pointers (c_dimensions, c_context, filter_prefix)
RustIF->>Logic: parse c_dimensions -> HashMap<DimensionInfo>
RustIF->>Logic: evaluate_local_cohorts(_skip/unresolved_) -> evaluated Context
RustIF->>Core: call internal async get_satisfied / get_filtered with Context and prefixes
Core-->>RustIF: JSON result string
RustIF-->>FFI: return *mut c_char (result)
FFI-->>Hs: Haskell wrapper receives CString result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
ayushjain17
force-pushed
the
fix/exp_filter
branch
from
4f37494 to
6f7d65a
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: 5
🤖 Fix all issues with AI agents
In @clients/haskell/hs-exp-client/src/Client.hs:
- Line 126: The call to c_get_satisfied_experiments via withForeignPtr is
passing arguments in the wrong order (currently dimensions, client, context,
prefix); change the invocation inside withForeignPtr client so the arguments are
passed as (client, dimensions, context, prefix) to match the C signature for
c_get_satisfied_experiments; update the line where experiments is assigned (the
withForeignPtr client -> c_get_satisfied_experiments call) to swap the positions
of client and dimensions accordingly.
- Line 143: The call to c_get_filtered_satisfied_experiments inside the
withForeignPtr lambda passes arguments in the wrong order (dimensions, client,
filters, prefix); change the argument order so the C function is invoked as
(client, dimensions, filters, prefix) — update the invocation of
c_get_filtered_satisfied_experiments in the withForeignPtr client lambda (the
experiments binding) to pass the client first, then dimensions, filters, and
prefix.
- Around line 119-127: The CString allocated for dimensions in
getSatisfiedExperiments is not freed; update the cleanup so all allocated
CStrings are released: allocate prefix as before, then call cleanup with at
least [context, dimensions] and, if mbPrefix was Just, include prefix in that
list (i.e. use a conditional or build the list with maybe). Ensure the cleanup
happens after c_get_satisfied_experiments and uses the same local names
(context, dimensions, prefix) so no CString is leaked.
- Around line 134-144: The CString allocated for dimensions in
getFilteredSatisfiedExperiments is leaked because only filters is passed to
cleanup; update the cleanup invocation to free all allocated CStrings
(dimensions, filters, and prefix when non-null) and avoid shadowing the function
parameter by renaming the local CString variable or reusing a different name;
ensure the final cleanup call (the one currently calling cleanup [filters])
includes dimensions and prefix as appropriate so all allocated pointers are
freed after c_get_filtered_satisfied_experiments returns.
In @crates/superposition_core/src/experiment.rs:
- Line 276: The call to superposition_types::partial_apply is passing an
unnecessary reference for the local variable context, triggering clippy's
needless_borrow; change superposition_types::partial_apply(&exp.context,
&context) to superposition_types::partial_apply(&exp.context, context) so the
function receives the value (or appropriately borrowed type) without the
redundant & on context.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clients/haskell/hs-exp-client/src/Client.hsclients/haskell/hs-exp-client/src/Main.hscrates/experimentation_client/src/interface.rscrates/superposition_core/src/experiment.rscrates/superposition_types/src/config.rscrates/superposition_types/src/logic.rsheaders/libexperimentation_client.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/superposition_core/src/experiment.rscrates/superposition_types/src/logic.rscrates/superposition_types/src/config.rscrates/experimentation_client/src/interface.rs
🧬 Code graph analysis (4)
headers/libexperimentation_client.h (1)
crates/experimentation_client/src/interface.rs (1)
expt_get_filtered_satisfied_experiments(278-335)
crates/superposition_core/src/experiment.rs (2)
crates/superposition_types/src/logic.rs (2)
apply(40-42)partial_apply(44-49)crates/experimentation_client/src/lib.rs (4)
get_filtered_satisfied_experiments(188-235)experiments(154-176)experiments(195-225)filter_experiments_by_prefix(243-275)
crates/superposition_types/src/config.rs (1)
crates/superposition_types/src/logic.rs (1)
evaluate_local_cohorts_skip_unresolved(208-213)
crates/experimentation_client/src/interface.rs (2)
crates/superposition_types/src/logic.rs (3)
evaluate_local_cohorts(200-205)evaluate_local_cohorts_skip_unresolved(208-213)dimensions(224-228)crates/cac_client/src/interface.rs (1)
cstring_to_rstring(45-53)
🪛 GitHub Actions: CI Checks on PRs
crates/superposition_core/src/experiment.rs
[error] 276-276: clippy: this expression creates a reference which is immediately dereferenced by the compiler. Change this to: 'context'. See -D clippy::needless_borrow for details.
🪛 GitHub Actions: CI Nix
clients/haskell/hs-exp-client/src/Client.hs
[error] 126-126: Haskell compile error: Couldn't match type ‘Foreign.C.Types.CChar’ with ‘Arc_Client’ in call to c_get_satisfied_experiments. This occurs in the first argument when constructing 'experiments'.
[error] 143-143: Haskell compile error: Couldn't match type ‘Foreign.C.Types.CChar’ with ‘Arc_Client’ in call to c_get_filtered_satisfied_experiments. This occurs in the second argument when constructing 'experiments'.
🔇 Additional comments (7)
crates/superposition_types/src/config.rs (1)
19-19: LGTM!The switch to
evaluate_local_cohorts_skip_unresolvedis appropriate forfilter_by_dimensionssince it avoids defaulting unresolved local cohorts to "otherwise", which aligns with the partial apply semantics needed for dimension-based filtering.Also applies to: 363-364
crates/superposition_core/src/experiment.rs (1)
217-248: Verify the semantic difference betweenget_satisfied_experimentsandget_filtered_satisfied_experiments.
get_satisfied_experimentsnow uses strictapply()semantics (line 233), while the newget_filtered_satisfied_experimentsuses lenientpartial_apply()semantics (line 276). This means:
get_satisfied_experiments: Only returns experiments where context fully matchesget_filtered_satisfied_experiments: Returns experiments that could match (including ambiguous/partial matches)Please confirm this behavioral difference is intentional, as the function names don't clearly convey this distinction.
Also applies to: 250-292
clients/haskell/hs-exp-client/src/Main.hs (1)
27-29: LGTM!The test calls correctly pass an empty dimensions JSON object
"{}"as the new first argument after the client pointer. This is appropriate for testing purposes.headers/libexperimentation_client.h (1)
28-36: LGTM!The C header declarations correctly reflect the new
c_dimensionsparameter added to bothexpt_get_satisfied_experimentsandexpt_get_filtered_satisfied_experiments, with proper parameter ordering matching the Rust FFI implementation.crates/superposition_types/src/logic.rs (1)
144-213: LGTM!The refactoring is well-structured:
- Private
_evaluate_local_cohortswith theskip_unresolvedflag provides a clean core implementation- Public wrappers maintain API clarity and backward compatibility
- Documentation clearly explains the difference between the two variants
crates/experimentation_client/src/interface.rs (2)
219-275: LGTM!The
expt_get_satisfied_experimentsfunction correctly:
- Parses the new
c_dimensionsparameter- Uses
evaluate_local_cohorts(with "otherwise" defaulting) for strict matching semantics- Maintains proper error handling with early returns on parse failures
277-335: LGTM!The
expt_get_filtered_satisfied_experimentsfunction correctly:
- Parses the new
c_dimensionsparameter- Uses
evaluate_local_cohorts_skip_unresolved(no defaulting) for partial/lenient matching semantics- Maintains consistency with the corresponding client-side
get_filtered_satisfied_experimentslogic
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)
clients/haskell/hs-exp-client/src/Client.hs (1)
134-149: Fix critical argument order error in FFI call.Line 143 passes arguments in the wrong order to
c_get_filtered_satisfied_experiments. The Rust FFI expects(client, dimensions, filters, prefix), but the code currently passes(dimensions, client, filters, prefix).This type mismatch is confirmed by the pipeline failure: "Couldn't match type 'Foreign.C.Types.CChar' with 'Arc_Client'".
Additionally, line 136 shadows the
dimensionsparameter with a local binding of the same name, which reduces clarity.🐛 Proposed fix for argument order and variable shadowing
getFilteredSatisfiedExperiments :: ForeignPtr ExpClient -> String -> Maybe String -> Maybe String -> IO (Either Error Value) getFilteredSatisfiedExperiments client dimensions mbFilters mbPrefix = do - dimensions <- newCString dimensions + cDimensions <- newCString dimensions filters <- case mbFilters of Just filters' -> newCString filters' Nothing -> return nullPtr prefix <- case mbPrefix of Just prefix' -> newCString prefix' Nothing -> return nullPtr - experiments <- withForeignPtr client $ \client -> c_get_filtered_satisfied_experiments dimensions client filters prefix + experiments <- withForeignPtr client $ \c -> c_get_filtered_satisfied_experiments c cDimensions filters prefix _ <- cleanup [filters] if experiments == nullPtr
🤖 Fix all issues with AI agents
In @clients/haskell/hs-exp-client/src/Client.hs:
- Around line 119-132: getSatisfiedExperiments currently shadows the dimensions
parameter and passes arguments to c_get_satisfied_experiments in the wrong
order; change the local CString binding name (e.g. dimensionsC or dimsPtr)
instead of reusing "dimensions", allocate it with newCString, pass arguments to
c_get_satisfied_experiments as (client, dimensionsC, context, prefix) when using
withForeignPtr, and ensure all allocated CStrings (context, the renamed
dimensions CString, and prefix when non-null) are included in the cleanup list
so they are freed.
In @crates/superposition_core/src/experiment.rs:
- Line 276: Clippy flags a needless borrow in the call to
superposition_types::partial_apply; change the second argument from a borrowed
value to the value itself by passing context instead of &context so the compiler
doesn't immediately dereference it—update the call
superposition_types::partial_apply(&exp.context, context) (keeping exp.context
as a reference if intended) to remove the unnecessary & on context.
🧹 Nitpick comments (4)
crates/superposition_core/src/experiment.rs (1)
250-254: Consider adding documentation to clarify when to use this function.The new
get_filtered_satisfied_experimentsfunction serves a distinct purpose fromget_satisfied_experiments(partial vs. full context evaluation), but this difference isn't immediately clear from the function signature alone. Consider adding doc comments explaining:
- When to use this function vs.
get_satisfied_experiments- That it includes experiments with ambiguous/partial matches
- The implications of including experiments where context evaluation is incomplete
clients/haskell/hs-exp-client/src/Main.hs (1)
27-29: Consider demonstrating dimensions usage in the example.The example code passes empty JSON objects
"{}"for the newdimensionsparameter. While this works, it doesn't demonstrate the dimensions feature that was added. Consider updating the example to show how to pass actual dimension information, which would make it more educational for users learning the API.crates/experimentation_client/src/interface.rs (2)
234-258: Verify the silent fallback behavior for non-object context.Line 257 uses
unwrap_or_default()which silently converts a non-object context to an empty object before passing it toevaluate_local_cohorts. This could mask issues if the context is expected to always be an object type.Consider whether this silent fallback is intentional or if you should explicitly validate that the context is an object and return an error otherwise.
292-318: Verify the silent fallback behavior for non-object context.Similar to
expt_get_satisfied_experiments, line 317 usesunwrap_or_default()which silently converts a non-object context to an empty object. This could mask validation issues if the context is expected to always be an object type.Consider whether this silent fallback is intentional or if explicit validation would be more appropriate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clients/haskell/hs-exp-client/src/Client.hsclients/haskell/hs-exp-client/src/Main.hscrates/experimentation_client/src/interface.rscrates/superposition_core/src/experiment.rscrates/superposition_types/src/config.rscrates/superposition_types/src/logic.rsheaders/libexperimentation_client.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/superposition_core/src/experiment.rscrates/superposition_types/src/config.rscrates/superposition_types/src/logic.rscrates/experimentation_client/src/interface.rs
🧬 Code graph analysis (4)
crates/superposition_types/src/config.rs (1)
crates/superposition_types/src/logic.rs (1)
evaluate_local_cohorts_skip_unresolved(208-213)
crates/superposition_types/src/logic.rs (3)
clients/javascript/sdk/src/models/models_0.ts (1)
DimensionInfo(757-768)clients/python/sdk/superposition_sdk/models.py (1)
DimensionInfo(2515-2574)clients/python/bindings/superposition_bindings/superposition_types.py (1)
DimensionInfo(986-1013)
crates/experimentation_client/src/interface.rs (2)
crates/superposition_types/src/logic.rs (3)
evaluate_local_cohorts(200-205)evaluate_local_cohorts_skip_unresolved(208-213)dimensions(224-228)crates/cac_client/src/interface.rs (1)
cstring_to_rstring(45-53)
headers/libexperimentation_client.h (1)
crates/experimentation_client/src/interface.rs (1)
expt_get_filtered_satisfied_experiments(278-335)
🪛 GitHub Actions: CI Checks on PRs
crates/superposition_core/src/experiment.rs
[error] 276-276: Clippy: needlessly_borrow. this expression creates a reference which is immediately dereferenced by the compiler. Change '&context' to 'context' (see -D warnings).
🪛 GitHub Actions: CI Nix
clients/haskell/hs-exp-client/src/Client.hs
[error] 126-126: GHC type error: Couldn't match type ‘Foreign.C.Types.CChar’ with ‘Arc_Client’. In the first argument of ‘c_get_satisfied_experiments’, dimension/c_get_satisfied_experiments call uses client as CString instead of Arc_Client.
[error] 143-143: GHC type error: Couldn't match type ‘Foreign.C.Types.CChar’ with ‘Arc_Client’. In the second argument of ‘c_get_filtered_satisfied_experiments’, client parameter type mismatch (CString vs Arc_Client).
🔇 Additional comments (6)
crates/superposition_core/src/experiment.rs (1)
233-233: LGTM: Aligns with jsonlogic deprecation strategy.This change makes the non-jsonlogic evaluation path consistent with the overall migration to simple key-value conditions, as noted in the learnings.
Based on learnings, this is part of the broader effort to replace jsonlogic::apply with superposition_types::apply across the codebase.
crates/superposition_types/src/logic.rs (1)
144-213: Well-structured refactoring with clear API.The refactoring to extract common logic into
_evaluate_local_cohortswith askip_unresolvedparameter is clean and maintainable. The two public wrappers provide clear intent:
evaluate_local_cohorts: Default behavior, populates unresolved cohorts with "otherwise"evaluate_local_cohorts_skip_unresolved: Returns only computable cohortsThe documentation clearly explains the differences between the two variants.
crates/superposition_types/src/config.rs (1)
19-19: Appropriate use of the skip_unresolved variant.Using
evaluate_local_cohorts_skip_unresolvedinfilter_by_dimensionsis the correct choice here, as it ensures only dimensions that can be definitively computed from the provided data are used for filtering. This prevents contexts from being incorrectly filtered based on "otherwise" defaults for unresolved local cohorts.Also applies to: 363-364
headers/libexperimentation_client.h (1)
28-31: LGTM: Consistent C header updates.The addition of the
c_dimensionsparameter to bothexpt_get_satisfied_experimentsandexpt_get_filtered_satisfied_experimentsis consistent with the Rust FFI implementation. The parameter placement (after client, before c_context) maintains a logical ordering.Also applies to: 33-36
crates/experimentation_client/src/interface.rs (1)
14-17: LGTM: Imports align with new dimension-based evaluation logic.The imports correctly bring in the dimension evaluation functions and types needed for the new functionality.
clients/haskell/hs-exp-client/src/Client.hs (1)
57-60: LGTM: Foreign import signatures correctly updated.The function signatures correctly declare the new
c_dimensionsparameter in the proper position (second argument after the client pointer).
| _ => None, | ||
| } | ||
| } else { | ||
| superposition_types::partial_apply(&exp.context, &context) |
There was a problem hiding this comment.
Fix the needless borrow flagged by Clippy.
The pipeline failure indicates that &context should be context since the reference is immediately dereferenced by the compiler.
🔧 Proposed fix
} else {
- superposition_types::partial_apply(&exp.context, &context)
+ superposition_types::partial_apply(&exp.context, context)
.then(|| exp.clone())
}📝 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.
| superposition_types::partial_apply(&exp.context, &context) | |
| superposition_types::partial_apply(&exp.context, context) |
🧰 Tools
🪛 GitHub Actions: CI Checks on PRs
[error] 276-276: Clippy: needlessly_borrow. this expression creates a reference which is immediately dereferenced by the compiler. Change '&context' to 'context' (see -D warnings).
🤖 Prompt for AI Agents
In @crates/superposition_core/src/experiment.rs at line 276, Clippy flags a
needless borrow in the call to superposition_types::partial_apply; change the
second argument from a borrowed value to the value itself by passing context
instead of &context so the compiler doesn't immediately dereference it—update
the call superposition_types::partial_apply(&exp.context, context) (keeping
exp.context as a reference if intended) to remove the unnecessary & on context.
9f16f81 to
3444f6a
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/experimentation_client/src/interface.rs:
- Around line 234-242: The code calls cstring_to_rstring(c_dimensions) without
checking for null, causing undefined behavior if c_dimensions is NULL; add a
null check like the existing filter_prefix handling: if c_dimensions.is_null() {
return std::ptr::null_mut(); } before calling cstring_to_rstring, then proceed
to unwrap_safe! on the resulting Rust string and to
serde_json::from_str::<HashMap<String, DimensionInfo>>(...) as before so that
c_dimensions is safely handled.
- Around line 292-300: In expt_get_filtered_satisfied_experiments, add the same
null-check for the incoming c_dimensions pointer before calling
cstring_to_rstring/serde_json::from_str: if c_dimensions is null then return
std::ptr::null_mut() early (matching the prior fix), and only call
cstring_to_rstring(c_dimensions) and the subsequent unwrap_safe! on
serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()) when
c_dimensions is non-null so you avoid attempting to convert a null C string.
🧹 Nitpick comments (2)
clients/haskell/hs-exp-client/src/Main.hs (1)
27-28: Empty dimensions"{}"passed for testing.Both calls pass an empty JSON object
"{}"as the dimensions parameter. While this works for basic testing, it won't exercise the new dimension-based cohort evaluation logic. Consider adding test cases with actual dimension definitions to verify the new functionality end-to-end.crates/superposition_core/src/experiment.rs (1)
250-292: Newget_filtered_satisfied_experimentsfunction looks correct.The implementation correctly:
- Includes experiments with empty context unconditionally
- Uses
partial_applysemantics (includingAmbiguousoutcomes in jsonlogic mode)- Applies prefix filtering when provided
This mirrors the logic in
crates/experimentation_client/src/lib.rs(lines 153-175). Consider extracting the shared filtering logic into a helper to reduce duplication if this pattern is used in multiple places.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clients/haskell/hs-exp-client/src/Client.hsclients/haskell/hs-exp-client/src/Main.hscrates/experimentation_client/src/interface.rscrates/superposition_core/src/experiment.rscrates/superposition_types/src/config.rscrates/superposition_types/src/logic.rsheaders/libexperimentation_client.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/superposition_types/src/config.rscrates/superposition_core/src/experiment.rscrates/experimentation_client/src/interface.rscrates/superposition_types/src/logic.rs
🧬 Code graph analysis (4)
crates/superposition_types/src/config.rs (1)
crates/superposition_types/src/logic.rs (1)
evaluate_local_cohorts_skip_unresolved(208-213)
crates/superposition_core/src/experiment.rs (2)
crates/superposition_types/src/logic.rs (2)
apply(40-42)partial_apply(44-49)crates/experimentation_client/src/lib.rs (4)
get_filtered_satisfied_experiments(188-235)experiments(154-176)experiments(195-225)filter_experiments_by_prefix(243-275)
crates/experimentation_client/src/interface.rs (2)
crates/superposition_types/src/logic.rs (3)
evaluate_local_cohorts(200-205)evaluate_local_cohorts_skip_unresolved(208-213)dimensions(224-228)crates/cac_client/src/interface.rs (1)
cstring_to_rstring(45-53)
headers/libexperimentation_client.h (1)
crates/experimentation_client/src/interface.rs (1)
expt_get_filtered_satisfied_experiments(278-335)
⏰ 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). (5)
- GitHub Check: Provider Tests (py)
- GitHub Check: Testing
- GitHub Check: Provider Tests (js)
- GitHub Check: Provider Tests (rust)
- GitHub Check: Provider Tests (kotlin)
🔇 Additional comments (10)
headers/libexperimentation_client.h (1)
28-36: Breaking API change: newc_dimensionsparameter added.The addition of
c_dimensionsas the second parameter to bothexpt_get_satisfied_experimentsandexpt_get_filtered_satisfied_experimentsis consistent with the Rust interface changes. However, this is a breaking change for any existing C/FFI consumers.Ensure that all downstream consumers (e.g., other language bindings, existing integrations) are updated to pass the new dimensions parameter.
crates/superposition_types/src/config.rs (1)
19-19: Appropriate use ofevaluate_local_cohorts_skip_unresolvedfor filtering.Using the
_skip_unresolvedvariant here is correct for the filtering use case. When filtering by dimensions, we want to preserve only the dimensions that can be resolved from the input data rather than defaulting unresolved local cohorts to "otherwise".Also applies to: 363-364
crates/superposition_core/src/experiment.rs (1)
233-233: Correct change frompartial_applytoapplyfor strict matching.This aligns with the semantic difference:
get_satisfied_experimentsshould perform exact matching (usingapply), while the newget_filtered_satisfied_experimentsperforms partial matching (usingpartial_apply). Based on learnings, this also aligns with the migration away from jsonlogic-based conditions.crates/experimentation_client/src/interface.rs (1)
255-258: Semantic consistency between the two functions is correct.
expt_get_satisfied_experimentsusesevaluate_local_cohorts(unresolved → "otherwise")expt_get_filtered_satisfied_experimentsusesevaluate_local_cohorts_skip_unresolved(unresolved skipped)This aligns with the intended behavior where filtered experiments should only consider dimensions that can be resolved from the input.
Also applies to: 315-318
crates/superposition_types/src/logic.rs (2)
144-174: Clean refactoring withskip_unresolvedflag.The internal
_evaluate_local_cohortsfunction is well-structured with a clear early return path whenskip_unresolvedis true. This avoids setting unresolved local cohorts to "otherwise" in scenarios where partial/ambiguous matching is desired.
193-213: Good API design with public wrappers.The two public wrappers (
evaluate_local_cohortsandevaluate_local_cohorts_skip_unresolved) provide clear semantics while keeping the implementation DRY. The documentation adequately explains the difference in behavior.clients/haskell/hs-exp-client/src/Client.hs (4)
56-60: FFI declarations correctly updated with dimensions parameter.The foreign imports now match the updated C header declarations, with
c_dimensionsas the second parameter after the client pointer.
119-127:getSatisfiedExperimentscorrectly updated with dimensions parameter.The function now takes dimensions as a required
String, allocates it properly, passes it to the FFI call, and includes it in cleanup. The implementation is correct.
134-144:getFilteredSatisfiedExperimentscorrectly updated with dimensions parameter.The function properly handles the new dimensions parameter. Note that this function uses
Maybe StringformbFilters(the context) whilegetSatisfiedExperimentsuses a requiredStringforquery. This asymmetry may be intentional (filtered experiments can work without a context filter), but consider documenting this difference.
108-108: Good: cleanup includes the dimensions pointer.Memory management is correctly updated to free the dimensions CString allocation.
| let dimensions = unwrap_safe!( | ||
| cstring_to_rstring(c_dimensions), | ||
| return std::ptr::null_mut() | ||
| ); | ||
|
|
||
| let dimensions = unwrap_safe!( | ||
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | ||
| return std::ptr::null_mut() | ||
| ); |
There was a problem hiding this comment.
Missing null check for c_dimensions parameter.
Unlike filter_prefix which has explicit null handling, c_dimensions is passed directly to cstring_to_rstring without a null check. If a caller passes NULL for dimensions, this will cause undefined behavior when CStr::from_ptr is called on a null pointer.
Consider adding a null check similar to the filter_prefix handling, or document that c_dimensions must never be null.
🐛 Proposed fix
+ null_check!(c_dimensions, "Dimensions cannot be a null string", return std::ptr::null_mut());
+
let dimensions = unwrap_safe!(
cstring_to_rstring(c_dimensions),
return std::ptr::null_mut()
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let dimensions = unwrap_safe!( | |
| cstring_to_rstring(c_dimensions), | |
| return std::ptr::null_mut() | |
| ); | |
| let dimensions = unwrap_safe!( | |
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | |
| return std::ptr::null_mut() | |
| ); | |
| null_check!(c_dimensions, "Dimensions cannot be a null string", return std::ptr::null_mut()); | |
| let dimensions = unwrap_safe!( | |
| cstring_to_rstring(c_dimensions), | |
| return std::ptr::null_mut() | |
| ); | |
| let dimensions = unwrap_safe!( | |
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | |
| return std::ptr::null_mut() | |
| ); |
🤖 Prompt for AI Agents
In @crates/experimentation_client/src/interface.rs around lines 234 - 242, The
code calls cstring_to_rstring(c_dimensions) without checking for null, causing
undefined behavior if c_dimensions is NULL; add a null check like the existing
filter_prefix handling: if c_dimensions.is_null() { return std::ptr::null_mut();
} before calling cstring_to_rstring, then proceed to unwrap_safe! on the
resulting Rust string and to serde_json::from_str::<HashMap<String,
DimensionInfo>>(...) as before so that c_dimensions is safely handled.
| let dimensions = unwrap_safe!( | ||
| cstring_to_rstring(c_dimensions), | ||
| return std::ptr::null_mut() | ||
| ); | ||
|
|
||
| let dimensions = unwrap_safe!( | ||
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | ||
| return std::ptr::null_mut() | ||
| ); |
There was a problem hiding this comment.
Same null check issue for c_dimensions in expt_get_filtered_satisfied_experiments.
Apply the same null check fix here as well.
🐛 Proposed fix
+ null_check!(c_dimensions, "Dimensions cannot be a null string", return std::ptr::null_mut());
+
let dimensions = unwrap_safe!(
cstring_to_rstring(c_dimensions),
return std::ptr::null_mut()
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let dimensions = unwrap_safe!( | |
| cstring_to_rstring(c_dimensions), | |
| return std::ptr::null_mut() | |
| ); | |
| let dimensions = unwrap_safe!( | |
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | |
| return std::ptr::null_mut() | |
| ); | |
| null_check!(c_dimensions, "Dimensions cannot be a null string", return std::ptr::null_mut()); | |
| let dimensions = unwrap_safe!( | |
| cstring_to_rstring(c_dimensions), | |
| return std::ptr::null_mut() | |
| ); | |
| let dimensions = unwrap_safe!( | |
| serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()), | |
| return std::ptr::null_mut() | |
| ); |
🤖 Prompt for AI Agents
In @crates/experimentation_client/src/interface.rs around lines 292 - 300, In
expt_get_filtered_satisfied_experiments, add the same null-check for the
incoming c_dimensions pointer before calling
cstring_to_rstring/serde_json::from_str: if c_dimensions is null then return
std::ptr::null_mut() early (matching the prior fix), and only call
cstring_to_rstring(c_dimensions) and the subsequent unwrap_safe! on
serde_json::from_str::<HashMap<String, DimensionInfo>>(dimensions.as_str()) when
c_dimensions is non-null so you avoid attempting to convert a null C string.
3444f6a to
4d329b7
Compare
ayushjain17
force-pushed
the
fix/exp_filter
branch
from
4d329b7 to
3804013
Compare
ayushjain17
added
bug
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
✏️ Tip: You can customize this high-level summary in your review settings.