Config v6 #39
Config v6 #39
Conversation
Added Comparator enum Removed:IS ONE OF, IS NOT ONE OF Changed: CONTAINS, DOES_NOT_CONTAIN Added: DATE_BEFORE, DATE_AFTER, HASHED_EQUALS, HASHED_NOT_EQUALS, HASHED_STARTS_WITH, HASHED_ENDS_WITH, HASHED_ARRAY_CONTAINS, HASHED_ARRAY_NOT_CONTAINS
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Nice job! 👍 The PR is pretty much complete, what needs to be set straight are mostly edge cases.
I found only one thing that was missed: ConfigCatClient.getKeyAndValueFromSettingsMap should also be updated to correctly traverse the new config model. See also: https://github.com/configcat/python-sdk/blob/master/configcatclient/configcatclient.py#L188
| evaluateLogger.logTargetingRuleConsequence(targetingRule, error, conditionsEvaluationResult, newLine); | ||
| } | ||
| if (error != null) { | ||
| throw new RolloutEvaluatorException(error); |
There was a problem hiding this comment.
low: Using exceptions to control flow is generally considered to be an anti-pattern (or at least, a code smell) as it's explained e.g. in these discussions:
- https://softwareengineering.stackexchange.com/questions/273029/is-throwing-an-exception-an-anti-pattern-here
- https://softwareengineering.stackexchange.com/questions/189222/are-exceptions-as-control-flow-considered-a-serious-antipattern-if-so-why
- https://stackoverflow.com/questions/52331428/avoid-using-exceptions-as-flow-control
So, I'd prefer an implementation that doesn't use exceptions for these expected error cases but some solution like this, which would result in a cleaner and more performant code. However, I understand that it would require a pretty extensive refactoring at this point, so I can live with the current solution if other reviewers are ok with it.
There was a problem hiding this comment.
Yeah. That is a tricky one.
Part of me agrees with you. It is not nice when we catch the exception we just throw. I didn't want to use an extra class as a return type, and the exceptions worked fine until I added the EvaluateLogger logic. Most of the time the logger logic requires the catch and the rethrows.
At that point, I choose to go with the exceptions and do not refactor the code, but if necessary, I can do the refactor. No problem.
Let's see what the others think of it.
There was a problem hiding this comment.
For now, I think we can go with the exceptions, but please create a note for later to investigate how much effort would be to do a refactor that switches to an exception-less control flow. Thanks!
| package com.configcat; | ||
|
|
||
| /** | ||
| * Describes the Rollout Evaluator User Condition Comparators. |
There was a problem hiding this comment.
low: Please consider synchronizing intellisense docs with other SDKs which has been already reviewed and approved by the UX team. For example: https://github.com/configcat/.net-sdk/tree/b6361d0c714f1198e42b914ef8633c144b4786ea/src/ConfigCatClient/Models
There was a problem hiding this comment.
I updated the Javadoc of the classes. There are some differences in the structure. The UX team (@configcat/ux-qa-team) could check these different docs.
The list of the docs:
- UserCondition - stringValue, doubleValue, stringArrayValue : https://github.com/configcat/java-sdk/pull/39/files#diff-644f0ac679954949ceb76372e54ad134a5162b634016b12de5220d4add3bc7f8
- SettingsValue: https://github.com/configcat/java-sdk/pull/39/files#diff-68328e3bb4b9d4d35d5e2fc6cbba968bcb8ff4f42cdc3993bfbf1c53ba88ef70
- SegmentCondition - segmentIndex: https://github.com/configcat/java-sdk/pull/39/files#diff-41d9254ce4ea33dec847eed4437713cb270935be8867fee7322b4fcd9f258a52
- Preferences: https://github.com/configcat/java-sdk/pull/39/files#diff-9c451063fa50bda96b8bb4c0a37e9381bd0917eda07ef00341191b8542c32b47
- PercentageOption - value, variationId: https://github.com/configcat/java-sdk/pull/39/files#diff-3dbbe6d93e4c28ea83f44eb9cbddb5e8f0432d990806ee34331c786cf8427f45
Add Type validation tests. Remove Object from accepted types.
| evaluateLogger.logTargetingRuleConsequence(targetingRule, error, conditionsEvaluationResult, newLine); | ||
| } | ||
| if (error != null) { | ||
| throw new RolloutEvaluatorException(error); |
There was a problem hiding this comment.
For now, I think we can go with the exceptions, but please create a note for later to investigate how much effort would be to do a refactor that switches to an exception-less control flow. Thanks!
|
Describe the purpose of your pull request
Features/improvements:
config_v6.json) and update config modelIEvaluationDetails.matchedTargetingRule/matchedPercentageOptionproperties (rename + set combinations correctly)ConfigCatClient.getKeyAndValueFromSettingsMapTests:
IEvaluationDetails.matchedTargetingRule/matchedPercentageOptionproperties (rename + set combinations correctly)Related improvements/fixes:
Dynamically typed languages should log a warning when default value doesn't match the setting type (see https://trello.com/c/Le6vimGu)We should go to the cache in all polling modes instead of using the in memory variable (see https://trello.com/c/rreKm64A)(already released)Breaking changes:
Configand related ConfigCat models to support v6.EvaluationDetailsgetMatchedEvaluationRulemethod togetMatchedTargetingRuleandgetMatchedEvaluationPercentageRuletogetMatchedPercentageOption.Requirement checklist (only if applicable)