-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fraction operation - add validations for nil values (#1285) #1294
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
…e#1285) Signed-off-by: Baalekshan <[email protected]>
@@ -72,13 +72,13 @@ func parseFractionalEvaluationData(values, data any) (string, []fractionalEvalua | |||
return bucketBy, feDistributions, nil | |||
} | |||
|
|||
func parseFractionalEvaluationDistributions(values []any) ([]fractionalEvaluationDistribution, error) { | |||
func parseFractionalEvaluationDistributions(values []interface{}) ([]fractionalEvaluationDistribution, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this does not solve the underlying issue . any
& interface{}
are type aliasses.
You have to validate the content of the array and ignore nil values to continue.
Besides, you need to add a unit test to validate this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if values[i] == nil {
continue
}
Will this be good if I included it inside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an option, but I would avoid adding the check into the parseFractionalEvaluationDistributions
. The parameters here should already be filtered and validated.
The fix should focus on parsing of the distributions [1]. If the first parameter is not a string, then we should check it's non-nil
[1] - https://github.com/open-feature/flagd/blob/core/v0.9.0/core/pkg/evaluator/fractional.go#L58-L64
@Baalekshan thank you for your interest in this issue. I had to fix this as we were preparing the next release of flagd [1]. You can check the fix I did here [2]. I hope you find it helpful and continue to contribute in future :) |
@Kavindu-Dodan Yeah I will look for contributing in future. |
This PR
adds validation for nil values by using []interfaces{} instead of []any
Related Issues
Fixes #1285
Notes
Follow-up Tasks
How to test