fix(fractional): support nested JSON Logic, boolean names, negative weight clamping, null key error#44
Open
fix(fractional): support nested JSON Logic, boolean names, negative weight clamping, null key error#44
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the fractional operator by allowing bucket names to be any JSON scalar (string, boolean, or number) and ensuring that nested JSON Logic within bucket definitions is evaluated. It also implements clamping for negative weights and refines the bucketing logic to handle zero-weight scenarios more gracefully. Review feedback recommends optimizing performance by avoiding an unnecessary clone of context data and refactoring the manual loop in the fractional function to use more idiomatic Rust iterators.
…eight clamping, null key error
Three related fixes to the fractional operator:
1. Evaluate inner bucket elements as JSON Logic
Previously bucket name/weight inner elements were taken as literal JSON
values after the outer array was evaluated. Now each inner element is
evaluated through the JSON Logic evaluator, enabling nested expressions:
- [{'if': [{'==': [{'var': 'tier'}, 'premium']}, 'premium', 'standard']}, 50]
- [{'var': 'color'}, 50]
and boolean literals used when fractional is a condition:
- [false, 0], [true, 100]
2. Clamp negative weights to zero
Previously a negative weight triggered a hard error. Negative values are
now clamped to 0 so the bucket participates in the name list but receives
no traffic, matching the expected spec behavior.
3. Return error on null bucket key expression
When the first argument is a JSON Logic expression that evaluates to null
or a non-string value, return an error so the flag engine falls back to
the defaultVariant. The existing implicit fallback (flagKey+targetingKey)
is preserved only for array literals as the first argument (no-seed form).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
- Avoid unnecessary .clone() on context data by holding root reference - Replace manual while-loop with idiomatic chunks(2) iterator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
95582ba to
8506dc1
Compare
Update two callers in integration_tests.rs that assumed fractional() returned String — now it returns serde_json::Value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
8506dc1 to
8ae755d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three related fixes to the
fractionaloperator uncovered by new testbed scenarios (flagd-testbed PRs #341-#357).1. Evaluate inner bucket elements as JSON Logic
Previously, the outer
[name_expr, weight_expr]array was evaluated but its inner elements were passed as-is tofractional(). Now each inner element is evaluated through the JSON Logic evaluator, enabling nested expressions as variant names/weights and boolean literals as bucket names.2. Clamp negative weights to zero
Negative weights are now clamped to 0 so the bucket participates in the list but receives no traffic — matching expected spec behavior.
3. Return error on null bucket key expression
When the first argument is a JSON Logic expression that evaluates to null or non-string, return an error so the flag engine falls back to
defaultVariant.All existing tests pass with zero regressions. New unit tests added for each fix.