patch user attribute evaluation (#152)

* patch evaluation by using equals
* make value optional in the UserAttribute class. Add malformed audience and experiment using it in the parser
* add test for null value evaluation. add audience to experiment
* add test for evaluating audience with missing condition value

Author: wangjoshuah

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

@@ -16,11 +16,10 @@
  */
 package com.optimizely.ab.config.audience;
 
-import java.util.List;
-import java.util.Map;
-
 import javax.annotation.Nonnull;
 import javax.annotation.concurrent.Immutable;
+import java.util.List;
+import java.util.Map;
 
 /**
  * Represents an 'And' conditions condition operation.
@@ -29,7 +28,6 @@
 public class AndCondition implements Condition {
 
     private final List<Condition> conditions;
-
     public AndCondition(@Nonnull List<Condition> conditions) {
         this.conditions = conditions;
     }

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

@@ -16,18 +16,16 @@
  */
 package com.optimizely.ab.config.audience;
 
+import javax.annotation.Nonnull;
+import javax.annotation.concurrent.Immutable;
 import java.util.List;
 import java.util.Map;
 
-import javax.annotation.concurrent.Immutable;
-import javax.annotation.Nonnull;
-
 /**
  * Represents an 'Or' conditions condition operation.
  */
 @Immutable
 public class OrCondition implements Condition {
-
     private final List<Condition> conditions;
 
     public OrCondition(@Nonnull List<Condition> conditions) {

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

@@ -17,6 +17,7 @@
 package com.optimizely.ab.config.audience;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import java.util.Map;
 
@@ -30,7 +31,7 @@ public class UserAttribute implements Condition {
     private final String type;
     private final String value;
 
-    public UserAttribute(@Nonnull String name, @Nonnull String type, @Nonnull String value) {
+    public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String value) {
         this.name = name;
         this.type = type;
         this.value = value;
@@ -51,7 +52,17 @@ public String getValue() {
     public boolean evaluate(Map<String, String> attributes) {
         String userAttributeValue = attributes.get(name);
 
-        return value.equals(userAttributeValue);
+        if (value != null) { // if there is a value in the condition
+            // check user attribute value is equal
+            return value.equals(userAttributeValue);
+        }
+        else if (userAttributeValue != null) { // if the datafile value is null but user has a value for this attribute
+            // return false since null != nonnull
+            return false;
+        }
+        else { // both are null
+            return true;
+        }
     }
 
     @Override
@@ -63,23 +74,22 @@ public String toString() {
     }
 
     @Override
-    public boolean equals(Object other) {
-        if (!(other instanceof UserAttribute))
-            return false;
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
 
-        UserAttribute otherConditionObj = (UserAttribute)other;
+        UserAttribute that = (UserAttribute) o;
 
-        return name.equals(otherConditionObj.getName()) && type.equals(otherConditionObj.getType())
-            && value.equals(otherConditionObj.getValue());
+        if (!name.equals(that.name)) return false;
+        if (!type.equals(that.type)) return false;
+        return value != null ? value.equals(that.value) : that.value == null;
     }
 
     @Override
     public int hashCode() {
-        final int prime = 31;
-        int result = 1;
-        result = prime * result + name.hashCode();
-        result = prime * result + type.hashCode();
-        result = prime * result + value.hashCode();
+        int result = name.hashCode();
+        result = 31 * result + type.hashCode();
+        result = 31 * result + (value != null ? value.hashCode() : 0);
         return result;
     }
 }

core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java

@@ -234,8 +234,15 @@ private Condition parseConditions(JSONArray conditionJson) {
                 conditions.add(parseConditions(conditionJson.getJSONArray(i)));
             } else {
                 JSONObject conditionMap = (JSONObject)obj;
-                conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
-                               (String)conditionMap.get("value")));
+                String value = null;
+                if (conditionMap.has("value")) {
+                    value = conditionMap.getString("value");
+                }
+                conditions.add(new UserAttribute(
+                        (String)conditionMap.get("name"),
+                        (String)conditionMap.get("type"),
+                               value
+                ));
             }
         }
 

core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java

@@ -16,10 +16,12 @@
  */
 package com.optimizely.ab.config.audience;
 
+import com.optimizely.ab.config.Experiment;
 import org.junit.Before;
 import org.junit.Test;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -187,4 +189,28 @@ public void andConditionEvaluateFalse() throws Exception {
         // shouldn't be called due to short-circuiting in 'And' evaluation
         verify(orCondition2, times(0)).evaluate(testUserAttributes);
     }
+
+    /**
+     * Verify that {@link UserAttribute#evaluate(Map)}
+     * called when its attribute value is null
+     * returns True when the user's attribute value is also null
+     *          True when the attribute is not in the map
+     *          False when empty string is used.
+     * @throws Exception
+     */
+    @Test
+    public void nullValueEvaluate() throws Exception {
+        String attributeName = "attribute_name";
+        String attributeType = "attribute_type";
+        String attributeValue = null;
+        UserAttribute nullValueAttribute = new UserAttribute(
+                attributeName,
+                attributeType,
+                attributeValue
+        );
+
+        assertTrue(nullValueAttribute.evaluate(Collections.<String, String>emptyMap()));
+        assertTrue(nullValueAttribute.evaluate(Collections.singletonMap(attributeName, attributeValue)));
+        assertFalse(nullValueAttribute.evaluate((Collections.singletonMap(attributeName, ""))));
+    }
 }

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

@@ -16,13 +16,13 @@
  */
 package com.optimizely.ab.internal;
 
-import com.optimizely.ab.config.audience.Audience;
-import com.optimizely.ab.config.audience.Condition;
 import com.optimizely.ab.config.Experiment;
 import com.optimizely.ab.config.Experiment.ExperimentStatus;
 import com.optimizely.ab.config.ProjectConfig;
 import com.optimizely.ab.config.TrafficAllocation;
 import com.optimizely.ab.config.Variation;
+import com.optimizely.ab.config.audience.Audience;
+import com.optimizely.ab.config.audience.Condition;
 import org.junit.BeforeClass;
 import org.junit.Test;