New audience match types #213

thomaszurkan-optimizely · 2018-09-10T19:33:33Z

New match types along with leaf matchers
Changed logic of AND,OR,NOT conditions to allow for nulls to bubble through appropriately.
Change audience evaluation to always evaluate regardless if there was an attribute key passed in or not.
Still allows null in your attribute list.
Added match to UserAttribute in preparation of typedAttributes. Added to all json parsers as a default.
Parse typedAudiences and include in v4 datafile validation. V3 and V2

…ustomDimensionMatcher to mimic current custom dimension behavior. This should be changed to use Exact.

…tps://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#

…/java-sdk into newAudienceMatchTypes

coveralls · 2018-09-10T19:39:10Z

Pull Request Test Coverage Report for Build 658

155 of 162 (95.68%) changed or added relevant lines in 21 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.4%) to 89.381%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchType.java	25	26	96.15%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	7	10	70.0%
core-api/src/main/java/com/optimizely/ab/config/audience/match/AttributeMatch.java	3	6	50.0%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/event/LogEvent.java	1	83.33%

Totals
Change from base Build 655:	0.4%
Covered Lines:	2500
Relevant Lines:	2797

💛 - Coveralls

mikeproeng37 · 2018-09-11T17:03:40Z

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

+    Boolean evaluate(Map<String, ?> attributes) {
+        boolean foundNull = false;
+        // https://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#
+        // According to the matix mentioned in the above document.


nit: typo in matrix

mikeproeng37 · 2018-09-11T18:14:46Z

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

+ * custom dimension.  This will be dropped for ExactMatch and the unit tests need to be fixed.
+ * @param <T>
+ */
+class CustomDimensionMatch<T> extends LeafMatch<T> {


We don't support custom dimensions anymore, even in 1.x. I think we can safely omit this.

mikeproeng37 · 2018-09-11T18:19:26Z

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

+}
+
+abstract class LeafMatch<T> implements LeafMatcher {
+    T convert(Object o) {


Please document this method.

Also, I think we should be letting these throw so we can log the type mismatch as a debug level log?

mikeproeng37 · 2018-09-11T18:24:55Z

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

+    private LeafMatcher matcher;
+
+    public static MatchType getMatchType(String type, Object value) {
+        if (type == null) type = "custom_dimension";


I am not sure this is accurate, we should default to exact right?

mikeproeng37 · 2018-09-11T18:25:51Z

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

+    }
+
+    public @Nullable Boolean eval(Object otherValue) {
+        return null;


This is a bit confusing, shouldn't this match the value to null?

mikeproeng37 · 2018-09-11T18:26:27Z

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

@@ -36,10 +37,22 @@ public OrCondition(@Nonnull List<Condition> conditions) {
        return conditions;
    }

-    public boolean evaluate(Map<String, ?> attributes) {
+    // https://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#
+    // According to the matix mentioned in the above document.


nit: typo in matrix

mikeproeng37 · 2018-09-11T18:32:03Z

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

    public Object getValue() {
        return value;
    }

-    public boolean evaluate(Map<String, ?> attributes) {
+    public Boolean evaluate(Map<String, ?> attributes) {


Please add unit tests for this method.

mikeproeng37 · 2018-09-11T18:32:40Z

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

@@ -0,0 +1,187 @@
+package com.optimizely.ab.config.audience;


Please add unit tests for the Match types as well as the different Conditions classes, especially around the evaluate logic.

… write more unit tests that use them

…true

…typed audience.

mikeproeng37

Looks good for the most part. Sending it back for thoughts on bubbling up exceptions so we can log proper debug messages

mikeproeng37 · 2018-09-17T17:21:12Z

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

+    public @Nullable
+    Boolean evaluate(Map<String, ?> attributes) {
+        boolean foundNull = false;
+        // https://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#


Please remove this as this is public and the doc is not :)

mikeproeng37 · 2018-09-17T17:26:05Z

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

@@ -36,10 +37,22 @@ public OrCondition(@Nonnull List<Condition> conditions) {
        return conditions;
    }

-    public boolean evaluate(Map<String, ?> attributes) {
+    // https://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#


Please remove this

mikeproeng37 · 2018-09-17T17:30:49Z

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

        for (Condition condition : conditions) {
-            if (condition.evaluate(attributes))
+            Boolean conditionEval = condition.evaluate(attributes);
+            if (conditionEval == null) {// true with falses and nulls is still true


nit: space before the comment?

mikeproeng37 · 2018-09-17T17:34:03Z

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

-            // check user attribute value is equal
-            return value.equals(userAttributeValue);
+        if (!"custom_attribute".equals(type)) {
+            return null; // unknown type


I wonder, if in these cases, we should be throwing and have the caller catch and log the fact that the types are mismatched. I think it would be extremely valuable for debugging

added logging instead.

mikeproeng37 · 2018-09-17T17:34:40Z

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

+ * custom dimension.  This will be dropped for ExactMatch and the unit tests need to be fixed.
+ * @param <T>
+ */
+class CustomDimensionMatch<T> extends LeafMatch<T> {


I think I asked this before, but do we need this? CustomDimensions are from Classic

If there is no match we use the old style matcher which is captured in CustomDimension (this is before we changed everything to custom attribute. Do you think it should be named DefaultMatchForLegacyAttributes?

mikeproeng37 · 2018-09-17T18:05:09Z

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

@@ -71,6 +71,10 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse

            List<EventType> events = parseEvents((JSONArray)rootObject.get("events"));
            List<Audience> audiences = parseAudiences((JSONArray)parser.parse(rootObject.get("audiences").toString()));
+            List<Audience> typedAudiences =null;


nit: space after =

mikeproeng37 · 2018-09-17T18:05:45Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

@@ -66,14 +66,11 @@ public static boolean isUserInExperiment(@Nonnull ProjectConfig projectConfig,
            return true;
        }

-        // if there are audiences, but no user attributes, the user is not in the experiment.


mikeproeng37 · 2018-09-17T18:06:32Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

-            if (conditions.evaluate(attributes)) {
+            Boolean conditionEval = conditions.evaluate(attributes);
+
+            if (conditionEval != null && conditionEval){


Can this be shortened to if conditionEval == true?

Need to check for null. Auto boxing I believe would throw an exception here.

OK.

Also, nit: space before the opening {

mikeproeng37 · 2018-09-17T18:06:53Z

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

@@ -106,7 +106,7 @@ public void verifyGetExperimentsForInvalidEvent() throws Exception {
    @Test
    public void verifyGetAudienceConditionsFromValidId() throws Exception {
        List<Condition> userAttributes = new ArrayList<Condition>();
-        userAttributes.add(new UserAttribute("browser_type", "custom_attribute", "firefox"));
+        userAttributes.add(new UserAttribute("browser_type", "custom_attribute", null,"firefox"));


nit: space after null

mikeproeng37 · 2018-09-17T18:07:10Z

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

@@ -245,7 +245,7 @@ private static ProjectConfig generateValidProjectConfigV3() {
                                        new EventType("100", "no_running_experiments", singletonList("118")));

        List<Condition> userAttributes = new ArrayList<Condition>();
-        userAttributes.add(new UserAttribute("browser_type", "custom_attribute", "firefox"));
+        userAttributes.add(new UserAttribute("browser_type", "custom_attribute", null,"firefox"));


nit: space after null

thomaszurkan-optimizely · 2018-09-18T21:05:12Z

build

mikeproeng37

Almost there, I have a couple of nits and a question on the legacy support. Would like @nchilada to also chime in here.

FYI: I ran the compat suite tests for typed audiences and they all pass on your branch 😄

mikeproeng37 · 2018-09-18T21:50:02Z

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

+            }
+        }
+
+        if (foundNull) {// if found null and false return null.  all false return false


nit: space before the start of the comment

mikeproeng37 · 2018-09-18T21:51:00Z

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

-        if (value != null) { // if there is a value in the condition
-            // check user attribute value is equal
-            return value.equals(userAttributeValue);
+        if (!"custom_attribute".equals(type)) {


nit: Why not invoke the equals method from the variable itself?

this works when the variable is null.

Good thinking @thomaszurkan-optimizely!

mikeproeng37 · 2018-09-18T21:52:17Z

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

+            return convert(otherValue).doubleValue() > value.doubleValue();
+        }
+        catch (Exception e) {
+            MatchType.logger.error("Greater than match ", e);


nit: Can be a bit more verbose: Greater than match failed

the exception will be included in the output making it pretty obvious it is an error.

mikeproeng37 · 2018-09-18T21:52:22Z

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

+            return convert(otherValue).doubleValue() < value.doubleValue();
+        }
+        catch (Exception e) {
+            MatchType.logger.error("Less than match ", e);


Same as above

mikeproeng37 · 2018-09-18T21:53:20Z

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

+                    return new MatchType(type, new LTMatch((Number) value));
+                }
+                break;
+            case "legacy_custom_attribute":


Huh? Is this something that we are populating in the datafile? @nchilada I don't recall seeing this in the design doc. Isn't this the default case?

No, I changed the legacy so that if no match type is provide, I try and use the legacy custom attribute matcher.

It's set at the top of the method when match is null (name="browser", type="custom_attribute" value="firefox") would end up using legacy. I thought changing the name would make that clearer. :(

@thomaszurkan-optimizely I think it would be sufficient—and less confusing, IMO—to just use "exact" instead of defining a new "legacy_custom_attribute" match type and bifurcating the code.

mikeproeng37 · 2018-09-18T21:53:42Z

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

+            return value.contains(convert(otherValue));
+        }
+        catch (Exception e) {
+            MatchType.logger.error("Substring match ", e);


Substring match failed

mikeproeng37 · 2018-09-18T21:54:45Z

core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java

-            if (conditions.evaluate(attributes)) {
+            Boolean conditionEval = conditions.evaluate(attributes);
+
+            if (conditionEval != null && conditionEval){


OK.

Also, nit: space before the opening {

mikeproeng37

lgtm

aliabbasrizvi · 2018-09-19T19:22:16Z

.travis.yml

@@ -26,3 +26,6 @@ branches:
  - /^\d+\.\d+\.\d+(-SNAPSHOT|-alpha|-beta)?\d*$/ # trigger builds on tags which are semantically versioned to ship the SDK.
 after_success:
  - ./gradlew coveralls uploadArchives --console plain
+after_failure:
+  - cat /home/travis/build/optimizely/java-sdk/core-api/build/reports/findbugs/main.html
+


nit. Remove extraneous line.

aliabbasrizvi · 2018-09-19T19:24:04Z

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

+            this.typedAudiences = Collections.unmodifiableList(typedAudiences);
+        }
+        else {
+            this.typedAudiences = Collections.emptyList();


Wouldn't it make life easier if this.typedAudiences = Collections.unmodifiableList(audiences) instead?

@aliabbasrizvi I think that might be unnecessary and confusing. We still make use of this.audences since audiences that happen to be backwards-compatible are still going to be serialized in audiences (rather than in typedAudiences).

aliabbasrizvi · 2018-09-19T19:24:42Z

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

 import java.util.Map;

 /**
 * Interface implemented by all conditions condition objects to aid in condition evaluation.
 */
 public interface Condition {

-    boolean evaluate(Map<String, ?> attributes);
+    @Nullable
+    Boolean evaluate(Map<String, ?> attributes);
 }


nit. Insert new line here.

aliabbasrizvi · 2018-09-19T19:25:42Z

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

+          "endOfRange": 10000
+        }
+      ],
+      "audienceIds": ["3468206643","3468206644","3468206645"],


nit. Insert space after ,

aliabbasrizvi · 2018-09-19T19:27:45Z

...i/src/main/java/com/optimizely/ab/config/audience/match/DefaultMatchForLegacyAttributes.java

+
+/**
+ * This is a temporary class.  It mimics the current behaviour for
+ * legasy custom attributes.  This will be dropped for ExactMatch and the unit tests need to be fixed.


typo. legacy

aliabbasrizvi · 2018-09-19T19:28:47Z

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

+ */
+package com.optimizely.ab.config.audience.match;
+
+abstract class LeafMatch<T> implements LeafMatcher {


Not a big fan of this name LeafMatch. May be just Match?

aliabbasrizvi · 2018-09-19T19:29:07Z

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

+ */
+package com.optimizely.ab.config.audience.match;
+
+public interface LeafMatcher {


Same feedback as above. Perhaps just call this Matcher

aliabbasrizvi

Setting to request changes.

nchilada

I'll get to MatchType, AttributeMatch, or their subclasses later today, but I finished the ProjectConfig and Condition side, and everything looks good overall! Nice work @thomaszurkan-optimizely, I only have minor feedback so far

nchilada · 2018-09-21T19:31:59Z

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

+            this.typedAudiences = Collections.unmodifiableList(typedAudiences);
+        }
+        else {
+            this.typedAudiences = Collections.emptyList();


@aliabbasrizvi I think that might be unnecessary and confusing. We still make use of this.audences since audiences that happen to be backwards-compatible are still going to be serialized in audiences (rather than in typedAudiences).

nchilada · 2018-09-21T19:35:09Z

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

+        else {
+            List<Audience> combinedList = new ArrayList<>(audiences);
+            combinedList.addAll(typedAudiences);
+            this.audienceIdMapping = ProjectConfigUtils.generateIdMapping(combinedList);


If a given audience ID is found in both audiences and typedAudiences, we want to use the audience in typedAudiences. Are we sure this code achieves that? Do we have a unit test for this?

yes. that is exactly the point of this combination. the map is audience ids. The first set of mappings is for audiences. If the Id exists, it is replaced when the typed audiences is added to the map.

Okay thanks. That's quite implicit but I guess it works. A code comment and unit tests would help!

A code comment and unit tests would help!

Bump

This is in a bunch of unit tests that I added with V4 datafile with typedAudiences. They share Ids for the string exact ones (i.e. legacy and typed). The audience id map is used to get the audience and uses the combined map in question.

Oh cool! Didn't see that just now, but that's perfect, thanks.

nchilada · 2018-09-21T19:38:40Z

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

@@ -69,6 +69,11 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse

            List<EventType> events = parseEvents(rootObject.getJSONArray("events"));
            List<Audience> audiences = parseAudiences(rootObject.getJSONArray("audiences"));


Can we gate this with if (rootObject.has("audiences")), the same way we're handling typedAudiences?

So that app backend isn't tied down with audiences if we decide to move entirely to typedAudiences at some point. Semantically, it seems reasonable to make audiences optional once typedAudiences is supported too.

nchilada · 2018-09-21T19:39:10Z

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

@@ -70,6 +70,10 @@ public ProjectConfig deserialize(JsonElement json, Type typeOfT, JsonDeserializa
        List<Audience> audiences =
            context.deserialize(jsonObject.get("audiences").getAsJsonArray(), audienceType);


Can we gate this with if (rootObject.has("audiences")), the same way we're handling typedAudiences?

nchilada · 2018-09-21T19:40:16Z

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

@@ -66,6 +66,14 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte
        List<Audience> audiences = mapper.readValue(node.get("audiences").toString(),
                                                    new TypeReference<List<Audience>>() {});


Can we gate this with if (rootObject.has("audiences")), the same way we're handling typedAudiences?

nchilada · 2018-09-21T20:16:13Z

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

+      "name": "audience_with_missing_value",
+      "conditions": "[\"and\", [\"or\", [\"or\", {\"name\": \"nationality\", \"type\": \"custom_attribute\", \"value\": \"English\"}, {\"name\": \"nationality\", \"type\": \"custom_attribute\"}]]]"
+    }
+  ],


Since you'd asked me to check: this looks accurate. But I'd also test with an audience whose ID is found in both audiences and typedAudiences.

It might also be wise to include audience conditions with match being null instead of being undefined, though I have no intention of having the app backend serialize such conditions. 🤷

All the string conditions that map to exact are also in the audience list and replace the audience lookup map with the typed audience since they share ids. Look at the audience list above typed audience definition.

Ah, gotcha. I should've expanded the whole file and checked 😬

nchilada · 2018-09-21T20:26:52Z

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

-        if (value != null) { // if there is a value in the condition
-            // check user attribute value is equal
-            return value.equals(userAttributeValue);
+        if (!"custom_attribute".equals(type)) {


Good thinking @thomaszurkan-optimizely!

nchilada · 2018-09-21T20:29:22Z

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

-        if (value != null) { // if there is a value in the condition
-            // check user attribute value is equal
-            return value.equals(userAttributeValue);
+        if (!"custom_attribute".equals(type)) {


Once we support other condition types we'll probably want to do this check in ProjectConfig or wherever. But this is fine for now. I'm not overly concerned about the fact that we're constructing CustomAttribute objects for things that might not semantically represent custom_attribute conditions.

nchilada · 2018-09-21T20:31:14Z

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

-            return false;
+        // check user attribute value is equal
+        try {
+            return MatchType.getMatchType(match, value).getMatcher().eval(userAttributeValue);


Based on the name alone, I find it surprising that we're passing value to .getMatchType(...) and not to .getMatcher(...) or .eval(...). That said, this is minor and I haven't looked at those functions yet.

nchilada · 2018-09-21T20:32:06Z

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

-        else { // both are null
-            return true;
+        catch (NullPointerException np) {
+            MatchType.logger.error(String.format("attribute or value null for match %s", match != null ? match : "legacy condition"),np);


We can definitely wait until after the initial PR is merged, but we may want to have super-specific error messages. cc @ceimaj

nchilada · 2018-09-21T23:00:29Z

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

+                    return new MatchType(type, new LTMatch((Number) value));
+                }
+                break;
+            case "legacy_custom_attribute":


@thomaszurkan-optimizely I think it would be sufficient—and less confusing, IMO—to just use "exact" instead of defining a new "legacy_custom_attribute" match type and bifurcating the code.

nchilada · 2018-09-21T23:02:26Z

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

+    private String type;
+    private Match matcher;
+
+    public static MatchType getMatchType(String type, Object value) {


In the context of this function, maybe rename type -> matchType so it can't be misread as "valueType"?

nchilada · 2018-09-21T23:12:24Z

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

+    private String type;
+    private Match matcher;
+
+    public static MatchType getMatchType(String type, Object value) {


Can we drop the MatchType abstraction and just return a Match object instead? In this PR I only see one call to MatchType.getMatchType(...), and the caller only seems to care about the Match object.

(And if you take this suggestion, it might also make sense to move this static function from MatchType.getMatchType to Match.getMatcher.)

nchilada · 2018-09-21T23:18:32Z

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

+                }
+                break;
+            case "gt":
+                if (value instanceof Number) {


We only track Integers and Doubles, not all Numbers; can we do the same for targeting? That'd make the SDK behavior more consistent and easier to document, IMO.

And this is a design decision (which I've yet to see anyone else weigh in one) so I'll confirm with others offline.

nchilada · 2018-09-21T23:24:38Z

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

+                }
+                break;
+            case "lt":
+                if (value instanceof Number) {


Same as above. I'd rather proceed only if value instanceof Integer or value instanceof Double.

nchilada · 2018-09-21T23:34:35Z

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

+
+import javax.annotation.Nullable;
+
+class ExactMatch<T> extends AttributeMatch<T> {


In Match and all its implementations: maybe rename otherValue -> userValue/userAttributeValue and rename value -> conditionValue?

nchilada · 2018-09-21T23:35:13Z

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

+package com.optimizely.ab.config.audience.match;
+
+public interface Match {
+    Boolean eval(Object otherValue);


In Match and all its implementations: maybe rename otherValue -> userValue/userAttributeValue and rename value -> conditionValue?

nchilada · 2018-09-21T23:36:27Z

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

+package com.optimizely.ab.config.audience.match;
+
+public interface Match {
+    Boolean eval(Object otherValue);


And I'm just curious, since I don't know anything about Java annotations: would @Nullable be relevant here?

nchilada · 2018-09-21T23:43:37Z

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

+        try {
+            T rv = (T)o;
+            return rv;
+        } catch(java.lang.ClassCastException e) {


Can we log this? In principle something like "Cannot evaluate targeting condition since the value for attribute is of an incompatible value"

Adding logging

nchilada · 2018-09-22T00:04:08Z

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

+            T rv = (T)o;
+            return rv;
+        } catch(java.lang.ClassCastException e) {
+            return null;


Swallowing the error and returning null feels pretty weird to me. This causes subclasses' eval(...) methods to proceed blindly with a null value (in some cases producing NullPointerExceptions which they must know to catch) when the more straightforward, robust behavior might be for them to immediately return null.

Can we try a slightly different pattern? Maybe something like

abstract class TypedMatch<T> implements Match { Boolean eval(Object otherValue) { try { T otherValueT = (T) otherValue; } catch(java.lang.ClassCastException e) { return null; } return eval(typedOtherValue); } Boolean eval(T otherValue); }

with our subclasses implementing eval(T otherValue) instead of eval(Object otherValue).

(Or should we give up on having common, generic-powered type-checking code? But maybe that's just my dynamic typing background coming through 🤷)

This is a great idea and I really like where you are going with it. However, in Java (and other strongly typed languages) the eval here ends up with the same signature. Meaning a value is both of type object and type t. So, it becomes ambiguous. :(

Ah, interesting, thanks.

I think this is technically fine. Since we won't ever be constructing instances of AttributeMatch<String>, AttributeMatch<Number>, AttributeMatch<Boolean>, etc. where this.value (the condition value) is null, we won't accidentally evaluate to true when the value returned by convert is compared to this.value.

The customer may see NullPointerException or some other exception that doesn't specifically talk about the type mismatch, but that's not my top concern.

BTW I was hoping for us to support conditions like {..., "match": "exact", "value": null}. That doesn't work so well with the current scheme, but I assume we could hack it if necessary. It's certainly not worth optimizing for 🤷‍♂️

nchilada · 2018-09-26T17:59:15Z

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

+                condition = new OrCondition(conditions);
+                break;
+            default:
+                condition = new NotCondition(conditions.get(0));


After discussing with @mjc1283: we should also return null instead of raising IndexOutOfBoundsException if conditions.length == 0.

Is there ever going to be a audience without a condition (not to be confused with audience id list)?

Is there ever going to be a audience without a condition (not to be confused with audience id list)?

As we started to discuss in Slack: if an audience object has null or missing conditions, I think that's rather problematic and it's appropriate for the SDK to reject the datafile. On the other hand, if the conditions are empty ([], {}, etc.) then it's fine to apply our recursive evaluation algorithm to that value.

…/java-sdk into newAudienceMatchTypes

nchilada

Looks good, just some minor feedback since this is a reference SDK.

As a heads up, there are a couple smaller changes that we couldn't get to here but which I'd like to make separately

Recognizing audience conditions that aren't double-JSON-serialized in the datafile
Inferring “or” rather than “not” if we see a composite condition whose first element isn't "and"/"or"/"not".

nchilada · 2018-10-02T20:09:34Z

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

+            T rv = (T)o;
+            return rv;
+        } catch(java.lang.ClassCastException e) {
+            return null;


I think this is technically fine. Since we won't ever be constructing instances of AttributeMatch<String>, AttributeMatch<Number>, AttributeMatch<Boolean>, etc. where this.value (the condition value) is null, we won't accidentally evaluate to true when the value returned by convert is compared to this.value.

The customer may see NullPointerException or some other exception that doesn't specifically talk about the type mismatch, but that's not my top concern.

BTW I was hoping for us to support conditions like {..., "match": "exact", "value": null}. That doesn't work so well with the current scheme, but I assume we could hack it if necessary. It's certainly not worth optimizing for 🤷‍♂️

nchilada · 2018-10-02T20:16:16Z

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

-        List<Audience> audiences = mapper.readValue(node.get("audiences").toString(),
-                                                    new TypeReference<List<Audience>>() {});
+
+        List<Audience> audiences = Collections.emptyList();


Same question as above - can we initialize to a consistent value?

nchilada · 2018-10-02T20:17:11Z

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

@@ -68,7 +69,17 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse
            attributes = parseAttributes(rootObject.getJSONArray("attributes"));

            List<EventType> events = parseEvents(rootObject.getJSONArray("events"));
-            List<Audience> audiences = parseAudiences(rootObject.getJSONArray("audiences"));
+            List<Audience> audiences = Collections.emptyList();


Why is this initialized to Collections.emptyList() while the typed version is initialized to null? Can we make these consistent with each other?

backward compatibility. A v2 or V3 would possibly have problems with a null audience.

Whereas typedAudience is new and will be null in V2 and V3

Hmm, I think it's just a question of whether the missing audience arrays get infilled with empty lists in the various JSON parsers vs. in the ProjectConfig constructors, but shouldn't affect correctness either way.

Thanks for making both audiences and typedAudiences optional in the JSON config! 😊

nchilada · 2018-10-02T20:17:41Z

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

@@ -70,7 +71,17 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse
            attributes = parseAttributes((JSONArray)rootObject.get("attributes"));

            List<EventType> events = parseEvents((JSONArray)rootObject.get("events"));
-            List<Audience> audiences = parseAudiences((JSONArray)parser.parse(rootObject.get("audiences").toString()));
+            List<Audience> audiences = Collections.emptyList();


Same question as above - can we initialize to a consistent value?

nchilada · 2018-10-02T20:17:58Z

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

@@ -67,9 +68,15 @@ public ProjectConfig deserialize(JsonElement json, Type typeOfT, JsonDeserializa

        List<EventType> events =
            context.deserialize(jsonObject.get("events").getAsJsonArray(), eventsType);
-        List<Audience> audiences =
-            context.deserialize(jsonObject.get("audiences").getAsJsonArray(), audienceType);
+        List<Audience> audiences = Collections.emptyList();


Same question as above - can we initialize to a consistent value?

nchilada · 2018-10-02T20:19:30Z

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

@@ -170,6 +173,14 @@ public ProjectConfig(String accountId,

        this.attributes = Collections.unmodifiableList(attributes);
        this.audiences = Collections.unmodifiableList(audiences);


As mentioned above - can we allow the audiences and typedAudiences args are equally nullable?

nchilada · 2018-10-02T20:21:50Z

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

+        else {
+            List<Audience> combinedList = new ArrayList<>(audiences);
+            combinedList.addAll(typedAudiences);
+            this.audienceIdMapping = ProjectConfigUtils.generateIdMapping(combinedList);


A code comment and unit tests would help!

Bump

thomaszurkan-optimizely and others added 15 commits August 31, 2018 10:22

update with some thoughts on how to implement new match types.

e6a72c4

add exists

c77ab0b

get the lt gt logic correct

a757dde

suppress unread field in 'exists' match type

42a4590

nullmatch, substringmatch implementaiton

d63ed0b

unit tests

fd79cb7

fix Exact to pass back null if the value types are different. added C…

775be99

…ustomDimensionMatcher to mimic current custom dimension behavior. This should be changed to use Exact.

unit test descriptions

4fbc2e4

update condition evaluation to return null under matrix given here ht…

b20e730

…tps://docs.google.com/document/d/158_83difXVXF0nb91rxzrfHZwnhsybH21ImRA_si7sg/edit#

Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…

1a66177

…/java-sdk into newAudienceMatchTypes

some unit tests (and)

d6f762e

add new match type?

86ac434

Merge branch 'master' into newAudienceMatchTypes

1aa49fd

update tests after evaluation of audience with no attributes

23cd6e1

Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…

714a44d

…/java-sdk into newAudienceMatchTypes

thomaszurkan-optimizely requested review from mikeproeng37 and nchilada September 10, 2018 19:33

bring up test coverage

c480c8b

mikeproeng37 suggested changes Sep 11, 2018

View reviewed changes

thomaszurkan-optimizely added 10 commits September 12, 2018 10:55

refactor for java standard 1 class per file.

1b4bd6c

trying to get lint to pass java 9

ac62a99

trying to pass findbugs java 9

61a1e1f

just seeing if I can get java 9 to pass again

e83698c

trying to get travis to pass

988579d

i don't know why findbugs is finding problems in java 9

3bc466b

allow for typedAudiences to exist or not exist in the datafile.

7ae8484

added typed attributes to v4 datafile parse and compare them. Need to…

e72aab5

… write more unit tests that use them

trying to get travis jdk 9 failures

20dddcf

allow null to be returned from evaluate

51e18c3

thomaszurkan-optimizely changed the title ~~WIP: New audience match types~~ New audience match types Sep 13, 2018

thomaszurkan-optimizely added 3 commits September 13, 2018 16:35

added unit tests to test AND and OR conditions with null, false, and …

685982e

…true

add typed_audience_experiment

01021d6

added tests using datafile with typed audience with experiment using …

03ab597

…typed audience.

mikeproeng37 suggested changes Sep 17, 2018

View reviewed changes

thomaszurkan-optimizely added 2 commits September 17, 2018 16:51

add exists test. Cleanup code via mike's comments.

b24db00

add logging, rename custom dimension, clean up code a little

91d0752

remove unused import

9cc8dfa

mikeproeng37 reviewed Sep 18, 2018

View reviewed changes

nit cleanup

4ba4555

mikeproeng37 approved these changes Sep 19, 2018

View reviewed changes

aliabbasrizvi reviewed Sep 19, 2018

View reviewed changes

aliabbasrizvi suggested changes Sep 19, 2018

View reviewed changes

thomaszurkan-optimizely added 2 commits September 19, 2018 14:11

rename match interface and abstract base class.

a159e4c

Merge branch 'master' into newAudienceMatchTypes

0a738d3

aliabbasrizvi approved these changes Sep 20, 2018

View reviewed changes

nchilada suggested changes Sep 21, 2018

View reviewed changes

nchilada reviewed Sep 22, 2018

View reviewed changes

Merge branch 'master' into newAudienceMatchTypes

35df63e

nchilada reviewed Sep 26, 2018

View reviewed changes

thomaszurkan-optimizely added 2 commits October 2, 2018 12:50

refactor to incorporate Nikhil's comments

7c63338

Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…

030a0d6

…/java-sdk into newAudienceMatchTypes

nchilada approved these changes Oct 2, 2018

View reviewed changes

Merge branch 'master' into newAudienceMatchTypes

ec4385a

thomaszurkan-optimizely merged commit 684e47a into master Oct 2, 2018

nchilada deleted the newAudienceMatchTypes branch October 2, 2018 22:40

		@@ -69,6 +69,11 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse

		List<EventType> events = parseEvents(rootObject.getJSONArray("events"));
		List<Audience> audiences = parseAudiences(rootObject.getJSONArray("audiences"));

		@@ -70,6 +70,10 @@ public ProjectConfig deserialize(JsonElement json, Type typeOfT, JsonDeserializa
		List<Audience> audiences =
		context.deserialize(jsonObject.get("audiences").getAsJsonArray(), audienceType);

		@@ -66,6 +66,14 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte
		List<Audience> audiences = mapper.readValue(node.get("audiences").toString(),
		new TypeReference<List<Audience>>() {});


		import javax.annotation.Nullable;

		class ExactMatch<T> extends AttributeMatch<T> {

		@@ -170,6 +173,14 @@ public ProjectConfig(String accountId,

		this.attributes = Collections.unmodifiableList(attributes);
		this.audiences = Collections.unmodifiableList(audiences);

New audience match types #213

New audience match types #213

Conversation

thomaszurkan-optimizely commented Sep 10, 2018 • edited Loading

coveralls commented Sep 10, 2018 • edited Loading

Pull Request Test Coverage Report for Build 658

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaszurkan-optimizely commented Sep 18, 2018

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi left a comment

Choose a reason for hiding this comment

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaszurkan-optimizely commented Sep 10, 2018 •

edited

Loading

coveralls commented Sep 10, 2018 •

edited

Loading