fix: Removed filtering of unknown attributes and assignment of empty map to null attributes and null event tags. (#221)

mnoman09 · mikeproeng37 · commit da3b24f9c807 · 2018-10-17T11:30:41.000-07:00
diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java
@@ -209,18 +209,15 @@ Variation activate(@Nonnull ProjectConfig projectConfig,
             logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
             return null;
         }
-        // determine whether all the given attributes are present in the project config. If not, filter out the unknown
-        // attributes.
-        Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);
-
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
         // bucket the user to the given experiment and dispatch an impression event
-        Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes);
+        Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes);
         if (variation == null) {
             logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
             return null;
         }
 
-        sendImpression(projectConfig, experiment, userId, filteredAttributes, variation);
+        sendImpression(projectConfig, experiment, userId, copiedAttributes, variation);
 
         return variation;
     }
@@ -292,6 +289,7 @@ public void track(@Nonnull String eventName,
         }
 
         ProjectConfig currentConfig = getProjectConfig();
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
 
         EventType eventType = currentConfig.getEventTypeForName(eventName, errorHandler);
         if (eventType == null) {
@@ -300,20 +298,15 @@ public void track(@Nonnull String eventName,
             return;
         }
 
-        // determine whether all the given attributes are present in the project config. If not, filter out the unknown
-        // attributes.
-        Map<String, ?> filteredAttributes = filterAttributes(currentConfig, attributes);
-
         if (eventTags == null) {
             logger.warn("Event tags is null when non-null was expected. Defaulting to an empty event tags map.");
-            eventTags = Collections.<String, String>emptyMap();
         }
 
         List<Experiment> experimentsForEvent = projectConfig.getExperimentsForEventKey(eventName);
         Map<Experiment, Variation> experimentVariationMap = new HashMap<Experiment, Variation>(experimentsForEvent.size());
         for (Experiment experiment : experimentsForEvent) {
             if (experiment.isRunning()) {
-                Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes);
+                Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes);
                 if (variation != null) {
                     experimentVariationMap.put(experiment, variation);
                 }
@@ -331,7 +324,7 @@ public void track(@Nonnull String eventName,
                 userId,
                 eventType.getId(),
                 eventType.getKey(),
-                filteredAttributes,
+                copiedAttributes,
                 eventTags);
 
         if (conversionEvent == null) {
@@ -354,7 +347,7 @@ public void track(@Nonnull String eventName,
         }
 
         notificationCenter.sendNotifications(NotificationCenter.NotificationType.Track, eventName, userId,
-                filteredAttributes, eventTags, conversionEvent);
+                copiedAttributes, eventTags, conversionEvent);
     }
 
     //======== FeatureFlag APIs ========//
@@ -406,17 +399,15 @@ else if (userId == null) {
             logger.info("No feature flag was found for key \"{}\".", featureKey);
             return false;
         }
-
-        Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);
-
-        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes);
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes);
         if (featureDecision.variation != null) {
             if (featureDecision.decisionSource.equals(FeatureDecision.DecisionSource.EXPERIMENT)) {
                 sendImpression(
                         projectConfig,
                         featureDecision.experiment,
                         userId,
-                        filteredAttributes,
+                        copiedAttributes,
                         featureDecision.variation);
             } else {
                 logger.info("The user \"{}\" is not included in an experiment for feature \"{}\".",
@@ -655,8 +646,9 @@ else if (userId == null) {
         }
 
         String variableValue = variable.getDefaultValue();
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
 
-        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, attributes);
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes);
         if (featureDecision.variation != null) {
             LiveVariableUsageInstance liveVariableUsageInstance =
                     featureDecision.variation.getVariableIdToLiveVariableUsageInstanceMap().get(variable.getId());
@@ -716,10 +708,9 @@ Variation getVariation(@Nonnull Experiment experiment,
     Variation getVariation(@Nonnull Experiment experiment,
                            @Nonnull String userId,
                            @Nonnull Map<String, ?> attributes) throws UnknownExperimentException {
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
 
-        Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);
-
-        return decisionService.getVariation(experiment, userId, filteredAttributes);
+        return decisionService.getVariation(experiment, userId, copiedAttributes);
     }
 
     public @Nullable
@@ -754,10 +745,8 @@ Variation getVariation(@Nonnull String experimentKey,
             // if we're unable to retrieve the associated experiment, return null
             return null;
         }
-
-        Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);
-
-        return decisionService.getVariation(experiment,userId,filteredAttributes);
+        Map<String, ?> copiedAttributes = copyAttributes(attributes);
+        return decisionService.getVariation(experiment, userId, copiedAttributes);
     }
 
     /**
@@ -817,50 +806,6 @@ public UserProfileService getUserProfileService() {
     }
 
     //======== Helper methods ========//
-
-    /**
-     * Helper method to verify that the given attributes map contains only keys that are present in the
-     * {@link ProjectConfig}.
-     *
-     * @param projectConfig the current project config
-     * @param attributes the attributes map to validate and potentially filter. Attributes which starts with reserved key
-     * {@link ProjectConfig#RESERVED_ATTRIBUTE_PREFIX} are kept.
-     * @return the filtered attributes map (containing only attributes that are present in the project config) or an
-     * empty map if a null attributes object is passed in
-     */
-    private Map<String, ?> filterAttributes(@Nonnull ProjectConfig projectConfig,
-                                                 @Nonnull Map<String, ?> attributes) {
-        if (attributes == null) {
-            logger.warn("Attributes is null when non-null was expected. Defaulting to an empty attributes map.");
-            return Collections.<String, String>emptyMap();
-        }
-
-        // List of attribute keys
-        List<String> unknownAttributes = null;
-
-        Map<String, Attribute> attributeKeyMapping = projectConfig.getAttributeKeyMapping();
-        for (Map.Entry<String, ?> attribute : attributes.entrySet()) {
-            if (!attributeKeyMapping.containsKey(attribute.getKey()) &&
-                    !attribute.getKey().startsWith(ProjectConfig.RESERVED_ATTRIBUTE_PREFIX)) {
-                if (unknownAttributes == null) {
-                    unknownAttributes = new ArrayList<String>();
-                }
-                unknownAttributes.add(attribute.getKey());
-            }
-        }
-
-        if (unknownAttributes != null) {
-            logger.warn("Attribute(s) {} not in the datafile.", unknownAttributes);
-            // make a copy of the passed through attributes, then remove the unknown list
-            attributes = new HashMap<>(attributes);
-            for (String unknownAttribute : unknownAttributes) {
-                attributes.remove(unknownAttribute);
-            }
-        }
-
-        return attributes;
-    }
-
     /**
      * Helper function to check that the provided userId is valid
      *
@@ -880,6 +825,20 @@ private boolean validateUserId(String userId) {
         return true;
     }
 
+    /**
+     * Helper function to check that the provided attributes are null if not than it returns a copy
+     *
+     * @param attributes the attributes map being validated
+     * @return copy of attributes
+     */
+    private Map<String, ?> copyAttributes(Map<String, ?> attributes) {
+        Map<String, ?> copiedAttributes = null;
+        if (attributes != null) {
+            copiedAttributes = new HashMap<>(attributes);
+        }
+        return copiedAttributes;
+    }
+
     //======== Builder ========//
 
     public static Builder builder(@Nonnull String datafile,
diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java
@@ -24,6 +24,7 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
+import java.util.Collections;
 import java.util.Map;
 
 /**
@@ -66,6 +67,9 @@ public Object getValue() {
     }
 
     public @Nullable Boolean evaluate(Map<String, ?> attributes) {
+        if (attributes == null) {
+            attributes = Collections.emptyMap();
+        }
         // Valid for primitive types, but needs to change when a value is an object or an array
         Object userAttributeValue = attributes.get(name);
 
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
@@ -167,38 +167,41 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
     private List<Attribute> buildAttributeList(ProjectConfig projectConfig, Map<String, ?> attributes) {
         List<Attribute> attributesList = new ArrayList<Attribute>();
 
-        for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+        if (attributes != null) {
+            for (Map.Entry<String, ?> entry : attributes.entrySet()) {
+
+                // Ignore attributes with empty key
+                if (entry.getKey().isEmpty()) {
+                    continue;
+                }
+
+                // 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;
+                }
+
+                Attribute attribute = new Attribute.Builder()
+                        .setEntityId(attributeId)
+                        .setKey(entry.getKey())
+                        .setType(Attribute.CUSTOM_ATTRIBUTE_TYPE)
+                        .setValue(entry.getValue())
+                        .build();
+
+                attributesList.add(attribute);
 
-            //Ignore attributes with empty key
-            if (entry.getKey().isEmpty()) {
-                continue;
             }
-
-            // 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;
-            }
-
-            Attribute attribute = new Attribute.Builder()
-                    .setEntityId(attributeId)
-                    .setKey(entry.getKey())
-                    .setType(Attribute.CUSTOM_ATTRIBUTE_TYPE)
-                    .setValue(entry.getValue())
-                    .build();
-
-            attributesList.add(attribute);
         }
 
         //checks if botFiltering value is not set in the project config file.
diff --git a/core-api/src/main/java/com/optimizely/ab/internal/EventTagUtils.java b/core-api/src/main/java/com/optimizely/ab/internal/EventTagUtils.java
@@ -1,6 +1,6 @@
 /**
  *
- *    Copyright 2016-2017, Optimizely and contributors
+ *    Copyright 2016-2018, Optimizely and contributors
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
  *    you may not use this file except in compliance with the License.
@@ -33,7 +33,7 @@ public final class EventTagUtils {
      */
     public static Long getRevenueValue(@Nonnull Map<String, ?> eventTags) {
         Long eventValue = null;
-        if (eventTags.containsKey(ReservedEventKey.REVENUE.toString())) {
+        if (eventTags != null && eventTags.containsKey(ReservedEventKey.REVENUE.toString())) {
             Object rawValue = eventTags.get(ReservedEventKey.REVENUE.toString());
             if (Long.class.isInstance(rawValue)) {
                 eventValue = (Long)rawValue;
@@ -53,7 +53,7 @@ public static Long getRevenueValue(@Nonnull Map<String, ?> eventTags) {
      */
     public static Double getNumericValue(@Nonnull Map<String, ?> eventTags) {
         Double eventValue = null;
-        if (eventTags.containsKey(ReservedEventKey.VALUE.toString())) {
+        if (eventTags != null && eventTags.containsKey(ReservedEventKey.VALUE.toString())) {
             Object rawValue = eventTags.get(ReservedEventKey.VALUE.toString());
             if (rawValue instanceof Number) {
                 eventValue = ((Number) rawValue).doubleValue();
diff --git a/core-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListener.java b/core-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListener.java
@@ -37,8 +37,11 @@ public final void notify(Object... args) {
         Experiment experiment = (Experiment) args[0];
         assert(args[1] instanceof String);
         String userId = (String) args[1];
-        assert(args[2] instanceof java.util.Map);
-        Map<String, ?> attributes = (Map<String, ?>) args[2];
+        Map<String, ?> attributes = null;
+        if (args[2] != null) {
+            assert (args[2] instanceof java.util.Map);
+            attributes = (Map<String, ?>) args[2];
+        }
         assert(args[3] instanceof Variation);
         Variation variation = (Variation) args[3];
         assert(args[4] instanceof LogEvent);
diff --git a/core-api/src/main/java/com/optimizely/ab/notification/TrackNotificationListener.java b/core-api/src/main/java/com/optimizely/ab/notification/TrackNotificationListener.java
@@ -35,10 +35,16 @@ public final void notify(Object... args) {
         String eventKey = (String) args[0];
         assert(args[1] instanceof String);
         String userId = (String) args[1];
-        assert(args[2] instanceof java.util.Map);
-        Map<String, ?> attributes = (Map<String, ?>) args[2];
-        assert(args[3] instanceof java.util.Map);
-        Map<String, ?> eventTags = (Map<String, ?>) args[3];
+        Map<String, ?> attributes = null;
+        if (args[2] != null) {
+            assert (args[2] instanceof java.util.Map);
+            attributes = (Map<String, ?>) args[2];
+        }
+        Map<String, ?> eventTags = null;
+        if (args[3] != null) {
+            assert (args[3] instanceof java.util.Map);
+            eventTags = (Map<String, ?>) args[3];
+        }
         assert(args[4] instanceof LogEvent);
         LogEvent logEvent = (LogEvent) args[4];
 
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
