[NO TASK] Fix oneOf comparisons for both Booleans and Numbers (#14) #54
Conversation
There was a problem hiding this comment.
Great catch @simondale00! Apologies for the SDK deviating from the documented behavior. Love the test cases you made that will prevent such a regression in the future 💪
My one suggestion is to add a comment to toString() indicating it is used for IN and NOT IN comparisons, and changes should be made with caution. (As toString() typically is not used in business logic)
| @@ -118,7 +118,19 @@ public String toString() { | |||
| case STRING: | |||
| return this.stringValue; | |||
| case NUMBER: | |||
| return this.doubleValue.toString(); | |||
| // By default, `String.valueOf(<double>)` will include at least one decimal place. | |||
There was a problem hiding this comment.
🎉 Appreciate the comment including the documentation link!
| @@ -153,9 +153,9 @@ private static boolean evaluateCondition( | |||
| return Compare.compareRegex(value.stringValue(), | |||
| Pattern.compile(condition.value.stringValue())); | |||
| case ONE_OF: | |||
| return Compare.isOneOf(value.stringValue(), condition.value.arrayValue()); | |||
| return Compare.isOneOf(value.toString(), condition.value.arrayValue()); | |||
| case NOT_ONE_OF: | ||
| return !Compare.isOneOf(value.stringValue(), condition.value.arrayValue()); | ||
| return !Compare.isOneOf(value.toString(), condition.value.arrayValue()); |
| @@ -71,6 +71,44 @@ public void addOneOfCondition(Rule rule) { | |||
| addConditionToRule(rule, condition); | |||
| } | |||
|
|
|||
| public void addOneOfConditionWithIntegers(Rule rule) { | |||
| addConditionToRule(rule, condition); | ||
| } | ||
|
|
||
| public void addOneOfConditionWithDoubles(Rule rule) { |
| addConditionToRule(rule, condition); | ||
| } | ||
|
|
||
| public void addOneOfConditionWithBoolean(Rule rule) { |
Done! Thanks for the quick feedback.
I waffled on putting the change here for this exact reason. TBH I'd be just as happy to move this to a helper function. Longer term it also may be worth exploring the behaviour of |
I thought about this too but realized we'd just be re-implemting
Agree. I think at some point we may want to consider making it loose comparisons (e.g., Thanks for iterating and adding the comment! Happy to merge, we just need to bump the version in <version>2.4.3</version> |
context
Fixes
oneOfandnotOneOfevaluation for Number and Boolean types.description
The targeting rule documentation states that oneOf is evaluated by casting booleans and numbers to a string.
However, there was a recent update to the SDK that changed the implementation of stringValue(); The method now returns
nullwhen theEppoValueis either a number or boolean type.The proposed change does the following:
toString()instead ofstringValue()when evaluatingoneOfornotOneOfdoubleis used internally, an intended integer value of2will be cast to a string as2.0.toString()method to better represent the number type.