Evaluate targeting rules with semver string format (FF-1433) #38
Conversation
| { | ||
| operator: OperatorType.GTE, | ||
| attribute: 'version', | ||
| value: '1.0.0', |
There was a problem hiding this comment.
how about test data that makes sure sem ver comparisons are at play (vs string ones). Such as GTE 1.6.4 and then you can compare it against 1.12.3 or something like that
| valid as validSemver, | ||
| gt as semverGt, | ||
| lt as semverLt, | ||
| gte as semverGte, | ||
| lte as semverLte, |
There was a problem hiding this comment.
I appreciate the more explicit renaming
| if (value != null) { | ||
| switch (condition.operator) { | ||
| case getMD5Hash(OperatorType.GTE): | ||
| if (conditionValueType === OperatorValueType.SEM_VER) { |
There was a problem hiding this comment.
not your code, but seems kind of crazy w have to duplicate the switch statement. If easy, consider cleaning it up by changing from a switch to a Map or something.
There was a problem hiding this comment.
Aaron do you mean the duplication between the evaluateCondition and evaluateObfuscatedCondition functions?
There was a problem hiding this comment.
Yeah (had to expand the PR to confirm). Whenever I see the same business logic change being made in multiple places in a PR like that it triggers some spidey senses 🕷️ 🕸️
There was a problem hiding this comment.
Aaron I gave it a shot but really didn't like the way the code turned out - a complicating factor is the difference behavior in the lt, gt.. and the one of, not on off branches. Want to pair for a few minutes?
labels: mergeable
Fixes: #issue
Motivation and Context
RAC API will start returning targeting rule values containing semver strings (https://github.com/Eppo-exp/eppo/pull/8697).
Our SDKs need to be able to parse these strings and perform rule evaluation on them.
Description
The operations
lt, gt, lte, gtenow only apply to numeric values; insert a higher priority step to check if the value is a semver string; if so apply the comparison with it. Otherwise fall through and attempt to evaluate using numerics.behavior of un-upgraded SDKs
They will continue to assume that only numeric values will be present for the
lt, gt, lte, gteoperations - thecompareNumberfunction will return false if both sides of the comparison are not numbers.How has this been tested?
New unit tests.