Skip to content

Commit 5f2f3f5

Browse files
wangjoshuahwangjoshuah
authored
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
1 parent 96ae369 commit 5f2f3f5

File tree

8 files changed

+169
-25
lines changed

8 files changed

+169
-25
lines changed

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,10 @@
1616
*/
1717
package com.optimizely.ab.config.audience;
1818

19-
import java.util.List;
20-
import java.util.Map;
21-
2219
import javax.annotation.Nonnull;
2320
import javax.annotation.concurrent.Immutable;
21+
import java.util.List;
22+
import java.util.Map;
2423

2524
/**
2625
* Represents an 'And' conditions condition operation.
@@ -29,7 +28,6 @@
2928
public class AndCondition implements Condition {
3029

3130
private final List<Condition> conditions;
32-
3331
public AndCondition(@Nonnull List<Condition> conditions) {
3432
this.conditions = conditions;
3533
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,16 @@
1616
*/
1717
package com.optimizely.ab.config.audience;
1818

19+
import javax.annotation.Nonnull;
20+
import javax.annotation.concurrent.Immutable;
1921
import java.util.List;
2022
import java.util.Map;
2123

22-
import javax.annotation.concurrent.Immutable;
23-
import javax.annotation.Nonnull;
24-
2524
/**
2625
* Represents an 'Or' conditions condition operation.
2726
*/
2827
@Immutable
2928
public class OrCondition implements Condition {
30-
3129
private final List<Condition> conditions;
3230

3331
public OrCondition(@Nonnull List<Condition> conditions) {

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

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package com.optimizely.ab.config.audience;
1818

1919
import javax.annotation.Nonnull;
20+
import javax.annotation.Nullable;
2021
import javax.annotation.concurrent.Immutable;
2122
import java.util.Map;
2223

@@ -30,7 +31,7 @@ public class UserAttribute implements Condition {
3031
private final String type;
3132
private final String value;
3233

33-
public UserAttribute(@Nonnull String name, @Nonnull String type, @Nonnull String value) {
34+
public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String value) {
3435
this.name = name;
3536
this.type = type;
3637
this.value = value;
@@ -51,7 +52,17 @@ public String getValue() {
5152
public boolean evaluate(Map<String, String> attributes) {
5253
String userAttributeValue = attributes.get(name);
5354

54-
return value.equals(userAttributeValue);
55+
if (value != null) { // if there is a value in the condition
56+
// check user attribute value is equal
57+
return value.equals(userAttributeValue);
58+
}
59+
else if (userAttributeValue != null) { // if the datafile value is null but user has a value for this attribute
60+
// return false since null != nonnull
61+
return false;
62+
}
63+
else { // both are null
64+
return true;
65+
}
5566
}
5667

5768
@Override
@@ -63,23 +74,22 @@ public String toString() {
6374
}
6475

6576
@Override
66-
public boolean equals(Object other) {
67-
if (!(other instanceof UserAttribute))
68-
return false;
77+
public boolean equals(Object o) {
78+
if (this == o) return true;
79+
if (o == null || getClass() != o.getClass()) return false;
6980

70-
UserAttribute otherConditionObj = (UserAttribute)other;
81+
UserAttribute that = (UserAttribute) o;
7182

72-
return name.equals(otherConditionObj.getName()) && type.equals(otherConditionObj.getType())
73-
&& value.equals(otherConditionObj.getValue());
83+
if (!name.equals(that.name)) return false;
84+
if (!type.equals(that.type)) return false;
85+
return value != null ? value.equals(that.value) : that.value == null;
7486
}
7587

7688
@Override
7789
public int hashCode() {
78-
final int prime = 31;
79-
int result = 1;
80-
result = prime * result + name.hashCode();
81-
result = prime * result + type.hashCode();
82-
result = prime * result + value.hashCode();
90+
int result = name.hashCode();
91+
result = 31 * result + type.hashCode();
92+
result = 31 * result + (value != null ? value.hashCode() : 0);
8393
return result;
8494
}
8595
}

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,8 +281,15 @@ private Condition parseConditions(JSONArray conditionJson) {
281281
conditions.add(parseConditions(conditionJson.getJSONArray(i)));
282282
} else {
283283
JSONObject conditionMap = (JSONObject)obj;
284-
conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
285-
(String)conditionMap.get("value")));
284+
String value = null;
285+
if (conditionMap.has("value")) {
286+
value = conditionMap.getString("value");
287+
}
288+
conditions.add(new UserAttribute(
289+
(String)conditionMap.get("name"),
290+
(String)conditionMap.get("type"),
291+
value
292+
));
286293
}
287294
}
288295

‎core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,31 @@ public class ValidProjectConfigV4 {
8383
CUSTOM_DIMENSION_TYPE,
8484
AUDIENCE_ENGLISH_CITIZENS_VALUE)))))))
8585
);
86+
private static final String AUDIENCE_WITH_MISSING_VALUE_ID = "2196265320";
87+
private static final String AUDIENCE_WITH_MISSING_VALUE_KEY = "audience_with_missing_value";
88+
public static final String AUDIENCE_WITH_MISSING_VALUE_VALUE = "English";
89+
private static final UserAttribute ATTRIBUTE_WITH_VALUE = new UserAttribute(
90+
ATTRIBUTE_NATIONALITY_KEY,
91+
CUSTOM_DIMENSION_TYPE,
92+
AUDIENCE_WITH_MISSING_VALUE_VALUE
93+
);
94+
private static final UserAttribute ATTRIBUTE_WITHOUT_VALUE = new UserAttribute(
95+
ATTRIBUTE_NATIONALITY_KEY,
96+
CUSTOM_DIMENSION_TYPE,
97+
null
98+
);
99+
private static final Audience AUDIENCE_WITH_MISSING_VALUE = new Audience(
100+
AUDIENCE_WITH_MISSING_VALUE_ID,
101+
AUDIENCE_WITH_MISSING_VALUE_KEY,
102+
new AndCondition(Collections.<Condition>singletonList(
103+
new OrCondition(Collections.<Condition>singletonList(
104+
new OrCondition(ProjectConfigTestUtils.<Condition>createListOfObjects(
105+
ATTRIBUTE_WITH_VALUE,
106+
ATTRIBUTE_WITHOUT_VALUE
107+
))
108+
))
109+
))
110+
);
86111

87112
// features
88113
private static final String FEATURE_BOOLEAN_FEATURE_ID = "4195505407";
@@ -703,6 +728,32 @@ public class ValidProjectConfigV4 {
703728
GROUP_2_ID
704729
);
705730

731+
private static final String EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID = "748215081";
732+
public static final String EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY = "experiment_with_malformed_audience";
733+
private static final String LAYER_EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID = "1238149537";
734+
private static final String VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID = "535538389";
735+
public static final String VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY = "var1";
736+
private static final Variation VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE = new Variation(
737+
VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID,
738+
VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY,
739+
Collections.<LiveVariableUsageInstance>emptyList()
740+
);
741+
private static final Experiment EXPERIMENT_WITH_MALFORMED_AUDIENCE = new Experiment(
742+
EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID,
743+
EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY,
744+
Experiment.ExperimentStatus.RUNNING.toString(),
745+
LAYER_EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID,
746+
Collections.singletonList(AUDIENCE_WITH_MISSING_VALUE_ID),
747+
Collections.singletonList(VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE),
748+
Collections.<String, String>emptyMap(),
749+
Collections.singletonList(
750+
new TrafficAllocation(
751+
VARIATION_EXPERIMENT_WITH_MALFORMED_AUDIENCE_ID,
752+
10000
753+
)
754+
)
755+
);
756+
706757
// generate groups
707758
private static final Group GROUP_1 = new Group(
708759
GROUP_1_ID,
@@ -962,6 +1013,7 @@ public static ProjectConfig generateValidProjectConfigV4() {
9621013
audiences.add(AUDIENCE_GRYFFINDOR);
9631014
audiences.add(AUDIENCE_SLYTHERIN);
9641015
audiences.add(AUDIENCE_ENGLISH_CITIZENS);
1016+
audiences.add(AUDIENCE_WITH_MISSING_VALUE);
9651017

9661018
// list events
9671019
List<EventType> events = new ArrayList<EventType>();
@@ -976,6 +1028,7 @@ public static ProjectConfig generateValidProjectConfigV4() {
9761028
experiments.add(EXPERIMENT_DOUBLE_FEATURE_EXPERIMENT);
9771029
experiments.add(EXPERIMENT_PAUSED_EXPERIMENT);
9781030
experiments.add(EXPERIMENT_LAUNCHED_EXPERIMENT);
1031+
experiments.add(EXPERIMENT_WITH_MALFORMED_AUDIENCE);
9791032

9801033
// list featureFlags
9811034
List<FeatureFlag> featureFlags = new ArrayList<FeatureFlag>();

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,12 @@
1616
*/
1717
package com.optimizely.ab.config.audience;
1818

19+
import com.optimizely.ab.config.Experiment;
1920
import org.junit.Before;
2021
import org.junit.Test;
2122

2223
import java.util.ArrayList;
24+
import java.util.Collections;
2325
import java.util.HashMap;
2426
import java.util.List;
2527
import java.util.Map;
@@ -187,4 +189,28 @@ public void andConditionEvaluateFalse() throws Exception {
187189
// shouldn't be called due to short-circuiting in 'And' evaluation
188190
verify(orCondition2, times(0)).evaluate(testUserAttributes);
189191
}
192+
193+
/**
194+
* Verify that {@link UserAttribute#evaluate(Map)}
195+
* called when its attribute value is null
196+
* returns True when the user's attribute value is also null
197+
* True when the attribute is not in the map
198+
* False when empty string is used.
199+
* @throws Exception
200+
*/
201+
@Test
202+
public void nullValueEvaluate() throws Exception {
203+
String attributeName = "attribute_name";
204+
String attributeType = "attribute_type";
205+
String attributeValue = null;
206+
UserAttribute nullValueAttribute = new UserAttribute(
207+
attributeName,
208+
attributeType,
209+
attributeValue
210+
);
211+
212+
assertTrue(nullValueAttribute.evaluate(Collections.<String, String>emptyMap()));
213+
assertTrue(nullValueAttribute.evaluate(Collections.singletonMap(attributeName, attributeValue)));
214+
assertFalse(nullValueAttribute.evaluate((Collections.singletonMap(attributeName, ""))));
215+
}
190216
}

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

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@
1616
*/
1717
package com.optimizely.ab.internal;
1818

19-
import com.optimizely.ab.config.audience.Audience;
20-
import com.optimizely.ab.config.audience.Condition;
2119
import com.optimizely.ab.config.Experiment;
2220
import com.optimizely.ab.config.Experiment.ExperimentStatus;
2321
import com.optimizely.ab.config.ProjectConfig;
2422
import com.optimizely.ab.config.TrafficAllocation;
2523
import com.optimizely.ab.config.Variation;
24+
import com.optimizely.ab.config.audience.Audience;
25+
import com.optimizely.ab.config.audience.Condition;
2626
import org.junit.BeforeClass;
2727
import org.junit.Test;
2828

@@ -32,6 +32,10 @@
3232

3333
import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigV2;
3434
import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2;
35+
import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV4;
36+
import static com.optimizely.ab.config.ValidProjectConfigV4.ATTRIBUTE_NATIONALITY_KEY;
37+
import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_WITH_MISSING_VALUE_VALUE;
38+
import static com.optimizely.ab.config.ValidProjectConfigV4.EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY;
3539
import static com.optimizely.ab.internal.ExperimentUtils.isExperimentActive;
3640
import static com.optimizely.ab.internal.ExperimentUtils.isUserInExperiment;
3741
import static org.junit.Assert.assertFalse;
@@ -44,11 +48,13 @@ public class ExperimentUtilsTest {
4448

4549
private static ProjectConfig projectConfig;
4650
private static ProjectConfig noAudienceProjectConfig;
51+
private static ProjectConfig v4ProjectConfig;
4752

4853
@BeforeClass
4954
public static void setUp() throws IOException {
5055
projectConfig = validProjectConfigV2();
5156
noAudienceProjectConfig = noAudienceProjectConfigV2();
57+
v4ProjectConfig = validProjectConfigV4();
5258
}
5359

5460
/**
@@ -152,6 +158,26 @@ public void isUserInExperimentReturnsTrueIfUserDoesNotSatisfyAnyAudiences() {
152158
assertFalse(isUserInExperiment(projectConfig, experiment, attributes));
153159
}
154160

161+
/**
162+
* If there are audiences with attributes on the experiment, but one of the attribute values is null,
163+
* they must explicitly pass in null in order for us to evaluate this. Otherwise we will say they do not match.
164+
*/
165+
@Test
166+
public void isUserInExperimentHandlesNullValue() {
167+
Experiment experiment = v4ProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_WITH_MALFORMED_AUDIENCE_KEY);
168+
Map<String, String> satisfiesFirstCondition = Collections.singletonMap(ATTRIBUTE_NATIONALITY_KEY,
169+
AUDIENCE_WITH_MISSING_VALUE_VALUE);
170+
Map<String, String> attributesWithNull = Collections.singletonMap(ATTRIBUTE_NATIONALITY_KEY, null);
171+
Map<String, String> nonMatchingMap = Collections.singletonMap(ATTRIBUTE_NATIONALITY_KEY, "American");
172+
173+
assertTrue(isUserInExperiment(v4ProjectConfig, experiment, satisfiesFirstCondition));
174+
assertTrue(isUserInExperiment(v4ProjectConfig, experiment, attributesWithNull));
175+
assertFalse(isUserInExperiment(v4ProjectConfig, experiment, nonMatchingMap));
176+
177+
// It should explicitly be set to null otherwise we will return false on empty maps
178+
assertFalse(isUserInExperiment(v4ProjectConfig, experiment, Collections.<String, String>emptyMap()));
179+
}
180+
155181
/**
156182
* Helper method to create an {@link Experiment} object with the provided status.
157183
*

‎core-api/src/test/resources/config/valid-project-config-v4.json‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@
1919
"id": "4194404272",
2020
"name": "english_citizens",
2121
"conditions": "[\"and\", [\"or\", [\"or\", {\"name\": \"nationality\", \"type\": \"custom_dimension\", \"value\":\"English\"}]]]"
22+
},
23+
{
24+
"id": "2196265320",
25+
"name": "audience_with_missing_value",
26+
"conditions": "[\"and\", [\"or\", [\"or\", {\"name\": \"nationality\", \"type\": \"custom_dimension\", \"value\": \"English\"}, {\"name\": \"nationality\", \"type\": \"custom_dimension\"}]]]"
2227
}
2328
],
2429
"attributes": [
@@ -266,6 +271,27 @@
266271
],
267272
"audienceIds": [],
268273
"forcedVariations": {}
274+
},
275+
{
276+
"id": "748215081",
277+
"key": "experiment_with_malformed_audience",
278+
"layerId": "1238149537",
279+
"status": "Running",
280+
"variations": [
281+
{
282+
"id": "535538389",
283+
"key": "var1",
284+
"variables": []
285+
}
286+
],
287+
"trafficAllocation": [
288+
{
289+
"entityId": "535538389",
290+
"endOfRange": 10000
291+
}
292+
],
293+
"audienceIds": ["2196265320"],
294+
"forcedVariations": {}
269295
}
270296
],
271297
"groups": [

0 commit comments

Comments
 (0)