Skip to content

feat(api): Accepting all types for attributes values #207

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Aug 31, 2018
Merged

feat(api): Accepting all types for attributes values #207

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c401466
wildcard replaces string; implement type checking
patrickshih-optimizely Aug 16, 2018
d73f4eb
deleting invalid types for filtered attributes
patrickshih-optimizely Aug 16, 2018
6173eee
attribute with null values are filtered out
patrickshih-optimizely Aug 16, 2018
be4de1c
resolved issues with null
patrickshih-optimizely Aug 18, 2018
0b23827
paramater/return changes
patrickshih-optimizely Aug 21, 2018
cc9a5a6
remove filtering invalid types
patrickshih-optimizely Aug 21, 2018
20f83bf
Merge branch 'master' into pshih/attribute-types
shihpatrick Aug 22, 2018
881774f
added from suggestions + unit tests
patrickshih-optimizely Aug 22, 2018
6274260
Merge branch 'master' into pshih/attribute-types
patrickshih-optimizely Aug 22, 2018
a47f8af
logger, work-around for unit test
patrickshih-optimizely Aug 23, 2018
e49c473
diamond notation for hashmap
patrickshih-optimizely Aug 23, 2018
20974f4
Merge branch 'master' into pshih/attribute-types
patrickshih-optimizely Aug 23, 2018
6ef444d
Merge branch 'pshih/attribute-types' of github.com:optimizely/java-sd…
patrickshih-optimizely Aug 23, 2018
7c868cb
funky casting to get Map<String,?> working
thomaszurkan-optimizely Aug 23, 2018
9f0593f
fixed unit tests
patrickshih-optimizely Aug 23, 2018
b484960
notification listener, features function signature change
patrickshih-optimizely Aug 23, 2018
4aa66fe
notif. listener tests function signature change
patrickshih-optimizely Aug 23, 2018
7e1dd58
code review changes
patrickshih-optimizely Aug 29, 2018
a17bdbd
Update valid-project-config-v4.json
patrickshih-optimizely Aug 29, 2018
f90b466
Update valid-project-config-v4.json
patrickshih-optimizely Aug 29, 2018
d9d23e2
small changes
patrickshih-optimizely Aug 30, 2018
0b6aad9
Merge branch 'pshih/attribute-types' of github.com:optimizely/java-sd…
patrickshih-optimizely Aug 30, 2018
182c4fe
Update NotificationListener.java
patrickshih-optimizely Aug 30, 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
27 changes: 14 additions & 13 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ Variation activate(@Nonnull String experimentKey,
public @Nullable
Variation activate(@Nonnull String experimentKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) throws UnknownExperimentException {
@Nonnull Map<String, ?> attributes) throws UnknownExperimentException {

if (experimentKey == null) {
logger.error("The experimentKey parameter must be nonnull.");
Expand Down Expand Up @@ -153,7 +153,7 @@ Variation activate(@Nonnull Experiment experiment,
public @Nullable
Variation activate(@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {

ProjectConfig currentConfig = getProjectConfig();

Expand All @@ -164,15 +164,15 @@ Variation activate(@Nonnull Experiment experiment,
Variation activate(@Nonnull ProjectConfig projectConfig,
@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {

if (!validateUserId(userId)){
logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
return null;
}
// determine whether all the given attributes are present in the project config. If not, filter out the unknown
// attributes.
Map<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);

// bucket the user to the given experiment and dispatch an impression event
Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes);
Expand All @@ -189,7 +189,7 @@ Variation activate(@Nonnull ProjectConfig projectConfig,
private void sendImpression(@Nonnull ProjectConfig projectConfig,
@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> filteredAttributes,
@Nonnull Map<String, ?> filteredAttributes,
@Nonnull Variation variation) {
if (experiment.isRunning()) {
LogEvent impressionEvent = eventFactory.createImpressionEvent(
Expand Down Expand Up @@ -259,7 +259,7 @@ public void track(@Nonnull String eventName,

// determine whether all the given attributes are present in the project config. If not, filter out the unknown
// attributes.
Map<String, String> filteredAttributes = filterAttributes(currentConfig, attributes);
Map<String, ?> filteredAttributes = filterAttributes(currentConfig, attributes);

if (eventTags == null) {
logger.warn("Event tags is null when non-null was expected. Defaulting to an empty event tags map.");
Expand Down Expand Up @@ -359,7 +359,7 @@ else if (userId == null) {
return false;
}

Map<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);

FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes);
if (featureDecision.variation != null) {
Expand Down Expand Up @@ -644,7 +644,7 @@ Variation getVariation(@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) throws UnknownExperimentException {

Map<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);

return decisionService.getVariation(experiment, userId, filteredAttributes);
}
Expand Down Expand Up @@ -677,7 +677,7 @@ Variation getVariation(@Nonnull String experimentKey,
return null;
}

Map<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
Map<String, ?> filteredAttributes = filterAttributes(projectConfig, attributes);

return decisionService.getVariation(experiment,userId,filteredAttributes);
}
Expand Down Expand Up @@ -742,17 +742,18 @@ public UserProfileService getUserProfileService() {
* @return the filtered attributes map (containing only attributes that are present in the project config) or an
* empty map if a null attributes object is passed in
*/
private Map<String, String> filterAttributes(@Nonnull ProjectConfig projectConfig,
@Nonnull Map<String, String> attributes) {
private Map<String, ?> filterAttributes(@Nonnull ProjectConfig projectConfig,
@Nonnull Map<String, ?> attributes) {
if (attributes == null) {
logger.warn("Attributes is null when non-null was expected. Defaulting to an empty attributes map.");
return Collections.<String, String>emptyMap();
}

// List of attribute keys
List<String> unknownAttributes = null;

Map<String, Attribute> attributeKeyMapping = projectConfig.getAttributeKeyMapping();
for (Map.Entry<String, String> attribute : attributes.entrySet()) {
for (Map.Entry<String, ?> attribute : attributes.entrySet()) {
if (!attributeKeyMapping.containsKey(attribute.getKey()) &&
!attribute.getKey().startsWith(ProjectConfig.RESERVED_ATTRIBUTE_PREFIX)) {
if (unknownAttributes == null) {
Expand All @@ -765,7 +766,7 @@ private Map<String, String> filterAttributes(@Nonnull ProjectConfig projectConfi
if (unknownAttributes != null) {
logger.warn("Attribute(s) {} not in the datafile.", unknownAttributes);
// make a copy of the passed through attributes, then remove the unknown list
attributes = new HashMap<String, String>(attributes);
attributes = new HashMap<String, Object>(attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a test that actually removes unknown attributes to see what happens here. But my understanding is that Object is actually different from ?.
See https://stackoverflow.com/questions/678822/what-is-the-difference-between-and-object-in-java-generics

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't use a question mark in a constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think that the proper way to use the constructor is HashMap<>(attributes). So, @wangjoshuah is correct.

Please update to something like:
HashMap<String, ?> attr = new HashMap<>();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like <> was not introduced until Java 7 and we're currently on Java 6. Thoughts?

https://stackoverflow.com/questions/16612157/why-does-new-hashmap-produce-an-error-in-jdk-1-6-but-not-1-7?lq=1

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, it sounds like

new HashMap<String, Object>();

is indeed the way to go...

Copy link
Contributor

Choose a reason for hiding this comment

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

#208 is the way to go. Good call @thomaszurkan-optimizely

for (String unknownAttribute : unknownAttributes) {
attributes.remove(unknownAttribute);
}
Expand Down
10 changes: 5 additions & 5 deletions core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
*/
public @Nullable Variation getVariation(@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> filteredAttributes) {
@Nonnull Map<String, ?> filteredAttributes) {

if (!ExperimentUtils.isExperimentActive(experiment)) {
return null;
Expand Down Expand Up @@ -131,7 +131,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
if (ExperimentUtils.isUserInExperiment(projectConfig, experiment, filteredAttributes)) {
String bucketingId = userId;
if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
bucketingId = (String) filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check the type just to be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it isn't a string, should we keep it as userId?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, we don't use the bucketingId if it's not a string and instead keep using the userId

Copy link
Contributor

Choose a reason for hiding this comment

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

And I assume you're on top of this, but we should log a ~warning if we're ignoring the bucketingId due to type mismatch.

And while you're at it...we should also log at ~info level in cases where do ingest a usable bucketingId!

}
variation = bucketer.bucket(experiment, bucketingId);

Expand Down Expand Up @@ -159,7 +159,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
*/
public @Nonnull FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag,
@Nonnull String userId,
@Nonnull Map<String, String> filteredAttributes) {
@Nonnull Map<String, ?> filteredAttributes) {
if (!featureFlag.getExperimentIds().isEmpty()) {
for (String experimentId : featureFlag.getExperimentIds()) {
Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId);
Expand Down Expand Up @@ -195,7 +195,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
*/
@Nonnull FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag,
@Nonnull String userId,
@Nonnull Map<String, String> filteredAttributes) {
@Nonnull Map<String, ?> filteredAttributes) {
// use rollout to get variation for feature
if (featureFlag.getRolloutId().isEmpty()) {
logger.info("The feature flag \"{}\" is not used in a rollout.", featureFlag.getKey());
Expand All @@ -212,7 +212,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
int rolloutRulesLength = rollout.getExperiments().size();
String bucketingId = userId;
if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
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 we should check for null value and that the value is a string.

bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
bucketingId = (String) filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
}
Variation variation;
for (int i = 0; i < rolloutRulesLength - 1; i++) {
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/audience/AndCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public List<Condition> getConditions() {
return conditions;
}

public boolean evaluate(Map<String, String> attributes) {
public boolean evaluate(Map<String, ?> attributes) {
for (Condition condition : conditions) {
if (!condition.evaluate(attributes))
return false;
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@
*/
public interface Condition {

boolean evaluate(Map<String, String> attributes);
boolean evaluate(Map<String, ?> attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the header on this file to be 2016-2018. Please also do it for other files you've touched in this commit.

}
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public Condition getCondition() {
return condition;
}

public boolean evaluate(Map<String, String> attributes) {
public boolean evaluate(Map<String, ?> attributes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the toString method on this class to account for non-string values? Seems like it could lead to exceptions

return !condition.evaluate(attributes);
}

Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public List<Condition> getConditions() {
return conditions;
}

public boolean evaluate(Map<String, String> attributes) {
public boolean evaluate(Map<String, ?> attributes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above re: the toString method

for (Condition condition : conditions) {
if (condition.evaluate(attributes))
return true;
Expand Down
10 changes: 5 additions & 5 deletions core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ public class UserAttribute implements Condition {

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

public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String value) {
public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@thomaszurkan-optimizely do we need to overload this constructor to allow you to pass in a String value? I don't think this change will break current usage of the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean Object value?
Would this change be more appropriate when dealing with audience conditions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I think this will be fine as is. Can ignore my previous comment.

this.name = name;
this.type = type;
this.value = value;
Expand All @@ -45,12 +45,12 @@ public String getType() {
return type;
}

public String getValue() {
public Object getValue() {
return value;
}

public boolean evaluate(Map<String, String> attributes) {
String userAttributeValue = attributes.get(name);
public boolean evaluate(Map<String, ?> attributes) {
Object userAttributeValue = attributes.get(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm should be careful with this. Eventually when we do accept objects and arrays to be passed in, the equals evaluation below will likely perform a reference check as well. That said, I think it's fine for now, but just leave a message saying that we should be extra considerate when we decide to allow objects and arrays.


if (value != null) { // if there is a value in the condition
// check user attribute value is equal
Expand Down
8 changes: 4 additions & 4 deletions core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public LogEvent createImpressionEvent(@Nonnull ProjectConfig projectConfig,
@Nonnull Experiment activatedExperiment,
@Nonnull Variation variation,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {

Decision decision = new Decision.Builder()
.setCampaignId(activatedExperiment.getLayerId())
Expand Down Expand Up @@ -108,7 +108,7 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
@Nonnull String userId,
@Nonnull String eventId, // Why is this not used?
@Nonnull String eventName,
@Nonnull Map<String, String> attributes,
@Nonnull Map<String, ?> attributes,
@Nonnull Map<String, ?> eventTags) {

if (experimentVariationMap.isEmpty()) {
Expand Down Expand Up @@ -164,10 +164,10 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
return new LogEvent(LogEvent.RequestMethod.POST, EVENT_ENDPOINT, Collections.<String, String>emptyMap(), eventBatch);
}

private List<Attribute> buildAttributeList(ProjectConfig projectConfig, Map<String, String> attributes) {
private List<Attribute> buildAttributeList(ProjectConfig projectConfig, Map<String, ?> attributes) {
List<Attribute> attributesList = new ArrayList<Attribute>();

for (Map.Entry<String, String> entry : attributes.entrySet()) {
for (Map.Entry<String, ?> entry : attributes.entrySet()) {
String attributeId = projectConfig.getAttributeId(projectConfig, entry.getKey());
if(attributeId == null) {
continue;
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public static boolean isExperimentActive(@Nonnull Experiment experiment) {
*/
public static boolean isUserInExperiment(@Nonnull ProjectConfig projectConfig,
@Nonnull Experiment experiment,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
List<String> experimentAudienceIds = experiment.getAudienceIds();

// if there are no audiences, ALL users should be part of the experiment
Expand Down