fix(number validation): Don't track NaN, Infinity, -Infinity, or > 2^53 #249

mnoman09 · 2018-12-21T15:12:11Z

Summary

Allow targeting Long and float values
Don't target Values > 2^53.
Modified isValidNumber to check if the number is smaller then 2^53 and checking instanceOf float and long attribute value.

Test plan

coveralls · 2018-12-21T15:21:28Z

Pull Request Test Coverage Report for Build 804

17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.05%) to 89.721%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchType.java	4	7	57.14%

Totals
Change from base Build 801:	-0.05%
Covered Lines:	2706
Relevant Lines:	3016

💛 - Coveralls

coveralls · 2018-12-21T15:21:29Z

Pull Request Test Coverage Report for Build 861

18 of 19 (94.74%) changed or added relevant lines in 5 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 89.545%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/internal/AttributesUtil.java	8	9	88.89%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/match/ExactNumberMatch.java	1	87.5%
core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java	2	77.78%
core-api/src/main/java/com/optimizely/ab/config/audience/match/LTMatch.java	2	77.78%

Totals
Change from base Build 859:	-0.2%
Covered Lines:	2715
Relevant Lines:	3032

💛 - Coveralls

msohailhussain · 2018-12-21T23:57:25Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchType.java

+            return Math.abs((Long) conditionValue) <= Math.pow(2, 53);
+        } else if (conditionValue instanceof Float) {
+            Float value = ((Number) conditionValue).floatValue();
+            return !(value.isNaN() || value.isInfinite() || value > Math.pow(2, 53));


Both validations look same for double and float, please consolidate into one.

msohailhussain · 2018-12-21T23:58:24Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchType.java

        } else if (conditionValue instanceof Double) {
            Double value = ((Number) conditionValue).doubleValue();
-            return !(value.isNaN() || value.isInfinite());
+            return !(value.isNaN() || value.isInfinite() || value > Math.pow(2, 53));


Math.abs missing or double doesn't take -ve values?

msohailhussain · 2018-12-22T00:02:38Z

core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java

@@ -218,4 +217,19 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,

        return attributesList;
    }
+
+    private static boolean isValidNumber(Object conditionValue) {


https://github.com/optimizely/java-sdk/pull/249/files#diff-922db348b1c4d7acd0c13f647f829eb6R73

we have two methods with same name, same functionality, can't it go into single helper method.

Please add documentation of this method.

msohailhussain · 2018-12-22T00:03:19Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+     * causing an exception to be thrown and passing only the valid attributes.
+     */
+    @Test
+    public void createConversionEventIgnoresInvalidAndAcceptsValidValueOfValidTypeAttributes() {


Please add -ve value cases.

too long method name, please make it bit shorter.

msohailhussain · 2018-12-22T00:08:16Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchType.java

@@ -72,10 +72,15 @@ public static MatchType getMatchType(String matchType, Object conditionValue) {

    private static boolean isValidNumber(Object conditionValue) {
        if (conditionValue instanceof Integer) {
-            return Math.abs((Integer) conditionValue) <= 1e53;
+            return Math.abs((Integer) conditionValue) <= Math.pow(2, 53);


Please add documentation of this method.

…tion and tests of -ve numbers

core-api/src/main/java/com/optimizely/ab/internal/AttributesUtil.java

# Conflicts: # core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java

nchilada · 2019-01-15T22:46:01Z

Hi, sorry for the delayed code review. Could we please revive this PR? In addition to merging master (which will include changes from #228), I think we need to

refactor slightly such that we have a single isValidNumber implementation for both targeting and tracking
make sure isValidNumber enforces a 2^53 size limits for floating-point numbers and not just for integers. Looks like we missed that in fix(number validation): Don't target NaN, Infinity, -Infinity, or > 2^53 #228!

aliabbasrizvi

You will have to rebase and adjust the new test.

aliabbasrizvi · 2019-01-16T00:54:23Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+     * causing an exception to be thrown and passing only the valid attributes.
+     */
+    @Test
+    public void createConversionEventIgnoresInvalidAcceptValidValOfValidAttr() {


I think you will have to adjust this test to accommodate changes from: #250

The expectation is that there will be no decisions and enrich_decisions should be set to true

aliabbasrizvi · 2019-01-16T00:54:34Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+     * causing an exception to be thrown and passing only the valid attributes.
+     */
+    @Test
+    public void createConversionEventIgnoresNegativeInvalidAndAcceptsValidValueOfValidTypeAttributes() {


Similar feedback as above.

nchilada · 2019-01-19T00:34:15Z

Thanks for the updates! The new code looks good. But I just realized that when we evaluate targeting conditions, we probably ought to call isValidNumber on the user attribute value and not just on the condition value. We're only doing the latter right now.

nchilada

This looks good to me. Thanks for updating!

nchilada · 2019-01-24T19:44:37Z

@mnoman09 do you think you could pull in latest master? I think we may have time to approve and merge this now. Sorry for the delays!

aliabbasrizvi

LGTM other than the nits.

Please address.

aliabbasrizvi · 2019-01-24T19:59:43Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+
+        // Bucket to the first variation for all experiments. However, only a subset of the experiments will actually
+        // call the bucket function.
+        for (Experiment experiment : allExperiments) {


You don't need this for loop now right?

aliabbasrizvi · 2019-01-24T20:01:08Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+        assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getKey(), boolAttribute.getKey());
+        assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getValue(), validBoolAttribute);
+        assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getKey(), validFloatAttribute.getKey());
+        //In below condition I am checking Value of float with double value because impression gets visitors from json so that converts it into double


nit. Change this to:

In the condition below we are checking Value of float with double value because impression gets visitors from JSON so that converts it into double

aliabbasrizvi · 2019-01-24T20:01:38Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+            attributes,
+            eventTagMap);
+
+        EventBatch impression = gson.fromJson(conversionEvent.getBody(), EventBatch.class);


Why are you calling this impression? Shouldn't this be called conversion?

aliabbasrizvi · 2019-01-24T20:02:26Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+
+        // Bucket to the first variation for all experiments. However, only a subset of the experiments will actually
+        // call the bucket function.
+        for (Experiment experiment : allExperiments) {


Again. This is not needed right.

aliabbasrizvi · 2019-01-24T20:02:46Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java

+            attributes,
+            eventTagMap);
+
+        EventBatch impression = gson.fromJson(conversionEvent.getBody(), EventBatch.class);


This should technically be called conversion and not impression right?

# Conflicts: # core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java # core-api/src/main/java/com/optimizely/ab/config/audience/match/LTMatch.java

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53

1c7b7f3

msohailhussain reviewed Dec 21, 2018

View reviewed changes

msohailhussain reviewed Dec 22, 2018

View reviewed changes

mnoman09 and others added 3 commits December 24, 2018 16:41

Refactored code added Attributes util class to add isValidNumber func…

b16dc7b

…tion and tests of -ve numbers

Merge branch 'master' into mnoman/NAN-INFCheck

333e3c9

Merge branch 'master' into mnoman/NAN-INFCheck

f43110b

msohailhussain reviewed Jan 2, 2019

View reviewed changes

core-api/src/main/java/com/optimizely/ab/internal/AttributesUtil.java Show resolved Hide resolved

Updated headers

18d54d3

msohailhussain requested review from mikeproeng37 and a team January 3, 2019 05:45

Merge branch 'main_master' into mnoman/NAN-INFCheck

ef35916

# Conflicts: # core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java

nchilada changed the title ~~feat(attribute_value): Don't target NAN, INF, -INF and > 2^53~~ fix(number validation): Don't track NaN, Infinity, -Infinity, or > 2^53 Jan 15, 2019

aliabbasrizvi suggested changes Jan 16, 2019

View reviewed changes

mnoman09 and others added 2 commits January 18, 2019 13:39

Merge branch 'master' into mnoman/NAN-INFCheck

d8e3143

removed decision service and experiment map from event factory tests

0800988

Feedback comments resolved. (Chk-in on behalf of Noman)

2257b5a

nchilada approved these changes Jan 22, 2019

View reviewed changes

aliabbasrizvi approved these changes Jan 24, 2019

View reviewed changes

mnoman09 added 2 commits January 25, 2019 12:04

Merge branch 'main_master' into mnoman/NAN-INFCheck

0e6b4ff

# Conflicts: # core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java # core-api/src/main/java/com/optimizely/ab/config/audience/match/LTMatch.java

Nit changes

efc80d5

aliabbasrizvi merged commit ea63898 into master Jan 25, 2019

aliabbasrizvi deleted the mnoman/NAN-INFCheck branch January 25, 2019 22:04

fix(number validation): Don't track NaN, Infinity, -Infinity, or > 2^53 #249

fix(number validation): Don't track NaN, Infinity, -Infinity, or > 2^53 #249

Uh oh!

Conversation

mnoman09 commented Dec 21, 2018

Summary

Test plan

Uh oh!

coveralls commented Dec 21, 2018

Pull Request Test Coverage Report for Build 804

💛 - Coveralls

Uh oh!

coveralls commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 861

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nchilada commented Jan 15, 2019

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada commented Jan 19, 2019

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

nchilada commented Jan 24, 2019

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Dec 21, 2018 •

edited

Loading