From 062b53631549b8a3ba23832b1ac1d21047b0f22c Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Tue, 11 Sep 2018 13:38:26 +0500 Subject: [PATCH 1/5] fix(nullfilter): filters out attributes with null values --- .../ab/event/internal/EventFactory.java | 5 ++++ .../ab/event/internal/EventFactoryTest.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index ba347e7af..b3a80bced 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -168,6 +168,11 @@ private List buildAttributeList(ProjectConfig projectConfig, Map attributesList = new ArrayList(); for (Map.Entry entry : attributes.entrySet()) { + //Ignore null value attribute + if (entry.getValue() == null) { + continue; + } + String attributeId = projectConfig.getAttributeId(projectConfig, entry.getKey()); if(attributeId == null) { continue; diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java index be170c839..040102286 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java @@ -52,6 +52,7 @@ import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV4; import static com.optimizely.ab.config.ValidProjectConfigV4.*; import static junit.framework.Assert.assertNotNull; +import static junit.framework.Assert.assertNotSame; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -249,6 +250,32 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { } } + + /** + * Verify that passing through an null value attribute causes that attribute to be ignored, rather than + * causing an exception to be thrown. + */ + @Test + public void createImpressionEventIgnoresInvalidAttributes() throws Exception { + // use the "valid" project config and its associated experiment, variation, and attributes + ProjectConfig projectConfig = validProjectConfig; + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + Attribute attribute = validProjectConfig.getAttributes().get(0); + + LogEvent impressionEvent = + factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId", + Collections.singletonMap(attribute.getKey(), null)); + + EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class); + + // verify that no Feature is created for attribute.getKey() -> null + for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) { + assertNotSame(feature.getKey(), attribute.getKey()); + assertNotSame(feature.getValue(), null); + } + } + /** * Verify that supplying {@link EventFactory} with a custom client engine and client version results in impression * events being sent with the overriden values. From 1a8c5c5f1a00c1b8f4d19aefe2cb0b862bcface3 Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Wed, 12 Sep 2018 19:22:41 +0500 Subject: [PATCH 2/5] Added checks if attribute value is String, Integer, Double, Boolean or null than ignore it --- .../ab/event/internal/EventFactory.java | 6 +++- .../ab/event/internal/EventFactoryTest.java | 36 ++++++++++++++----- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index b3a80bced..6fb8408f2 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -169,7 +169,11 @@ private List buildAttributeList(ProjectConfig projectConfig, Map entry : attributes.entrySet()) { //Ignore null value attribute - if (entry.getValue() == null) { + if (entry.getValue() == null || + (!(entry.getValue() instanceof String) && + !(entry.getValue() instanceof Integer) && + !(entry.getValue() instanceof Double) && + !(entry.getValue() instanceof Boolean))) { continue; } diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java index 040102286..6f2639adc 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java @@ -40,13 +40,7 @@ import javax.annotation.Nullable; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2; import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV4; @@ -252,11 +246,35 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { /** - * Verify that passing through an null value attribute causes that attribute to be ignored, rather than + * Verify that passing through an list value attribute causes that attribute to be ignored, rather than * causing an exception to be thrown. */ @Test - public void createImpressionEventIgnoresInvalidAttributes() throws Exception { + public void createImpressionEventIgnoresInvalidAttributes() { + // use the "valid" project config and its associated experiment, variation, and attributes + ProjectConfig projectConfig = validProjectConfig; + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + Attribute attribute = validProjectConfig.getAttributes().get(0); + List invalidAttribute = new LinkedList(); + LogEvent impressionEvent = + factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId", + Collections.singletonMap(attribute.getKey(), invalidAttribute)); + + EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class); + + // verify that no Feature is created for attribute.getKey() -> invalidAttribute + for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) { + assertNotSame(feature.getKey(), attribute.getKey()); + assertNotSame(feature.getValue(), invalidAttribute); + } + } + + /** + * Verify that passing through an null value attribute causes that attribute to be ignored, rather than + * causing an exception to be thrown. + */ + public void createImpressionEventIgnoresNullAttributes() { // use the "valid" project config and its associated experiment, variation, and attributes ProjectConfig projectConfig = validProjectConfig; Experiment activatedExperiment = projectConfig.getExperiments().get(0); From a76fc667490dcdc9b79f2ad4947e8560039d7e03 Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Thu, 13 Sep 2018 21:21:30 +0500 Subject: [PATCH 3/5] * Added Unit test to verify string double integer and boolean are getting passed in event factory building list * Added Unit test to verify bigdecimal and bigInteger are not getting passed in event factory building list --- .../ab/event/internal/EventFactory.java | 11 +-- .../ab/event/internal/EventFactoryTest.java | 72 +++++++++++++++++-- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index 6fb8408f2..b95e337a3 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -168,12 +168,13 @@ private List buildAttributeList(ProjectConfig projectConfig, Map attributesList = new ArrayList(); for (Map.Entry entry : attributes.entrySet()) { - //Ignore null value attribute + // Filters down to the types of values that are allowed. + // Don't allow Longs, BigIntegers, or BigDecimals - they /can/ theoretically be serialized as JSON numbers if (entry.getValue() == null || - (!(entry.getValue() instanceof String) && - !(entry.getValue() instanceof Integer) && - !(entry.getValue() instanceof Double) && - !(entry.getValue() instanceof Boolean))) { + !((entry.getValue() instanceof String) || + (entry.getValue() instanceof Integer) || + (entry.getValue() instanceof Double) || + (entry.getValue() instanceof Boolean))) { continue; } diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java index 6f2639adc..6856ec27c 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java @@ -40,6 +40,8 @@ import javax.annotation.Nullable; import java.io.IOException; +import java.math.BigDecimal; +import java.math.BigInteger; import java.util.*; import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2; @@ -55,6 +57,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -251,29 +254,86 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { */ @Test public void createImpressionEventIgnoresInvalidAttributes() { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); // use the "valid" project config and its associated experiment, variation, and attributes ProjectConfig projectConfig = validProjectConfig; Experiment activatedExperiment = projectConfig.getExperiments().get(0); Variation bucketedVariation = activatedExperiment.getVariations().get(0); - Attribute attribute = validProjectConfig.getAttributes().get(0); - List invalidAttribute = new LinkedList(); + Attribute attribute1 = validProjectConfig.getAttributes().get(0); + Attribute attribute2 = validProjectConfig.getAttributes().get(1); + BigInteger bigInteger = new BigInteger("12323"); + BigDecimal bigDecimal = new BigDecimal("123"); + + HashMap attributes = new HashMap<>(); + attributes.put(attribute1.getKey(), bigInteger); + attributes.put(attribute2.getKey(), bigDecimal); + LogEvent impressionEvent = factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId", - Collections.singletonMap(attribute.getKey(), invalidAttribute)); + attributes); EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class); // verify that no Feature is created for attribute.getKey() -> invalidAttribute for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) { - assertNotSame(feature.getKey(), attribute.getKey()); - assertNotSame(feature.getValue(), invalidAttribute); + assertNotSame(feature.getKey(), attribute1.getKey()); + assertNotSame(feature.getValue(), bigInteger); + assertNotSame(feature.getKey(), attribute2.getKey()); + assertNotSame(feature.getValue(), bigDecimal); } } + /** + * Verify that Integer, Decimal, Bool and String variables are allowed to pass. + */ + @Test + public void createImpressionEventWithIntegerDecimalBoolAndStringAttributes() { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + // use the "valid" project config and its associated experiment, variation, and attributes + ProjectConfig projectConfig = validProjectConfig; + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + Attribute doubleAttribute = validProjectConfig.getAttributes().get(5); + Attribute integerAttribute = validProjectConfig.getAttributes().get(4); + Attribute boolAttribute = validProjectConfig.getAttributes().get(3); + Attribute stringAttribute = validProjectConfig.getAttributes().get(0); + double validDoubleAttribute = 13.1; + int validIntegerAttribute = 12; + boolean validBoolAttribute = true; + String validStringAttribute = "grayfindor"; + + HashMap attributes = new HashMap<>(); + attributes.put(doubleAttribute.getKey(), validDoubleAttribute); + attributes.put(integerAttribute.getKey(), validIntegerAttribute); + attributes.put(boolAttribute.getKey(), validBoolAttribute); + attributes.put(stringAttribute.getKey(), validStringAttribute); + + LogEvent impressionEvent = + factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId", + attributes); + + EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class); + + + 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(), doubleAttribute.getKey()); + assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getValue(), validDoubleAttribute); + + assertEquals(impression.getVisitors().get(0).getAttributes().get(2).getKey(), integerAttribute.getKey()); + assertEquals((int) ((double) impression.getVisitors().get(0).getAttributes().get(2).getValue()), validIntegerAttribute); + + assertEquals(impression.getVisitors().get(0).getAttributes().get(3).getKey(), stringAttribute.getKey()); + assertEquals(impression.getVisitors().get(0).getAttributes().get(3).getValue(), validStringAttribute); + + } + /** * Verify that passing through an null value attribute causes that attribute to be ignored, rather than * causing an exception to be thrown. */ + @Test public void createImpressionEventIgnoresNullAttributes() { // use the "valid" project config and its associated experiment, variation, and attributes ProjectConfig projectConfig = validProjectConfig; @@ -290,7 +350,7 @@ public void createImpressionEventIgnoresNullAttributes() { // verify that no Feature is created for attribute.getKey() -> null for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) { assertNotSame(feature.getKey(), attribute.getKey()); - assertNotSame(feature.getValue(), null); + assertNotSame(feature.getValue(), null); } } From b573492f47925caf4e0674c50217284d80d61bbb Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Mon, 17 Sep 2018 12:58:39 +0500 Subject: [PATCH 4/5] * Updated Comment on eventFactory.BuildAttributeList Filter condition * Added Unit Test To test with ConversionEvent and verified that valid attributes are getting passed and invalid are getting ignored --- .../ab/event/internal/EventFactory.java | 4 +- .../ab/event/internal/EventFactoryTest.java | 85 ++++++++++++++++++- 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index b95e337a3..dc6f420f1 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -168,8 +168,10 @@ private List buildAttributeList(ProjectConfig projectConfig, Map attributesList = new ArrayList(); for (Map.Entry entry : attributes.entrySet()) { - // Filters down to the types of values that are allowed. + // 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 if (entry.getValue() == null || !((entry.getValue() instanceof String) || (entry.getValue() instanceof Integer) || diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java index 6856ec27c..f2b555526 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java @@ -247,6 +247,89 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { } } + /** + * Verify that passing through an list value attribute causes that attribute to be ignored, rather than + * causing an exception to be thrown and passing only the valid attributes. + */ + @Test + public void createConversionEventIgnoresInvalidAndAcceptsValidAttributes() { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + + Bucketer mockBucketAlgorithm = mock(Bucketer.class); + EventType eventType = validProjectConfig.getEventTypes().get(0); + + List allExperiments = validProjectConfig.getExperiments(); + + // 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) { + when(mockBucketAlgorithm.bucket(experiment, userId)) + .thenReturn(experiment.getVariations().get(0)); + } + Attribute attribute1 = validProjectConfig.getAttributes().get(0); + Attribute attribute2 = validProjectConfig.getAttributes().get(1); + Attribute doubleAttribute = validProjectConfig.getAttributes().get(5); + Attribute integerAttribute = validProjectConfig.getAttributes().get(4); + Attribute boolAttribute = validProjectConfig.getAttributes().get(3); + + BigInteger bigInteger = new BigInteger("12323"); + BigDecimal bigDecimal = new BigDecimal("123"); + double validDoubleAttribute = 13.1; + int validIntegerAttribute = 12; + boolean validBoolAttribute = true; + + Map eventTagMap = new HashMap<>(); + eventTagMap.put("boolean_param", false); + eventTagMap.put("string_param", "123"); + + HashMap attributes = new HashMap<>(); + attributes.put(attribute1.getKey(), bigInteger); + attributes.put(attribute2.getKey(), bigDecimal); + attributes.put(doubleAttribute.getKey(), validDoubleAttribute); + attributes.put(integerAttribute.getKey(), validIntegerAttribute); + attributes.put(boolAttribute.getKey(), validBoolAttribute); + + DecisionService decisionService = new DecisionService( + mockBucketAlgorithm, + mock(ErrorHandler.class), + validProjectConfig, + mock(UserProfileService.class) + ); + Map experimentVariationMap = createExperimentVariationMap( + validProjectConfig, + decisionService, + eventType.getKey(), + userId, + attributes); + LogEvent conversionEvent = factory.createConversionEvent( + validProjectConfig, + experimentVariationMap, + userId, + eventType.getId(), + eventType.getKey(), + attributes, + eventTagMap); + + EventBatch impression = gson.fromJson(conversionEvent.getBody(), EventBatch.class); + + //Check valid attributes are getting passed. + 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(), doubleAttribute.getKey()); + assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getValue(), validDoubleAttribute); + + assertEquals(impression.getVisitors().get(0).getAttributes().get(2).getKey(), integerAttribute.getKey()); + assertEquals((int) ((double) impression.getVisitors().get(0).getAttributes().get(2).getValue()), validIntegerAttribute); + + // verify that no Feature is created for attribute.getKey() -> invalidAttribute + for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) { + assertNotSame(feature.getKey(), attribute1.getKey()); + assertNotSame(feature.getValue(), bigInteger); + assertNotSame(feature.getKey(), attribute2.getKey()); + assertNotSame(feature.getValue(), bigDecimal); + } + } /** * Verify that passing through an list value attribute causes that attribute to be ignored, rather than @@ -1051,7 +1134,7 @@ public static Map createExperimentVariationMap(ProjectCon DecisionService decisionService, String eventName, String userId, - @Nullable Map attributes) { + @Nullable Map attributes) { List eventExperiments = projectConfig.getExperimentsForEventKey(eventName); Map experimentVariationMap = new HashMap(eventExperiments.size()); From 49da93d82d1755353a7338f11b01e8b70b626f67 Mon Sep 17 00:00:00 2001 From: Sohail Hussain Date: Wed, 19 Sep 2018 15:02:36 -0700 Subject: [PATCH 5/5] Changed schema link. --- .../java/com/optimizely/ab/event/internal/EventFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index dc6f420f1..0ea6000b0 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -171,7 +171,7 @@ private List buildAttributeList(ProjectConfig projectConfig, Map