-
Notifications
You must be signed in to change notification settings - Fork 32
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
Changes from 33 commits
e6a72c4
c77ab0b
a757dde
42a4590
d63ed0b
fd79cb7
775be99
4fbc2e4
b20e730
1a66177
d6f762e
86ac434
1aa49fd
23cd6e1
714a44d
c480c8b
1b4bd6c
ac62a99
61a1e1f
e83698c
988579d
3bc466b
7ae8484
e72aab5
20dddcf
51e18c3
685982e
01021d6
03ab597
b24db00
91d0752
9cc8dfa
4ba4555
a159e4c
0a738d3
35df63e
7c63338
030a0d6
ec4385a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -136,6 +137,7 @@ public ProjectConfig(String accountId, String projectId, String version, String | |
version, | ||
attributes, | ||
audiences, | ||
null, | ||
eventType, | ||
experiments, | ||
null, | ||
|
@@ -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, | ||
|
@@ -170,6 +173,14 @@ public ProjectConfig(String accountId, | |
|
||
this.attributes = Collections.unmodifiableList(attributes); | ||
this.audiences = Collections.unmodifiableList(audiences); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above - can we allow the |
||
|
||
if (typedAudiences != null) { | ||
this.typedAudiences = Collections.unmodifiableList(typedAudiences); | ||
} | ||
else { | ||
this.typedAudiences = Collections.emptyList(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make life easier if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.events = Collections.unmodifiableList(events); | ||
if (featureFlags == null) { | ||
this.featureFlags = Collections.emptyList(); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a given audience ID is found in both There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Bump There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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); | ||
|
||
|
@@ -581,6 +603,7 @@ public String toString() { | |
", botFiltering=" + botFiltering + | ||
", attributes=" + attributes + | ||
", audiences=" + audiences + | ||
", typedAudiences=" + typedAudiences + | ||
", events=" + events + | ||
", experiments=" + experiments + | ||
", featureFlags=" + featureFlags + | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. Insert new line here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Why not invoke the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this works when the variable is null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thinking @thomaszurkan-optimizely! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
MatchType.logger.error(String.format("condition type not equal to `custom_attribute` %s", type != null ? type : "")); | ||
return null; // unknown type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 | ||
* legasy custom attributes. This will be dropped for ExactMatch and the unit tests need to be fixed. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo. legacy |
||
* @param <T> | ||
*/ | ||
class DefaultMatchForLegacyAttributes<T> extends LeafMatch<T> { | ||
T value; | ||
protected DefaultMatchForLegacyAttributes(T value) { | ||
this.value = value; | ||
} | ||
|
||
public @Nullable | ||
Boolean eval(Object otherValue) { | ||
return value.equals(convert(otherValue)); | ||
} | ||
} |
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 LeafMatch<T> { | ||
T value; | ||
protected ExactMatch(T value) { | ||
this.value = value; | ||
} | ||
|
||
public @Nullable | ||
Boolean eval(Object otherValue) { | ||
if (!value.getClass().isInstance(otherValue)) return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a sanity check, right? |
||
return value.equals(convert(otherValue)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/** | ||
* | ||
* 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 edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
|
||
import javax.annotation.Nullable; | ||
|
||
class ExistsMatch extends LeafMatch<Object> { | ||
@SuppressFBWarnings("URF_UNREAD_FIELD") | ||
Object value; | ||
protected ExistsMatch(Object value) { | ||
this.value = value; | ||
} | ||
|
||
public @Nullable | ||
Boolean eval(Object otherValue) { | ||
return otherValue != null; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. Remove extraneous line.