fix: handle missing/null targeting keys in fractional evaluator#1949
Conversation
dosubot
Bot
added
the
size:XS
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces safety checks in the fractional evaluator to handle cases where distributions or the targeting key are missing or null. Specifically, it adds nil checks for feDistributions and the targetingKey in dataMap, ensuring the evaluator returns early instead of potentially encountering errors. Additionally, new test cases have been added to verify the behavior when the targeting key is null, missing, or empty. I have no feedback to provide.
|
Hey @utafrali - can you do a |
| }}, | ||
| flagKey: "headerColor", | ||
| context: map[string]any{ | ||
| "targetingKey": "", |
There was a problem hiding this comment.
Hi, this wasn't defined in the issue yet when you opened your PR, but the suggested fix is that targetingKey = "" should be treated the same as targetingkey = nil and a non-existent targeting key and therefore result in falling back to the default. :)
There was a problem hiding this comment.
+1, see the testbed test for this:
https://github.com/open-feature/flagd-testbed/blob/484ada4dd979f663214c024f3cb601141328a1b5/gherkin/targeting.feature#L376-L381
|
Done, pushed the fix. |
dosubot
Bot
added
size:S
| return nil | ||
| } | ||
|
|
||
| if feDistributions == nil { |
There was a problem hiding this comment.
Can this ever be the case?
Edit: Oh yes, I see, with your changes below.
| return "", nil, nil | ||
| } | ||
| targetingKey, ok := dataMap[targetingKeyKey].(string) | ||
| if !ok { |
There was a problem hiding this comment.
Just a suggestion: You could also simply add a check here for an empty string.
leakonvalinka
left a comment
leakonvalinka
left a comment
There was a problem hiding this comment.
I left a feedback comment, but just as a suggestion so not blocking of course!
|
Done, pushed the fix. |
This still need to be done for both commits. @utafrali |
|
Done, pushed the fix. |
toddbaert
force-pushed
the
fix/issue-1948-bug-missing-targeting-key-handling-diffe
branch
from
0577ad0 to
2f03138
Compare
|
4 participants
Fixes #1948
The fractional evaluator wasn't handling missing or null targeting keys the same way other SDKs did. Added nil checks to return the default variant when the key is missing or null. Also added tests for the three cases: missing key, null key, and empty string.