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 all 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
49 changes: 25 additions & 24 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 @@ -228,13 +228,13 @@ public void track(@Nonnull String eventName,

public void track(@Nonnull String eventName,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) throws UnknownEventTypeException {
@Nonnull Map<String, ?> attributes) throws UnknownEventTypeException {
track(eventName, userId, attributes, Collections.<String, String>emptyMap());
}

public void track(@Nonnull String eventName,
@Nonnull String userId,
@Nonnull Map<String, String> attributes,
@Nonnull Map<String, ?> attributes,
@Nonnull Map<String, ?> eventTags) throws UnknownEventTypeException {

if (!validateUserId(userId)) {
Expand All @@ -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 @@ -344,7 +344,7 @@ public void track(@Nonnull String eventName,
*/
public @Nonnull Boolean isFeatureEnabled(@Nonnull String featureKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
if (featureKey == null) {
logger.warn("The featureKey parameter must be nonnull.");
return false;
Expand All @@ -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 @@ -410,7 +410,7 @@ else if (userId == null) {
public @Nullable Boolean getFeatureVariableBoolean(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -450,7 +450,7 @@ else if (userId == null) {
public @Nullable Double getFeatureVariableDouble(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -495,7 +495,7 @@ else if (userId == null) {
public @Nullable Integer getFeatureVariableInteger(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -540,7 +540,7 @@ else if (userId == null) {
public @Nullable String getFeatureVariableString(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
return getFeatureVariableValueForType(
featureKey,
variableKey,
Expand All @@ -553,7 +553,7 @@ else if (userId == null) {
String getFeatureVariableValueForType(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes,
@Nonnull Map<String, ?> attributes,
@Nonnull LiveVariable.VariableType variableType) {
if (featureKey == null) {
logger.warn("The featureKey parameter must be nonnull.");
Expand Down Expand Up @@ -614,7 +614,7 @@ else if (userId == null) {
* @return List of the feature keys that are enabled for the user if the userId is empty it will
* return Empty List.
*/
public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<String, String> attributes) {
public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<String, ?> attributes) {
List<String> enabledFeaturesList = new ArrayList<String>();

if (!validateUserId(userId)){
Expand Down Expand Up @@ -642,9 +642,9 @@ Variation getVariation(@Nonnull Experiment experiment,
public @Nullable
Variation getVariation(@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) throws UnknownExperimentException {
@Nonnull Map<String, ?> attributes) throws UnknownExperimentException {

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

return decisionService.getVariation(experiment, userId, filteredAttributes);
}
Expand All @@ -659,7 +659,7 @@ Variation getVariation(@Nonnull String experimentKey,
public @Nullable
Variation getVariation(@Nonnull String experimentKey,
@Nonnull String userId,
@Nonnull Map<String, String> attributes) {
@Nonnull Map<String, ?> attributes) {
if (!validateUserId(userId)) {
return null;
}
Expand All @@ -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<>(attributes);
for (String unknownAttribute : unknownAttributes) {
attributes.remove(unknownAttribute);
}
Expand Down
38 changes: 27 additions & 11 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 @@ -129,10 +129,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());
}
String bucketingId = getBucketingId(userId, filteredAttributes);
variation = bucketer.bucket(experiment, bucketingId);

if (variation != null) {
Expand All @@ -159,7 +156,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 +192,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 @@ -210,10 +207,7 @@ public DecisionService(@Nonnull Bucketer bucketer,

// for all rules before the everyone else rule
int rolloutRulesLength = rollout.getExperiments().size();
String bucketingId = userId;
if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
}
String bucketingId = getBucketingId(userId, filteredAttributes);
Variation variation;
for (int i = 0; i < rolloutRulesLength - 1; i++) {
Experiment rolloutRule = rollout.getExperiments().get(i);
Expand Down Expand Up @@ -343,4 +337,26 @@ void saveVariation(@Nonnull Experiment experiment,
}
}
}

/**
* Get the bucketingId of a user if a bucketingId exists in attributes, or else default to userId.
* @param userId The userId of the user.
* @param filteredAttributes The user's attributes. This should be filtered to just attributes in the Datafile.
* @return bucketingId if it is a String type in attributes.
* else return userId
*/
String getBucketingId(@Nonnull String userId,
@Nonnull Map<String, ?> filteredAttributes) {
String bucketingId = userId;
if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
if (String.class.isInstance(filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString()))) {
bucketingId = (String) filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
logger.debug("BucketingId is valid: \"{}\"", bucketingId);
}
else {
logger.warn("BucketingID attribute is not a string. Defaulted to userId");
}
}
return bucketingId;
}
}
4 changes: 2 additions & 2 deletions 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
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, Optimizely and contributors
* Copyright 2016-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.
Expand Down 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
4 changes: 2 additions & 2 deletions 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
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, Optimizely and contributors
* Copyright 2016-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.
Expand All @@ -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.

}
4 changes: 2 additions & 2 deletions 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
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, Optimizely and contributors
* Copyright 2016-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.
Expand Down 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
4 changes: 2 additions & 2 deletions 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
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, Optimizely and contributors
* Copyright 2016-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.
Expand Down 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
15 changes: 8 additions & 7 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
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, Optimizely and contributors
* Copyright 2016-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.
Expand Down 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,13 @@ 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) {
// 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
Expand All @@ -69,7 +70,7 @@ else if (userAttributeValue != null) { // if the datafile value is null but user
public String toString() {
return "{name='" + name + "\'" +
", type='" + type + "\'" +
", value='" + value + "\'" +
", value='" + value.toString() + "\'" +
"}";
}

Expand Down
Loading