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

mnoman09 · mikeproeng37 · commit 1bb574a6c385 · 2018-09-19T16:00:57.000-07:00
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
@@ -168,6 +168,18 @@ private List<Attribute> buildAttributeList(ProjectConfig projectConfig, Map<Stri
         List<Attribute> attributesList = new ArrayList<Attribute>();
 
         for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+            // 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://developers.optimizely.com/x/events/api/#Attribute
+            if (entry.getValue() == null ||
+                    !((entry.getValue() instanceof String) ||
+                    (entry.getValue() instanceof Integer) ||
+                    (entry.getValue() instanceof Double) ||
+                    (entry.getValue() instanceof Boolean))) {
+                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
@@ -40,18 +40,15 @@
 
 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.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.*;
 
 import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2;
 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;
@@ -60,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;
@@ -249,6 +247,196 @@ 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<Experiment> 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<String, Object> eventTagMap = new HashMap<>();
+        eventTagMap.put("boolean_param", false);
+        eventTagMap.put("string_param", "123");
+
+        HashMap<String, Object> 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<Experiment, Variation> 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
+     * causing an exception to be thrown.
+     */
+    @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 attribute1 = validProjectConfig.getAttributes().get(0);
+        Attribute attribute2 = validProjectConfig.getAttributes().get(1);
+        BigInteger bigInteger = new BigInteger("12323");
+        BigDecimal bigDecimal = new BigDecimal("123");
+
+        HashMap<String, Object> attributes = new HashMap<>();
+        attributes.put(attribute1.getKey(), bigInteger);
+        attributes.put(attribute2.getKey(), bigDecimal);
+
+        LogEvent impressionEvent =
+                factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId",
+                        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(), 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<String, Object> 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;
+        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.
@@ -946,7 +1134,7 @@ public static Map<Experiment, Variation> createExperimentVariationMap(ProjectCon
                                                                           DecisionService decisionService,
                                                                           String eventName,
                                                                           String userId,
-                                                                          @Nullable Map<String, String> attributes) {
+                                                                          @Nullable Map<String, ?> attributes) {
 
         List<Experiment> eventExperiments = projectConfig.getExperimentsForEventKey(eventName);
         Map<Experiment, Variation> experimentVariationMap = new HashMap<Experiment, Variation>(eventExperiments.size());
