Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New audience match types #213

Merged
merged 39 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
e6a72c4
update with some thoughts on how to implement new match types.
thomaszurkan-optimizely Aug 31, 2018
c77ab0b
add exists
thomaszurkan-optimizely Sep 4, 2018
a757dde
get the lt gt logic correct
thomaszurkan-optimizely Sep 4, 2018
42a4590
suppress unread field in 'exists' match type
thomaszurkan-optimizely Sep 4, 2018
d63ed0b
nullmatch, substringmatch implementaiton
patrickshih-optimizely Sep 4, 2018
fd79cb7
unit tests
patrickshih-optimizely Sep 5, 2018
775be99
fix Exact to pass back null if the value types are different. added …
thomaszurkan-optimizely Sep 5, 2018
4fbc2e4
unit test descriptions
patrickshih-optimizely Sep 5, 2018
b20e730
update condition evaluation to return null under matrix given here ht…
thomaszurkan-optimizely Sep 6, 2018
1a66177
Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…
thomaszurkan-optimizely Sep 6, 2018
d6f762e
some unit tests (and)
patrickshih-optimizely Sep 7, 2018
86ac434
add new match type?
thomaszurkan-optimizely Sep 7, 2018
1aa49fd
Merge branch 'master' into newAudienceMatchTypes
thomaszurkan-optimizely Sep 10, 2018
23cd6e1
update tests after evaluation of audience with no attributes
thomaszurkan-optimizely Sep 10, 2018
714a44d
Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…
thomaszurkan-optimizely Sep 10, 2018
c480c8b
bring up test coverage
thomaszurkan-optimizely Sep 11, 2018
1b4bd6c
refactor for java standard 1 class per file.
thomaszurkan-optimizely Sep 12, 2018
ac62a99
trying to get lint to pass java 9
thomaszurkan-optimizely Sep 12, 2018
61a1e1f
trying to pass findbugs java 9
thomaszurkan-optimizely Sep 12, 2018
e83698c
just seeing if I can get java 9 to pass again
thomaszurkan-optimizely Sep 12, 2018
988579d
trying to get travis to pass
thomaszurkan-optimizely Sep 12, 2018
3bc466b
i don't know why findbugs is finding problems in java 9
thomaszurkan-optimizely Sep 12, 2018
7ae8484
allow for typedAudiences to exist or not exist in the datafile.
thomaszurkan-optimizely Sep 12, 2018
e72aab5
added typed attributes to v4 datafile parse and compare them. Need t…
thomaszurkan-optimizely Sep 13, 2018
20dddcf
trying to get travis jdk 9 failures
thomaszurkan-optimizely Sep 13, 2018
51e18c3
allow null to be returned from evaluate
thomaszurkan-optimizely Sep 13, 2018
685982e
added unit tests to test AND and OR conditions with null, false, and …
thomaszurkan-optimizely Sep 13, 2018
01021d6
add typed_audience_experiment
thomaszurkan-optimizely Sep 15, 2018
03ab597
added tests using datafile with typed audience with experiment using …
thomaszurkan-optimizely Sep 15, 2018
b24db00
add exists test. Cleanup code via mike's comments.
thomaszurkan-optimizely Sep 17, 2018
91d0752
add logging, rename custom dimension, clean up code a little
thomaszurkan-optimizely Sep 18, 2018
9cc8dfa
remove unused import
thomaszurkan-optimizely Sep 18, 2018
4ba4555
nit cleanup
thomaszurkan-optimizely Sep 18, 2018
a159e4c
rename match interface and abstract base class.
thomaszurkan-optimizely Sep 19, 2018
0a738d3
Merge branch 'master' into newAudienceMatchTypes
thomaszurkan-optimizely Sep 19, 2018
35df63e
Merge branch 'master' into newAudienceMatchTypes
thomaszurkan-optimizely Sep 24, 2018
7c63338
refactor to incorporate Nikhil's comments
thomaszurkan-optimizely Oct 2, 2018
030a0d6
Merge branch 'newAudienceMatchTypes' of https://github.com/optimizely…
thomaszurkan-optimizely Oct 2, 2018
ec4385a
Merge branch 'master' into newAudienceMatchTypes
thomaszurkan-optimizely Oct 2, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,5 @@ branches:
- /^\d+\.\d+\.(\d|[x])+(-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
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public String toString() {
private final Boolean botFiltering;
private final List<Attribute> attributes;
private final List<Audience> audiences;
private final List<Audience> typedAudiences;
private final List<EventType> events;
private final List<Experiment> experiments;
private final List<FeatureFlag> featureFlags;
Expand Down Expand Up @@ -136,6 +137,7 @@ public ProjectConfig(String accountId, String projectId, String version, String
version,
attributes,
audiences,
null,
eventType,
experiments,
null,
Expand All @@ -154,6 +156,7 @@ public ProjectConfig(String accountId,
String version,
List<Attribute> attributes,
List<Audience> audiences,
List<Audience> typedAudiences,
List<EventType> events,
List<Experiment> experiments,
List<FeatureFlag> featureFlags,
Expand All @@ -170,6 +173,14 @@ public ProjectConfig(String accountId,

this.attributes = Collections.unmodifiableList(attributes);
this.audiences = Collections.unmodifiableList(audiences);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (typedAudiences != null) {
this.typedAudiences = Collections.unmodifiableList(typedAudiences);
}
else {
this.typedAudiences = Collections.emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

}

this.events = Collections.unmodifiableList(events);
if (featureFlags == null) {
this.featureFlags = Collections.emptyList();
Expand Down Expand Up @@ -206,7 +217,14 @@ public ProjectConfig(String accountId,
this.featureKeyMapping = ProjectConfigUtils.generateNameMapping(this.featureFlags);

// generate audience id to audience mapping
this.audienceIdMapping = ProjectConfigUtils.generateIdMapping(audiences);
if (typedAudiences == null) {
this.audienceIdMapping = ProjectConfigUtils.generateIdMapping(audiences);
}
else {
List<Audience> combinedList = new ArrayList<>(audiences);
combinedList.addAll(typedAudiences);
this.audienceIdMapping = ProjectConfigUtils.generateIdMapping(combinedList);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A code comment and unit tests would help!

Bump

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
this.experimentIdMapping = ProjectConfigUtils.generateIdMapping(this.experiments);
this.groupIdMapping = ProjectConfigUtils.generateIdMapping(groups);
this.rolloutIdMapping = ProjectConfigUtils.generateIdMapping(this.rollouts);
Expand Down Expand Up @@ -386,6 +404,10 @@ public List<Audience> getAudiences() {
return audiences;
}

public List<Audience> getTypedAudiences() {
return typedAudiences;
}

public Condition getAudienceConditionsFromId(String audienceId) {
Audience audience = audienceIdMapping.get(audienceId);

Expand Down Expand Up @@ -581,6 +603,7 @@ public String toString() {
", botFiltering=" + botFiltering +
", attributes=" + attributes +
", audiences=" + audiences +
", typedAudiences=" + typedAudiences +
", events=" + events +
", experiments=" + experiments +
", featureFlags=" + featureFlags +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.List;
import java.util.Map;
Expand All @@ -36,13 +37,32 @@ public List<Condition> getConditions() {
return conditions;
}

public boolean evaluate(Map<String, ?> attributes) {
public @Nullable
Boolean evaluate(Map<String, ?> attributes) {
boolean foundNull = false;
// According to the matrix where:
// false and true is false
// false and null is false
// true and null is null.
// true and false is false
// true and true is true
// null and null is null
for (Condition condition : conditions) {
if (!condition.evaluate(attributes))
Boolean conditionEval = condition.evaluate(attributes);
if (conditionEval == null) {
foundNull = true;
}
else if (!conditionEval) { // false with nulls or trues is false.
return false;
}
// true and nulls with no false will be null.
}

return true;
if (foundNull) { // true and null or all null returns null
return null;
}

return true; // otherwise, return true
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
*/
package com.optimizely.ab.config.audience;

import javax.annotation.Nullable;
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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package com.optimizely.ab.config.audience;

import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
import javax.annotation.Nonnull;

Expand All @@ -37,8 +38,9 @@ public Condition getCondition() {
return condition;
}

public boolean evaluate(Map<String, ?> attributes) {
return !condition.evaluate(attributes);
public @Nullable Boolean evaluate(Map<String, ?> attributes) {
Boolean conditionEval = condition.evaluate(attributes);
return (conditionEval == null ? null : !conditionEval);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.List;
import java.util.Map;
Expand All @@ -36,10 +37,26 @@ public List<Condition> getConditions() {
return conditions;
}

public boolean evaluate(Map<String, ?> attributes) {
// According to the matrix:
// true returns true
// false or null is null
// false or false is false
// null or null is null
public @Nullable Boolean evaluate(Map<String, ?> attributes) {
boolean foundNull = false;
for (Condition condition : conditions) {
if (condition.evaluate(attributes))
Boolean conditionEval = condition.evaluate(attributes);
if (conditionEval == null) { // true with falses and nulls is still true
foundNull = true;
}
else if (conditionEval) {
return true;
}
}

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

return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package com.optimizely.ab.config.audience;

import com.optimizely.ab.config.audience.match.MatchType;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
Expand All @@ -29,11 +31,13 @@ public class UserAttribute implements Condition {

private final String name;
private final String type;
private final String match;
private final Object value;

public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable Object value) {
public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String match, @Nullable Object value) {
this.name = name;
this.type = type;
this.match = match;
this.value = value;
}

Expand All @@ -45,25 +49,31 @@ public String getType() {
return type;
}

public String getMatch() {
return match;
}

public Object getValue() {
return value;
}

public boolean evaluate(Map<String, ?> attributes) {
public @Nullable Boolean evaluate(Map<String, ?> attributes) {
// Valid for primative types, but needs to change when a value is an object or an array
Object userAttributeValue = attributes.get(name);

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works when the variable is null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking @thomaszurkan-optimizely!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

MatchType.logger.error(String.format("condition type not equal to `custom_attribute` %s", type != null ? type : ""));
return null; // unknown type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added logging instead.

}
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;
// check user attribute value is equal
try {
return MatchType.getMatchType(match, value).getMatcher().eval(userAttributeValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return null;
}

}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.config.audience.match;

abstract class AttributeMatch<T> implements Match {
T convert(Object o) {
try {
T rv = (T)o;
return rv;
} catch(java.lang.ClassCastException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding logging

MatchType.logger.error(
"Cannot evaluate targeting condition since the value for attribute is an incompatible type",
e
);
return null;
Copy link
Contributor

@nchilada nchilada Sep 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.config.audience.match;

import javax.annotation.Nullable;

/**
* This is a temporary class. It mimics the current behaviour for
* legacy custom attributes. This will be dropped for ExactMatch and the unit tests need to be fixed.
* @param <T>
*/
class DefaultMatchForLegacyAttributes<T> extends AttributeMatch<T> {
T value;
protected DefaultMatchForLegacyAttributes(T value) {
this.value = value;
}

public @Nullable
Boolean eval(Object attributeValue) {
return value.equals(convert(attributeValue));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
*
* Copyright 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.config.audience.match;

import javax.annotation.Nullable;

class ExactMatch<T> extends AttributeMatch<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

T value;
protected ExactMatch(T value) {
this.value = value;
}

public @Nullable
Boolean eval(Object attributeValue) {
if (!value.getClass().isInstance(attributeValue)) return null;
return value.equals(convert(attributeValue));
}
}
Loading