feat: allowing null/missing default values #1511
Conversation
Signed-off-by: Rahul Baradol <rahul.baradol.14@gmail.com>
There was a problem hiding this comment.
Thanks for the contribution, @Rahul-Baradol.
I've added some comments on things that cached my attention.
Wondreing what the Gherkin tests will reveal.
| @@ -203,6 +204,12 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC | |||
| // check variant existence | |||
| Object value = flag.getVariants().get(resolvedVariant); | |||
| if (value == null) { | |||
| if (resolvedVariant.isEmpty()) { | |||
There was a problem hiding this comment.
The resolved variant will probably not be an empty string, but null. Especially, if the defaultVariant property is not set in the data source.
There was a problem hiding this comment.
Surprisingly, the value is coming out be empty string even if the defaultVariant is not set in the config 🤔
| if (resolvedVariant.isEmpty()) { | ||
| String message = String.format("no default variant found in flag with key %s", key); | ||
| log.debug(message); | ||
| throw new FlagNotFoundError(message); |
There was a problem hiding this comment.
Support Explicit Code Default Values in flagd Configuration will be a default code flow, not an exceptional case.
Throwing an exception can be problematic in performance critical use-cases.
Consider returning a ProviderEvaluaitonResult with proper reason an error code instead of throwing an exception.
The same holds for the debug log message. The message is surely valuable. Consider checking if debug is enabled before constructing a new message. Or use a log.debug() overload that accepts format and arguments but does not concatenate the string beforehand.
There was a problem hiding this comment.
According to the spec, there should be no logging on hot code paths
| @@ -203,6 +204,12 @@ private <T> ProviderEvaluation<T> resolve(Class<T> type, String key, EvaluationC | |||
| // check variant existence | |||
| Object value = flag.getVariants().get(resolvedVariant); | |||
| if (value == null) { | |||
| if (resolvedVariant.isEmpty()) { | |||
| String message = String.format("no default variant found in flag with key %s", key); | |||
There was a problem hiding this comment.
The message could be misleading. The resolved variant could also be the result of a misconfigured targeting rule. E.g., if a targeting rule specifies an empty variant.
There was a problem hiding this comment.
Yeah, actually!
Need to figure out another way to solve this.
|
@Rahul-Baradol if it's any help, here is my JS implementation of the same thing. I'm happy to add the e2e tests here once your implementation is complete, so you can ignore that part. |
|
FYI, I've updated the title to remove the |
This PR
FLAG_NOT_FOUND,Related Issues
Fixes #1438