fix(nullfilter): filters out attributes with null values #214

mnoman09 · 2018-09-11T11:26:05Z

This will ignore attributes with null values on creation of impression or conversion event.

coveralls · 2018-09-11T11:32:54Z

Pull Request Test Coverage Report for Build 635

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 88.997%

Totals
Change from base Build 606:	0.02%
Covered Lines:	2386
Relevant Lines:	2681

💛 - Coveralls

nchilada · 2018-09-11T16:48:54Z

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

@@ -168,6 +168,11 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
        List<Attribute> attributesList = new ArrayList<Attribute>();

        for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+            //Ignore null value attribute
+            if (entry.getValue() == null) {


Can we whitelist strings, numbers (probably just ints and doubles for now, but the team might need to discuss more), and booleans rather than blacklisting some of the values we don't want to send? I should mention that OASIS-3537 has been updated and might not have mentioned that initially. 😬

@nchilada this is related to OASIS-3535 not for OASIS-3537

Oh, interesting. @mikeng13 can you clarify the difference between 3535 and 3537?

As per my understanding, this is only related to NULL values, while OASIS-3537 is more related to filtering out all unsupported values but we have hold-off for OASIS-3537.

I've deprecated 3537 in favor of 3535. So the requirement from @nchilada for this particular PR is right. We should filter out values that aren't strings, numbers, or booleans. This includes filtering out NULLs as well.

…r null than ignore it

nchilada · 2018-09-12T17:55:07Z

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

@@ -168,6 +168,15 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
        List<Attribute> attributesList = new ArrayList<Attribute>();

        for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+            //Ignore null value attribute


Slightly out of date. Maybe change to

// Filter down to the types of values we're allowed to track. // Don't allow Longs, BigIntegers, or BigDecimals - they /can/ theoretically be serialized as JSON numbers // but may take on values that can't be faithfully parsed by the backend. // https://github.com/optimizely/avro-schemas/blob/baeba19a7bd227aae67026474aec239110c5801d/event_batch/src/main/avro/EventBatch.avsc#L22

?

@mikecdavis @thomaszurkan-optimizely @mikeng13 @ceimaj can you review what I'm proposing as the supported data types?

nchilada · 2018-09-12T17:55:38Z

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

@@ -168,6 +168,15 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
        List<Attribute> attributesList = new ArrayList<Attribute>();

        for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+            //Ignore null value attribute
+            if (entry.getValue() == null ||


Technically this check isn't necessary since null instanceof ... evaluates to false, but it's nice to be explicit in the reference SDK since we may be porting to languages that behave differently. Good thinking!

nchilada · 2018-09-12T18:00:02Z

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

+            assertNotSame(feature.getValue(),  null);
+        }
+    }
+


Can we include explicit unit tests to make sure

Integers and Decimals are both tracked. I'm not sure if we can test them distinctly via the compatibility suite, since attribute values have to be serialized via JSON.

Longs, BigIntegers, and BigDecimals aren't tracked? We definitely can't test these via the compatibility suite.

Maybe test that Booleans are also trackable (in addition to Strings, which I assume are already tested) but I'm less concerned since those can theoretically be covered by the compatibility suite.

msohailhussain · 2018-09-12T20:19:13Z

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

+                    (!(entry.getValue() instanceof String) &&
+                    !(entry.getValue() instanceof Integer) &&
+                    !(entry.getValue() instanceof Double) &&
+                    !(entry.getValue() instanceof Boolean))) {


Just a suggestion, Usually don't like too many Not conditions. so why we can't convert it into one not.
!(e.getValue() instanceof String || e.getValue() instanceOf Int .... ) should be equal to the above condition.
@nchilada what do u suggest?

Seems reasonable to me!

…ting passed in event factory building list * Added Unit test to verify bigdecimal and bigInteger are not getting passed in event factory building list

nchilada · 2018-09-13T18:38:46Z

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

+            assertNotSame(feature.getValue(), null);
+        }
+    }
+


Should we test attribute selection for conversion events too?

BTW I think we could combine createImpressionEventIgnoresInvalidAttributes, createImpressionEventWithIntegerDecimalBoolAndStringAttributes , createImpressionEventIgnoresNullAttributes - that'd reduce complexity while also confirming that we merely ignore invalid attributes rather than dropping all attributes when one or two are invalid.

Lets leave these Unit Tests. I added one more unit test which is making ConversionEvent and in that unit test we are passing valid and invalid attributes in same attributes map, we are verifying that only valid attributes are getting passed and others are getting ignored. I think that will cover the condition you are asking to verify. So is that ok?

Okay, that works too. Thanks!

nchilada · 2018-09-13T18:42:55Z

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

@@ -168,6 +168,16 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
        List<Attribute> attributesList = new ArrayList<Attribute>();

        for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+            // Filters down to the types of values that are allowed.


Can we link to https://github.com/optimizely/avro-schemas/blob/baeba19a7bd227aae67026474aec239110c5801d/event_batch/src/main/avro/EventBatch.avsc#L22 since that's the source of truth? Or actually https://developers.optimizely.com/x/events/api/#Attribute_ might be preferable since not everyone can view the avro-schemas repo

* Added Unit Test To test with ConversionEvent and verified that valid attributes are getting passed and invalid are getting ignored

mikeproeng37 · 2018-09-18T21:03:09Z

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

+            // Filter down to the types of values we're allowed to track.
+            // Don't allow Longs, BigIntegers, or BigDecimals - they /can/ theoretically be serialized as JSON numbers
+            // but may take on values that can't be faithfully parsed by the backend.
+            // https://github.com/optimizely/avro-schemas/blob/baeba19a7bd227aae67026474aec239110c5801d/event_batch/src/main/avro/EventBatch.avsc#L22


This is a private repo. Please don't mention it here.

Okay, fair enough. Can we link to https://developers.optimizely.com/x/events/api/#Attribute_ instead?

mikeproeng37 · 2018-09-18T21:39:06Z

FYI, I ran integration tests for these changes and they passed =)

fix(nullfilter): filters out attributes with null values

062b536

mnoman09 mentioned this pull request Sep 11, 2018

fix(nullfilter): filters out attributes with null values #211

Closed

nchilada suggested changes Sep 11, 2018

View reviewed changes

mikeproeng37 mentioned this pull request Sep 11, 2018

fix(nullfilter): Filters attributes in getFeatureVariableValueForType #212

Closed

Added checks if attribute value is String, Integer, Double, Boolean o…

1a8c5c5

…r null than ignore it

nchilada reviewed Sep 12, 2018

View reviewed changes

msohailhussain reviewed Sep 12, 2018

View reviewed changes

* Added Unit test to verify string double integer and boolean are get…

a76fc66

…ting passed in event factory building list * Added Unit test to verify bigdecimal and bigInteger are not getting passed in event factory building list

nchilada reviewed Sep 13, 2018

View reviewed changes

* Updated Comment on eventFactory.BuildAttributeList Filter condition

b573492

* Added Unit Test To test with ConversionEvent and verified that valid attributes are getting passed and invalid are getting ignored

nchilada approved these changes Sep 18, 2018

View reviewed changes

mikeproeng37 suggested changes Sep 18, 2018

View reviewed changes

Changed schema link.

49da93d

mikeproeng37 approved these changes Sep 19, 2018

View reviewed changes

mikeproeng37 merged commit 1bb574a into master Sep 19, 2018

mikeproeng37 pushed a commit that referenced this pull request Sep 21, 2018

fix(attributes): filters out attributes with null values (#214)

d3be123

mikeproeng37 mentioned this pull request Sep 21, 2018

chore: Prepare release branch for new release #216

Merged

mikeproeng37 deleted the mnoman/removeNullAttrFromPayload branch September 21, 2018 22:35

fix(nullfilter): filters out attributes with null values #214

fix(nullfilter): filters out attributes with null values #214

Uh oh!

Conversation

mnoman09 commented Sep 11, 2018

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 635

💛 - 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!

mikeproeng37 Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

mikeproeng37 commented Sep 18, 2018

Uh oh!

Uh oh!

coveralls commented Sep 11, 2018 •

edited

Loading

mikeproeng37 Sep 11, 2018 •

edited

Loading