From d476539bfce64aff5394a02c7cf436901cca88f2 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 7 Oct 2020 09:29:30 -0700 Subject: [PATCH 01/17] add decide api --- .../java/com/optimizely/ab/Optimizely.java | 12 +- .../ab/notification/DecisionNotification.java | 98 ++++++++ .../ab/notification/NotificationCenter.java | 3 +- .../DecisionReasons.java | 33 +++ .../OptimizelyDecision.java | 10 +- .../OptimizelyUserContext.java | 227 +++++++++++++++++- 6 files changed, 367 insertions(+), 16 deletions(-) create mode 100644 core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 1ebc6d420..1f874b7ac 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -78,8 +78,7 @@ public class Optimizely implements AutoCloseable { private static final Logger logger = LoggerFactory.getLogger(Optimizely.class); - @VisibleForTesting - final DecisionService decisionService; + public final DecisionService decisionService; @VisibleForTesting @Deprecated final EventHandler eventHandler; @@ -87,8 +86,8 @@ public class Optimizely implements AutoCloseable { final EventProcessor eventProcessor; @VisibleForTesting final ErrorHandler errorHandler; - @VisibleForTesting - final OptimizelyDecideOption[] defaultDecideOptions; + + public final OptimizelyDecideOption[] defaultDecideOptions; private final ProjectConfigManager projectConfigManager; @@ -227,7 +226,7 @@ private Variation activate(@Nullable ProjectConfig projectConfig, return variation; } - private void sendImpression(@Nonnull ProjectConfig projectConfig, + public void sendImpression(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map filteredAttributes, @@ -742,8 +741,7 @@ T getFeatureVariableValueForType(@Nonnull String featureKey, } // Helper method which takes type and variable value and convert it to object to use in Listener DecisionInfo object variable value - @VisibleForTesting - Object convertStringToType(String variableValue, String type) { + public Object convertStringToType(String variableValue, String type) { if (variableValue != null) { switch (type) { case FeatureVariable.DOUBLE_TYPE: diff --git a/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java b/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java index 0a7ea6e3c..ad95a8ded 100644 --- a/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java +++ b/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java @@ -24,6 +24,7 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.HashMap; +import java.util.List; import java.util.Map; /** @@ -350,4 +351,101 @@ public DecisionNotification build() { decisionInfo); } } + + public static FlagDecisionNotificationBuilder newFlagDecisionNotificationBuilder() { + return new FlagDecisionNotificationBuilder(); + } + + public static class FlagDecisionNotificationBuilder { + public final static String FLAG_KEY = "flagKey"; + public final static String ENABLED = "enabled"; + public final static String VARIABLES = "variables"; + public final static String VARIATION_KEY = "variationKey"; + public final static String RULE_KEY = "ruleKey"; + public final static String REASONS = "reasons"; + public final static String DECISION_EVENT_DISPATCHED = "decisionEventDispatched"; + + private String flagKey; + private Boolean enabled; + private Object variables; + private String userId; + private Map attributes; + private String variationKey; + private String ruleKey; + private List reasons; + private Boolean decisionEventDispatched; + + private Map decisionInfo; + + public FlagDecisionNotificationBuilder withUserId(String userId) { + this.userId = userId; + return this; + } + + public FlagDecisionNotificationBuilder withAttributes(Map attributes) { + this.attributes = attributes; + return this; + } + + public FlagDecisionNotificationBuilder withFlagKey(String flagKey) { + this.flagKey = flagKey; + return this; + } + + public FlagDecisionNotificationBuilder withEnabled(Boolean enabled) { + this.enabled = enabled; + return this; + } + + public FlagDecisionNotificationBuilder withVariables(Object variables) { + this.enabled = enabled; + return this; + } + + public FlagDecisionNotificationBuilder withVariationKey(String key) { + this.variationKey = key; + return this; + } + + public FlagDecisionNotificationBuilder withRuleKey(String key) { + this.ruleKey = key; + return this; + } + + public FlagDecisionNotificationBuilder withReasons(List reasons) { + this.reasons = reasons; + return this; + } + + public FlagDecisionNotificationBuilder withDecisionEventDispatched(Boolean dispatched) { + this.decisionEventDispatched = dispatched; + return this; + } + + public DecisionNotification build() { + if (flagKey == null) { + throw new OptimizelyRuntimeException("flagKey not set"); + } + + if (enabled == null) { + throw new OptimizelyRuntimeException("enabled not set"); + } + + decisionInfo = new HashMap<>(); + decisionInfo.put(FLAG_KEY, flagKey); + decisionInfo.put(ENABLED, enabled); + decisionInfo.put(VARIABLES, variables); + decisionInfo.put(VARIATION_KEY, variationKey); + decisionInfo.put(RULE_KEY, ruleKey); + decisionInfo.put(REASONS, reasons); + decisionInfo.put(DECISION_EVENT_DISPATCHED, decisionEventDispatched); + + return new DecisionNotification( + NotificationCenter.DecisionNotificationType.FLAG.toString(), + userId, + attributes, + decisionInfo); + } + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java b/core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java index 4b0b3e406..ff13c8d09 100644 --- a/core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java +++ b/core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java @@ -55,7 +55,8 @@ public enum DecisionNotificationType { FEATURE("feature"), FEATURE_TEST("feature-test"), FEATURE_VARIABLE("feature-variable"), - ALL_FEATURE_VARIABLES("all-feature-variables"); + ALL_FEATURE_VARIABLES("all-feature-variables"), + FLAG("flag"); private final String key; diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java new file mode 100644 index 000000000..6cb8c6acb --- /dev/null +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java @@ -0,0 +1,33 @@ +package com.optimizely.ab.optimizelyusercontext; + +import java.util.ArrayList; +import java.util.List; + +public class DecisionReasons { + + List errors; + List logs; + + public DecisionReasons() { + this.errors = new ArrayList(); + this.logs = new ArrayList(); + } + + public void addError(String message) { + errors.add(message); + } + + public void addInfo(String message) { + logs.add(message); + } + + + public List toReport(List options) { + List reasons = new ArrayList<>(errors); + if(options.contains(OptimizelyDecideOption.INCLUDE_REASONS)) { + reasons.addAll(logs); + } + return reasons; + } + +} diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java index a9a0f6c17..e4a8b9a8b 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java @@ -4,6 +4,8 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.util.Arrays; +import java.util.List; public class OptimizelyDecision { @Nullable @@ -24,7 +26,7 @@ public class OptimizelyDecision { private final OptimizelyUserContext userContext; @Nonnull - private String[] reasons; + private List reasons; public OptimizelyDecision(@Nullable String variationKey, @@ -33,7 +35,7 @@ public OptimizelyDecision(@Nullable String variationKey, @Nullable String ruleKey, @Nonnull String flagKey, @Nullable OptimizelyUserContext userContext, - @Nonnull String[] reasons) { + @Nonnull List reasons) { this.variationKey = variationKey; this.enabled = enabled; this.variables = variables; @@ -73,7 +75,7 @@ public OptimizelyUserContext getUserContext() { } @Nonnull - public String[] getReasons() { + public List getReasons() { return reasons; } @@ -86,7 +88,7 @@ public static OptimizelyDecision createErrorDecision(@Nonnull String key, null, key, user, - new String[]{error}); + Arrays.asList(error)); } public boolean hasFailed() { diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index 8b62654f4..c17184f10 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -2,17 +2,37 @@ import com.optimizely.ab.Optimizely; import com.optimizely.ab.UnknownEventTypeException; +import com.optimizely.ab.bucketing.FeatureDecision; +import com.optimizely.ab.config.*; +import com.optimizely.ab.notification.DecisionNotification; +import com.optimizely.ab.notification.FeatureTestSourceInfo; +import com.optimizely.ab.notification.RolloutSourceInfo; +import com.optimizely.ab.notification.SourceInfo; +import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; +import java.util.*; public class OptimizelyUserContext { + @Nonnull private final String userId; + + @Nonnull private final Map attributes; + + @Nonnull private final Optimizely optimizely; + private static final Logger logger = LoggerFactory.getLogger(OptimizelyUserContext.class); + + public final static String SDK_NOT_READY = "Optimizely SDK not configured properly yet"; + public final static String FLAG_KEY_INVALID = "Flag key \"%s\" is not in datafile."; + public final static String VARIABLE_VALUE_INVALID = "Variable value for key \"%s\" is invalid or wrong type."; + public final static String OPTIMIZELY_JSON_ERROR = "Invalid variables for OptimizelyJSON."; + + public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull String userId, @Nonnull Map attributes) { @@ -37,44 +57,243 @@ public Optimizely getOptimizely() { return optimizely; } + /** + * Set an attribute for a given key. + * + * @param key An attribute key + * @param value An attribute value + */ public void setAttribute(@Nonnull String key, @Nonnull Object value) { attributes.put(key, value); } + /** + * Returns a decision result ({@link OptimizelyDecision}) for a given flag key and a user context, which contains all data required to deliver the flag. + *
    + *
  • If the SDK finds an error, it’ll return a decision with null for variationKey. The decision will include an error message in reasons. + *
+ * @param key A flag key for which a decision will be made. + * @param options An array of options for decision-making. + * @return A decision result. + */ public OptimizelyDecision decide(@Nonnull String key, @Nonnull OptimizelyDecideOption[] options) { - return OptimizelyDecision.createErrorDecision(key, this, "N/A"); + + ProjectConfig projectConfig = optimizely.getProjectConfig(); + if (projectConfig == null) { + return OptimizelyDecision.createErrorDecision(key, this, SDK_NOT_READY); + } + + FeatureFlag flag = projectConfig.getFeatureKeyMapping().get(key); + if (flag == null) { + return OptimizelyDecision.createErrorDecision(key, this, getFlagKeyInvalidMessage(key)); + } + + List allOptions = getAllOptions(options); + DecisionReasons decisionReasons = new DecisionReasons(); + Boolean sentEvent = false; + Boolean flagEnabled = false; + + Map copiedAttributes = copyAttributes(); + FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( + flag, + userId, + copiedAttributes, + projectConfig); + + FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT; + SourceInfo sourceInfo = new RolloutSourceInfo(); + + if (flagDecision.variation != null) { + if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { + if (!allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) { + optimizely.sendImpression( + projectConfig, + flagDecision.experiment, + userId, + copiedAttributes, + flagDecision.variation); + sentEvent = true; + } + + decisionSource = flagDecision.decisionSource; + sourceInfo = new FeatureTestSourceInfo(flagDecision.experiment.getKey(), flagDecision.variation.getKey()); + } else { + String message = String.format("The user \"%s\" is not included in an experiment for flag \"%s\".", userId, key); + logger.info(message); + decisionReasons.addInfo(message); + } + if (flagDecision.variation.getFeatureEnabled()) { + flagEnabled = true; + } + } + + Map variableMap = new HashMap<>(); + if (!allOptions.contains(OptimizelyDecideOption.EXCLUDE_VARIABLES)) { + variableMap = getDecisionVariableMap( + flag, + flagDecision.variation, + flagEnabled, + decisionReasons); + } + + OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); + if (optimizelyJSON == null) { + decisionReasons.addError(OPTIMIZELY_JSON_ERROR); + } + + List reasonsToReport = decisionReasons.toReport(allOptions); + String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; + // TODO: add ruleKey values when available later. + String ruleKey = null; + + DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder() + .withUserId(userId) + .withAttributes(copiedAttributes) + .withFlagKey(key) + .withEnabled(flagEnabled) + .withVariables(variableMap) + .withVariationKey(variationKey) + .withRuleKey(ruleKey) + .withReasons(reasonsToReport) + .withDecisionEventDispatched(sentEvent) + .build(); + optimizely.notificationCenter.send(decisionNotification); + + logger.info("Feature \"{}\" is enabled for user \"{}\"? {}", key, userId, flagEnabled); + + return new OptimizelyDecision( + variationKey, + flagEnabled, + optimizelyJSON, + ruleKey, + key, + this, + reasonsToReport); } + /** + * Returns a decision result ({@link OptimizelyDecision}) for a given flag key and a user context, which contains all data required to deliver the flag. + * + * @param key A flag key for which a decision will be made. + * @return A decision result. + */ public OptimizelyDecision decide(String key) { return decide(key, new OptimizelyDecideOption[0]); } + /** + * Returns a key-map of decision results ({@link OptimizelyDecision}) for multiple flag keys and a user context. + *
    + *
  • If the SDK finds an error for a key, the response will include a decision for the key showing reasons for the error. + *
  • The SDK will always return key-mapped decisions. When it can not process requests, it’ll return an empty map after logging the errors. + *
      + * @param keys An array of flag keys for which decisions will be made. + * @param options An array of options for decision-making. + * @return All decision results mapped by flag keys. + */ public Map decideAll(@Nonnull String[] keys, @Nonnull OptimizelyDecideOption[] options) { return new HashMap<>(); } + /** + * Returns a key-map of decision results for multiple flag keys and a user context. + * + * @param keys An array of flag keys for which decisions will be made. + * @return All decision results mapped by flag keys. + */ public Map decideAll(@Nonnull String[] keys) { return decideAll(keys, new OptimizelyDecideOption[0]); } + /** + * Returns a key-map of decision results ({@link OptimizelyDecision}) for all active flag keys. + * + * @param options An array of options for decision-making. + * @return All decision results mapped by flag keys. + */ public Map decideAll(@Nonnull OptimizelyDecideOption[] options) { String[] allFlagKeys = {}; return decideAll(allFlagKeys, options); } + /** + * Returns a key-map of decision results ({@link OptimizelyDecision}) for all active flag keys. + * + * @return A dictionary of all decision results, mapped by flag keys. + */ public Map decideAll() { return decideAll(new OptimizelyDecideOption[0]); } + /** + * Track an event. + * + * @param eventName The event name. + * @param eventTags A map of event tag names to event tag values. + * @throws UnknownEventTypeException + */ public void trackEvent(@Nonnull String eventName, @Nonnull Map eventTags) throws UnknownEventTypeException { optimizely.track(eventName, userId, attributes, eventTags); } + /** + * Track an event. + * + * @param eventName The event name. + * @throws UnknownEventTypeException + */ public void trackEvent(@Nonnull String eventName) throws UnknownEventTypeException { trackEvent(eventName, Collections.emptyMap()); } + // Utils + + private Map copyAttributes() { + return new HashMap<>(attributes); + } + + private List getAllOptions(OptimizelyDecideOption[] options) { + List allOptions = new ArrayList(Arrays.asList(optimizely.defaultDecideOptions)); + allOptions.addAll(Arrays.asList(options)); + return allOptions; + } + + public static String getFlagKeyInvalidMessage(String flagKey) { + return String.format(FLAG_KEY_INVALID, flagKey); + } + + public static String getVariableValueInvalidMessage(String variableKey) { + return String.format(VARIABLE_VALUE_INVALID, variableKey); + } + + private Map getDecisionVariableMap(FeatureFlag flag, + Variation variation, + Boolean featureEnabled, + DecisionReasons decisionReasons) { + Map valuesMap = new HashMap(); + for (FeatureVariable variable : flag.getVariables()) { + String value = variable.getDefaultValue(); + if (featureEnabled) { + FeatureVariableUsageInstance instance = variation.getVariableIdToFeatureVariableUsageInstanceMap().get(variable.getId()); + if (instance != null) { + value = instance.getValue(); + } + } + + Object convertedValue = optimizely.convertStringToType(value, variable.getType()); + if (convertedValue == null) { + decisionReasons.addError(getVariableValueInvalidMessage(variable.getKey())); + } else if (convertedValue instanceof OptimizelyJSON) { + convertedValue = ((OptimizelyJSON) convertedValue).toMap(); + } + + valuesMap.put(variable.getKey(), convertedValue); + } + + return valuesMap; + } + } From 8bea7388ae2a5824161eb9cb37e7105b9220f06a Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 7 Oct 2020 15:14:26 -0700 Subject: [PATCH 02/17] add options and reasons --- .../com/optimizely/ab/bucketing/Bucketer.java | 45 +++- .../ab/bucketing/DecisionService.java | 217 +++++++++++++----- .../ab/internal/ExperimentUtils.java | 72 +++++- .../OptimizelyUserContext.java | 4 +- 4 files changed, 266 insertions(+), 72 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java index c9295c6bb..f423472a7 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java @@ -23,6 +23,8 @@ import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.TrafficAllocation; import com.optimizely.ab.config.Variation; +import com.optimizely.ab.optimizelyusercontext.DecisionReasons; +import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -71,7 +73,9 @@ private String bucketToEntity(int bucketValue, List trafficAl private Experiment bucketToExperiment(@Nonnull Group group, @Nonnull String bucketingId, - @Nonnull ProjectConfig projectConfig) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { // "salt" the bucket id using the group id String bucketKey = bucketingId + group.getId(); @@ -91,7 +95,9 @@ private Experiment bucketToExperiment(@Nonnull Group group, } private Variation bucketToVariation(@Nonnull Experiment experiment, - @Nonnull String bucketingId) { + @Nonnull String bucketingId, + @Nullable List options, + @Nullable DecisionReasons reasons) { // "salt" the bucket id using the experiment id String experimentId = experiment.getId(); String experimentKey = experiment.getKey(); @@ -107,14 +113,14 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, if (bucketedVariationId != null) { Variation bucketedVariation = experiment.getVariationIdToVariationMap().get(bucketedVariationId); String variationKey = bucketedVariation.getKey(); - logger.info("User with bucketingId \"{}\" is in variation \"{}\" of experiment \"{}\".", bucketingId, variationKey, + DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is in variation \"%s\" of experiment \"%s\".", bucketingId, variationKey, experimentKey); return bucketedVariation; } // user was not bucketed to a variation - logger.info("User with bucketingId \"{}\" is not in any variation of experiment \"{}\".", bucketingId, experimentKey); + DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", bucketingId, experimentKey); return null; } @@ -123,12 +129,17 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, * * @param experiment The Experiment in which the user is to be bucketed. * @param bucketingId string A customer-assigned value used to create the key for the murmur hash. + * @param projectConfig The current projectConfig + * @param options An array of decision options + * @param reasons Decision log messages * @return Variation the user is bucketed into or null. */ @Nullable public Variation bucket(@Nonnull Experiment experiment, @Nonnull String bucketingId, - @Nonnull ProjectConfig projectConfig) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { // ---------- Bucket User ---------- String groupId = experiment.getGroupId(); // check whether the experiment belongs to a group @@ -136,9 +147,9 @@ public Variation bucket(@Nonnull Experiment experiment, Group experimentGroup = projectConfig.getGroupIdMapping().get(groupId); // bucket to an experiment only if group entities are to be mutually exclusive if (experimentGroup.getPolicy().equals(Group.RANDOM_POLICY)) { - Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig); + Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig, options, reasons); if (bucketedExperiment == null) { - logger.info("User with bucketingId \"{}\" is not in any experiment of group {}.", bucketingId, experimentGroup.getId()); + DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId()); return null; } else { @@ -146,19 +157,33 @@ public Variation bucket(@Nonnull Experiment experiment, // if the experiment a user is bucketed in within a group isn't the same as the experiment provided, // don't perform further bucketing within the experiment if (!bucketedExperiment.getId().equals(experiment.getId())) { - logger.info("User with bucketingId \"{}\" is not in experiment \"{}\" of group {}.", bucketingId, experiment.getKey(), + DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), experimentGroup.getId()); return null; } - logger.info("User with bucketingId \"{}\" is in experiment \"{}\" of group {}.", bucketingId, experiment.getKey(), + DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), experimentGroup.getId()); } } - return bucketToVariation(experiment, bucketingId); + return bucketToVariation(experiment, bucketingId, options, reasons); } + /** + * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3. + * + * @param experiment The Experiment in which the user is to be bucketed. + * @param bucketingId string A customer-assigned value used to create the key for the murmur hash. + * @param projectConfig The current projectConfig + * @return Variation the user is bucketed into or null. + */ + @Nullable + public Variation bucket(@Nonnull Experiment experiment, + @Nonnull String bucketingId, + @Nonnull ProjectConfig projectConfig) { + return bucket(experiment, bucketingId, projectConfig, null, null); + } //======== Helper methods ========// diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index 13091472d..d1cfd3b77 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -18,19 +18,20 @@ import com.optimizely.ab.OptimizelyRuntimeException; import com.optimizely.ab.config.*; import com.optimizely.ab.error.ErrorHandler; -import com.optimizely.ab.internal.ExperimentUtils; import com.optimizely.ab.internal.ControlAttribute; - +import com.optimizely.ab.internal.ExperimentUtils; +import com.optimizely.ab.optimizelyusercontext.DecisionReasons; +import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; - import static com.optimizely.ab.internal.LoggingConstants.LoggingEntityType.EXPERIMENT; import static com.optimizely.ab.internal.LoggingConstants.LoggingEntityType.RULE; @@ -81,24 +82,28 @@ public DecisionService(@Nonnull Bucketer bucketer, * @param experiment The Experiment the user will be bucketed into. * @param userId The userId of the user. * @param filteredAttributes The user's attributes. This should be filtered to just attributes in the Datafile. + * @param projectConfig The current projectConfig + * @param options An array of decision options + * @param reasons Decision log messages * @return The {@link Variation} the user is allocated into. */ @Nullable public Variation getVariation(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { - - if (!ExperimentUtils.isExperimentActive(experiment)) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { + if (!ExperimentUtils.isExperimentActive(experiment, options, reasons)) { return null; } // look for forced bucketing first. - Variation variation = getForcedVariation(experiment, userId); + Variation variation = getForcedVariation(experiment, userId, options, reasons); // check for whitelisting if (variation == null) { - variation = getWhitelistedVariation(experiment, userId); + variation = getWhitelistedVariation(experiment, userId, options, reasons); } if (variation != null) { @@ -112,21 +117,21 @@ public Variation getVariation(@Nonnull Experiment experiment, try { Map userProfileMap = userProfileService.lookup(userId); if (userProfileMap == null) { - logger.info("We were unable to get a user profile map from the UserProfileService."); + logInfo(logger, reasons, "We were unable to get a user profile map from the UserProfileService."); } else if (UserProfileUtils.isValidUserProfileMap(userProfileMap)) { userProfile = UserProfileUtils.convertMapToUserProfile(userProfileMap); } else { - logger.warn("The UserProfileService returned an invalid map."); + logWarn(logger, reasons, "The UserProfileService returned an invalid map."); } } catch (Exception exception) { - logger.error(exception.getMessage()); + logError(logger, reasons, exception.getMessage()); errorHandler.handleError(new OptimizelyRuntimeException(exception)); } } // check if user exists in user profile if (userProfile != null) { - variation = getStoredVariation(experiment, userProfile, projectConfig); + variation = getStoredVariation(experiment, userProfile, projectConfig, options, reasons); // return the stored variation if it exists if (variation != null) { return variation; @@ -135,13 +140,13 @@ public Variation getVariation(@Nonnull Experiment experiment, userProfile = new UserProfile(userId, new HashMap()); } - if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, experiment, filteredAttributes, EXPERIMENT, experiment.getKey())) { - String bucketingId = getBucketingId(userId, filteredAttributes); - variation = bucketer.bucket(experiment, bucketingId, projectConfig); + if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, experiment, filteredAttributes, EXPERIMENT, experiment.getKey(), options, reasons)) { + String bucketingId = getBucketingId(userId, filteredAttributes, options, reasons); + variation = bucketer.bucket(experiment, bucketingId, projectConfig, options, reasons); if (variation != null) { if (userProfileService != null) { - saveVariation(experiment, variation, userProfile); + saveVariation(experiment, variation, userProfile, options, reasons); } else { logger.debug("This decision will not be saved since the UserProfileService is null."); } @@ -150,46 +155,87 @@ public Variation getVariation(@Nonnull Experiment experiment, return variation; } - logger.info("User \"{}\" does not meet conditions to be in experiment \"{}\".", userId, experiment.getKey()); + logInfo(logger, reasons, "User \"%s\" does not meet conditions to be in experiment \"%s\".", userId, experiment.getKey()); return null; } + /** + * Get a {@link Variation} of an {@link Experiment} for a user to be allocated into. + * + * @param experiment The Experiment the user will be bucketed into. + * @param userId The userId of the user. + * @param filteredAttributes The user's attributes. This should be filtered to just attributes in the Datafile. + * @param projectConfig The current projectConfig + * @return The {@link Variation} the user is allocated into. + */ + @Nullable + public Variation getVariation(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + return getVariation(experiment, userId, filteredAttributes, projectConfig, null, null); + } + /** * Get the variation the user is bucketed into for the FeatureFlag * * @param featureFlag The feature flag the user wants to access. * @param userId User Identifier * @param filteredAttributes A map of filtered attributes. + * @param projectConfig The current projectConfig + * @param options An array of decision options + * @param reasons Decision log messages * @return {@link FeatureDecision} */ @Nonnull public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, @Nonnull String userId, @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { if (!featureFlag.getExperimentIds().isEmpty()) { for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); - Variation variation = getVariation(experiment, userId, filteredAttributes, projectConfig); + Variation variation = getVariation(experiment, userId, filteredAttributes, projectConfig, options, reasons); if (variation != null) { return new FeatureDecision(experiment, variation, FeatureDecision.DecisionSource.FEATURE_TEST); } } } else { - logger.info("The feature flag \"{}\" is not used in any experiments.", featureFlag.getKey()); + logInfo(logger, reasons, "The feature flag \"%s\" is not used in any experiments.", featureFlag.getKey()); } - FeatureDecision featureDecision = getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig); + FeatureDecision featureDecision = getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig, options, reasons); if (featureDecision.variation == null) { - logger.info("The user \"{}\" was not bucketed into a rollout for feature flag \"{}\".", + logInfo(logger, reasons, "The user \"%s\" was not bucketed into a rollout for feature flag \"%s\".", userId, featureFlag.getKey()); } else { - logger.info("The user \"{}\" was bucketed into a rollout for feature flag \"{}\".", + logInfo(logger, reasons, "The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, featureFlag.getKey()); } return featureDecision; } + /** + * Get the variation the user is bucketed into for the FeatureFlag + * + * @param featureFlag The feature flag the user wants to access. + * @param userId User Identifier + * @param filteredAttributes A map of filtered attributes. + * @param projectConfig The current projectConfig + * @return {@link FeatureDecision} + */ + @Nonnull + public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + + return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig,null, null); + } + + /** * Try to bucket the user into a rollout rule. * Evaluate the user for rules in priority order by seeing if the user satisfies the audience. @@ -198,49 +244,54 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, * @param featureFlag The feature flag the user wants to access. * @param userId User Identifier * @param filteredAttributes A map of filtered attributes. + * @param projectConfig The current projectConfig + * @param options An array of decision options + * @param reasons Decision log messages * @return {@link FeatureDecision} */ @Nonnull FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag, @Nonnull String userId, @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { // use rollout to get variation for feature if (featureFlag.getRolloutId().isEmpty()) { - logger.info("The feature flag \"{}\" is not used in a rollout.", featureFlag.getKey()); + logInfo(logger, reasons, "The feature flag \"%s\" is not used in a rollout.", featureFlag.getKey()); return new FeatureDecision(null, null, null); } Rollout rollout = projectConfig.getRolloutIdMapping().get(featureFlag.getRolloutId()); if (rollout == null) { - logger.error("The rollout with id \"{}\" was not found in the datafile for feature flag \"{}\".", + logError(logger, reasons, "The rollout with id \"%s\" was not found in the datafile for feature flag \"%s\".", featureFlag.getRolloutId(), featureFlag.getKey()); return new FeatureDecision(null, null, null); } // for all rules before the everyone else rule int rolloutRulesLength = rollout.getExperiments().size(); - String bucketingId = getBucketingId(userId, filteredAttributes); + String bucketingId = getBucketingId(userId, filteredAttributes, options, reasons); Variation variation; for (int i = 0; i < rolloutRulesLength - 1; i++) { Experiment rolloutRule = rollout.getExperiments().get(i); - if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, rolloutRule, filteredAttributes, RULE, Integer.toString(i + 1))) { - variation = bucketer.bucket(rolloutRule, bucketingId, projectConfig); + if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, rolloutRule, filteredAttributes, RULE, Integer.toString(i + 1), options, reasons)) { + variation = bucketer.bucket(rolloutRule, bucketingId, projectConfig, options, reasons); if (variation == null) { break; } return new FeatureDecision(rolloutRule, variation, FeatureDecision.DecisionSource.ROLLOUT); } else { - logger.debug("User \"{}\" does not meet conditions for targeting rule \"{}\".", userId, i + 1); + logDebug(logger, reasons, "User \"%s\" does not meet conditions for targeting rule \"%d\".", userId, i + 1); } } // get last rule which is the fall back rule Experiment finalRule = rollout.getExperiments().get(rolloutRulesLength - 1); - if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, finalRule, filteredAttributes, RULE, "Everyone Else")) { - variation = bucketer.bucket(finalRule, bucketingId, projectConfig); + if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, finalRule, filteredAttributes, RULE, "Everyone Else", options, reasons)) { + variation = bucketer.bucket(finalRule, bucketingId, projectConfig, options, reasons); if (variation != null) { - logger.debug("User \"{}\" meets conditions for targeting rule \"Everyone Else\".", userId); + logDebug(logger, reasons, "User \"%s\" meets conditions for targeting rule \"Everyone Else\".", userId); return new FeatureDecision(finalRule, variation, FeatureDecision.DecisionSource.ROLLOUT); } @@ -253,20 +304,25 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag * * @param experiment {@link Experiment} in which user is to be bucketed. * @param userId User Identifier + * @param options An array of decision options + * @param reasons Decision log messages * @return null if the user is not whitelisted into any variation * {@link Variation} the user is bucketed into if the user has a specified whitelisted variation. */ @Nullable - Variation getWhitelistedVariation(@Nonnull Experiment experiment, @Nonnull String userId) { + Variation getWhitelistedVariation(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nullable List options, + @Nullable DecisionReasons reasons) { // if a user has a forced variation mapping, return the respective variation Map userIdToVariationKeyMap = experiment.getUserIdToVariationKeyMap(); if (userIdToVariationKeyMap.containsKey(userId)) { String forcedVariationKey = userIdToVariationKeyMap.get(userId); Variation forcedVariation = experiment.getVariationKeyToVariationMap().get(forcedVariationKey); if (forcedVariation != null) { - logger.info("User \"{}\" is forced in variation \"{}\".", userId, forcedVariationKey); + logInfo(logger, reasons, "User \"%s\" is forced in variation \"%s\".", userId, forcedVariationKey); } else { - logger.error("Variation \"{}\" is not in the datafile. Not activating user \"{}\".", + logError(logger, reasons, "Variation \"%s\" is not in the datafile. Not activating user \"%s\".", forcedVariationKey, userId); } return forcedVariation; @@ -279,13 +335,21 @@ Variation getWhitelistedVariation(@Nonnull Experiment experiment, @Nonnull Strin * * @param experiment {@link Experiment} in which the user was bucketed. * @param userProfile {@link UserProfile} of the user. + * @param projectConfig The current projectConfig + * @param options An array of decision options + * @param reasons Decision log messages * @return null if the {@link UserProfileService} implementation is null or the user was not previously bucketed. * else return the {@link Variation} the user was previously bucketed into. */ @Nullable Variation getStoredVariation(@Nonnull Experiment experiment, @Nonnull UserProfile userProfile, - @Nonnull ProjectConfig projectConfig) { + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { + + if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return null; + // ---------- Check User Profile for Sticky Bucketing ---------- // If a user profile instance is present then check it for a saved variation String experimentId = experiment.getId(); @@ -299,20 +363,17 @@ Variation getStoredVariation(@Nonnull Experiment experiment, .getVariationIdToVariationMap() .get(variationId); if (savedVariation != null) { - logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" " + - "for user \"{}\" from user profile.", + logInfo(logger, reasons,"Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", savedVariation.getKey(), experimentKey, userProfile.userId); // A variation is stored for this combined bucket id return savedVariation; } else { - logger.info("User \"{}\" was previously bucketed into variation with ID \"{}\" for experiment \"{}\", " + - "but no matching variation was found for that user. We will re-bucket the user.", + logInfo(logger, reasons,"User \"%s\" was previously bucketed into variation with ID \"%s\" for experiment \"%s\", but no matching variation was found for that user. We will re-bucket the user.", userProfile.userId, variationId, experimentKey); return null; } } else { - logger.info("No previously activated variation of experiment \"{}\" " + - "for user \"{}\" found in user profile.", + logInfo(logger, reasons, "No previously activated variation of experiment \"%s\" for user \"%s\" found in user profile.", experimentKey, userProfile.userId); return null; } @@ -324,10 +385,17 @@ Variation getStoredVariation(@Nonnull Experiment experiment, * @param experiment The experiment the user was buck * @param variation The Variation to save. * @param userProfile A {@link UserProfile} instance of the user information. + * @param options An array of decision options + * @param reasons Decision log messages */ void saveVariation(@Nonnull Experiment experiment, @Nonnull Variation variation, - @Nonnull UserProfile userProfile) { + @Nonnull UserProfile userProfile, + @Nullable List options, + @Nullable DecisionReasons reasons) { + + if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return; + // only save if the user has implemented a user profile service if (userProfileService != null) { String experimentId = experiment.getId(); @@ -358,18 +426,22 @@ void saveVariation(@Nonnull Experiment experiment, * * @param userId The userId of the user. * @param filteredAttributes The user's attributes. This should be filtered to just attributes in the Datafile. + * @param options An array of decision options + * @param reasons Decision log messages * @return bucketingId if it is a String type in attributes. * else return userId */ String getBucketingId(@Nonnull String userId, - @Nonnull Map filteredAttributes) { + @Nonnull Map filteredAttributes, + @Nullable List options, + @Nullable DecisionReasons reasons) { String bucketingId = userId; if (filteredAttributes != null && 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"); + logWarn(logger, reasons, "BucketingID attribute is not a string. Defaulted to userId"); } } return bucketingId; @@ -393,8 +465,6 @@ public ConcurrentHashMap> getForcedVar public boolean setForcedVariation(@Nonnull Experiment experiment, @Nonnull String userId, @Nullable String variationKey) { - - Variation variation = null; // keep in mind that you can pass in a variationKey that is null if you want to @@ -455,12 +525,16 @@ public boolean setForcedVariation(@Nonnull Experiment experiment, * * @param experiment The experiment forced. * @param userId The user ID to be used for bucketing. + * @param options An array of decision options + * @param reasons Decision log messages * @return The variation the user was bucketed into. This value can be null if the * forced variation fails. */ @Nullable public Variation getForcedVariation(@Nonnull Experiment experiment, - @Nonnull String userId) { + @Nonnull String userId, + @Nullable List options, + @Nullable DecisionReasons reasons) { // if the user id is invalid, return false. if (!validateUserId(userId)) { @@ -473,7 +547,7 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, if (variationId != null) { Variation variation = experiment.getVariationIdToVariationMap().get(variationId); if (variation != null) { - logger.debug("Variation \"{}\" is mapped to experiment \"{}\" and user \"{}\" in the forced variation map", + logDebug(logger, reasons, "Variation \"%s\" is mapped to experiment \"%s\" and user \"%s\" in the forced variation map", variation.getKey(), experiment.getKey(), userId); return variation; } @@ -484,6 +558,20 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, return null; } + /** + * Gets the forced variation for a given user and experiment. + * + * @param experiment The experiment forced. + * @param userId The user ID to be used for bucketing. + * @return The variation the user was bucketed into. This value can be null if the + * forced variation fails. + */ + @Nullable + public Variation getForcedVariation(@Nonnull Experiment experiment, + @Nonnull String userId) { + return getForcedVariation(experiment, userId, null, null); + } + /** * Helper function to check that the provided userId is valid * @@ -498,4 +586,29 @@ private boolean validateUserId(String userId) { return true; } + + public static void logError(Logger logger, DecisionReasons reasons, String format, Object... args) { + String message = String.format(format, args); + logger.error(message); + if (reasons != null) reasons.addInfo(message); + } + + public static void logWarn(Logger logger, DecisionReasons reasons, String format, Object... args) { + String message = String.format(format, args); + logger.warn(message); + if (reasons != null) reasons.addInfo(message); + } + + public static void logInfo(Logger logger, DecisionReasons reasons, String format, Object... args) { + String message = String.format(format, args); + logger.info(message); + if (reasons != null) reasons.addInfo(message); + } + + public static void logDebug(Logger logger, DecisionReasons reasons, String format, Object... args) { + String message = String.format(format, args); + logger.debug(message); + if (reasons != null) reasons.addInfo(message); + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java index f5109b624..c1d40b8a3 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java @@ -16,11 +16,14 @@ */ package com.optimizely.ab.internal; +import com.optimizely.ab.bucketing.DecisionService; import com.optimizely.ab.config.Experiment; import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.audience.AudienceIdCondition; import com.optimizely.ab.config.audience.Condition; import com.optimizely.ab.config.audience.OrCondition; +import com.optimizely.ab.optimizelyusercontext.DecisionReasons; +import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -41,18 +44,26 @@ private ExperimentUtils() { * Helper method to validate all pre-conditions before bucketing a user. * * @param experiment the experiment we are validating pre-conditions for + * @param options An array of decision options + * @param reasons Decision log messages * @return whether the pre-conditions are satisfied */ - public static boolean isExperimentActive(@Nonnull Experiment experiment) { + public static boolean isExperimentActive(@Nonnull Experiment experiment, + @Nullable List options, + @Nullable DecisionReasons reasons) { if (!experiment.isActive()) { - logger.info("Experiment \"{}\" is not running.", experiment.getKey()); + DecisionService.logInfo(logger, reasons, "Experiment \"%s\" is not running.", experiment.getKey()); return false; } return true; } + public static boolean isExperimentActive(@Nonnull Experiment experiment) { + return isExperimentActive(experiment, null, null); + } + /** * Determines whether a user satisfies audience conditions for the experiment. * @@ -61,13 +72,17 @@ public static boolean isExperimentActive(@Nonnull Experiment experiment) { * @param attributes the attributes of the user * @param loggingEntityType It can be either experiment or rule. * @param loggingKey In case of loggingEntityType is experiment it will be experiment key or else it will be rule number. + * @param options An array of decision options + * @param reasons Decision log messages * @return whether the user meets the criteria for the experiment */ public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull Map attributes, @Nonnull String loggingEntityType, - @Nonnull String loggingKey) { + @Nonnull String loggingKey, + @Nullable List options, + @Nullable DecisionReasons reasons) { if (experiment.getAudienceConditions() != null) { logger.debug("Evaluating audiences for {} \"{}\": {}.", loggingEntityType, loggingKey, experiment.getAudienceConditions()); Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey); @@ -78,12 +93,32 @@ public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig proj } } + /** + * Determines whether a user satisfies audience conditions for the experiment. + * + * @param projectConfig the current projectConfig + * @param experiment the experiment we are evaluating audiences for + * @param attributes the attributes of the user + * @param loggingEntityType It can be either experiment or rule. + * @param loggingKey In case of loggingEntityType is experiment it will be experiment key or else it will be rule number. + * @return whether the user meets the criteria for the experiment + */ + public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig projectConfig, + @Nonnull Experiment experiment, + @Nonnull Map attributes, + @Nonnull String loggingEntityType, + @Nonnull String loggingKey) { + return doesUserMeetAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + } + @Nullable public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull Map attributes, @Nonnull String loggingEntityType, - @Nonnull String loggingKey) { + @Nonnull String loggingKey, + @Nullable List options, + @Nullable DecisionReasons reasons) { List experimentAudienceIds = experiment.getAudienceIds(); // if there are no audiences, ALL users should be part of the experiment @@ -103,30 +138,49 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, Boolean result = implicitOr.evaluate(projectConfig, attributes); - logger.info("Audiences for {} \"{}\" collectively evaluated to {}.", loggingEntityType, loggingKey, result); + DecisionService.logInfo(logger, reasons, "Audiences for %s \"%s\" collectively evaluated to %b.", loggingEntityType, loggingKey, result); return result; } + @Nullable + public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, + @Nonnull Experiment experiment, + @Nonnull Map attributes, + @Nonnull String loggingEntityType, + @Nonnull String loggingKey) { + return evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + } + @Nullable public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull Map attributes, @Nonnull String loggingEntityType, - @Nonnull String loggingKey) { + @Nonnull String loggingKey, + @Nullable List options, + @Nullable DecisionReasons reasons) { Condition conditions = experiment.getAudienceConditions(); if (conditions == null) return null; try { Boolean result = conditions.evaluate(projectConfig, attributes); - logger.info("Audiences for {} \"{}\" collectively evaluated to {}.", loggingEntityType, loggingKey, result); + DecisionService.logInfo(logger, reasons,"Audiences for %s \"%s\" collectively evaluated to %b.", loggingEntityType, loggingKey, result); return result; } catch (Exception e) { - logger.error("Condition invalid", e); + DecisionService.logError(logger, reasons,"Condition invalid: %s", e.getMessage()); return null; } } + @Nullable + public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectConfig, + @Nonnull Experiment experiment, + @Nonnull Map attributes, + @Nonnull String loggingEntityType, + @Nonnull String loggingKey) { + return evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + } -} +} \ No newline at end of file diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index c17184f10..a93d249f9 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -99,7 +99,9 @@ public OptimizelyDecision decide(@Nonnull String key, flag, userId, copiedAttributes, - projectConfig); + projectConfig, + allOptions, + decisionReasons); FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT; SourceInfo sourceInfo = new RolloutSourceInfo(); From 6800e66b1257779f8e594b0cfe890a6c7b597a4f Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 7 Oct 2020 17:18:38 -0700 Subject: [PATCH 03/17] add decide-all api --- .../OptimizelyUserContext.java | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index a93d249f9..06a67cc68 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -196,7 +196,26 @@ public OptimizelyDecision decide(String key) { */ public Map decideAll(@Nonnull String[] keys, @Nonnull OptimizelyDecideOption[] options) { - return new HashMap<>(); + Map decisionMap = new HashMap<>(); + + ProjectConfig projectConfig = optimizely.getProjectConfig(); + if (projectConfig == null) { + logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); + return decisionMap; + } + + if (keys.length == 0) return decisionMap; + + List allOptions = getAllOptions(options); + + for (String key : keys) { + OptimizelyDecision decision = decide(key, options); + if (!allOptions.contains(OptimizelyDecideOption.ENABLED_FLAGS_ONLY) || decision.getEnabled()) { + decisionMap.put(key, decision); + } + } + + return decisionMap; } /** @@ -216,7 +235,18 @@ public Map decideAll(@Nonnull String[] keys) { * @return All decision results mapped by flag keys. */ public Map decideAll(@Nonnull OptimizelyDecideOption[] options) { - String[] allFlagKeys = {}; + Map decisionMap = new HashMap<>(); + + ProjectConfig projectConfig = optimizely.getProjectConfig(); + if (projectConfig == null) { + logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); + return decisionMap; + } + + List allFlags = projectConfig.getFeatureFlags(); + String[] allFlagKeys = new String[allFlags.size()]; + for (int i = 0; i < allFlags.size(); i++) allFlagKeys[i] = allFlags.get(i).getKey(); + return decideAll(allFlagKeys, options); } From b8a19a7e4c1ec0643394d74ed4eb2bfbd0dc121a Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 8 Oct 2020 14:36:26 -0700 Subject: [PATCH 04/17] fix existing tests for decision service --- .../com/optimizely/ab/bucketing/Bucketer.java | 58 ++++--- .../ab/bucketing/DecisionService.java | 163 ++++++++++++------ .../ab/internal/ExperimentUtils.java | 4 +- .../ab/notification/DecisionNotification.java | 2 +- .../DecisionReasons.java | 16 ++ .../OptimizelyDecision.java | 2 +- .../OptimizelyUserContext.java | 16 +- .../com/optimizely/ab/OptimizelyTest.java | 26 +-- .../ab/bucketing/DecisionServiceTest.java | 153 ++++++++-------- 9 files changed, 254 insertions(+), 186 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java index f423472a7..b45abc203 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java @@ -18,11 +18,7 @@ import com.optimizely.ab.annotations.VisibleForTesting; import com.optimizely.ab.bucketing.internal.MurmurHash3; -import com.optimizely.ab.config.Experiment; -import com.optimizely.ab.config.Group; -import com.optimizely.ab.config.ProjectConfig; -import com.optimizely.ab.config.TrafficAllocation; -import com.optimizely.ab.config.Variation; +import com.optimizely.ab.config.*; import com.optimizely.ab.optimizelyusercontext.DecisionReasons; import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; import org.slf4j.Logger; @@ -140,6 +136,36 @@ public Variation bucket(@Nonnull Experiment experiment, @Nonnull ProjectConfig projectConfig, @Nullable List options, @Nullable DecisionReasons reasons) { + + // support existing custom Bucketers + if (isMethodOverriden("bucket", Experiment.class, String.class, ProjectConfig.class)) { + return bucket(experiment, bucketingId, projectConfig); + } + + return bucketCore(experiment, bucketingId, projectConfig, options, reasons); + } + + /** + * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3. + * + * @param experiment The Experiment in which the user is to be bucketed. + * @param bucketingId string A customer-assigned value used to create the key for the murmur hash. + * @param projectConfig The current projectConfig + * @return Variation the user is bucketed into or null. + */ + @Nullable + public Variation bucket(@Nonnull Experiment experiment, + @Nonnull String bucketingId, + @Nonnull ProjectConfig projectConfig) { + return bucketCore(experiment, bucketingId, projectConfig, null, null); + } + + @Nullable + public Variation bucketCore(@Nonnull Experiment experiment, + @Nonnull String bucketingId, + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { // ---------- Bucket User ---------- String groupId = experiment.getGroupId(); // check whether the experiment belongs to a group @@ -170,21 +196,6 @@ public Variation bucket(@Nonnull Experiment experiment, return bucketToVariation(experiment, bucketingId, options, reasons); } - /** - * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3. - * - * @param experiment The Experiment in which the user is to be bucketed. - * @param bucketingId string A customer-assigned value used to create the key for the murmur hash. - * @param projectConfig The current projectConfig - * @return Variation the user is bucketed into or null. - */ - @Nullable - public Variation bucket(@Nonnull Experiment experiment, - @Nonnull String bucketingId, - @Nonnull ProjectConfig projectConfig) { - return bucket(experiment, bucketingId, projectConfig, null, null); - } - //======== Helper methods ========// /** @@ -200,5 +211,12 @@ int generateBucketValue(int hashCode) { return (int) Math.floor(MAX_TRAFFIC_VALUE * ratio); } + Boolean isMethodOverriden(String methodName, Class... params) { + try { + return getClass() != Bucketer.class && getClass().getDeclaredMethod(methodName, params) != null; + } catch (NoSuchMethodException e) { + return false; + } + } } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index d1cfd3b77..588df9cf3 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -94,6 +94,30 @@ public Variation getVariation(@Nonnull Experiment experiment, @Nonnull ProjectConfig projectConfig, @Nullable List options, @Nullable DecisionReasons reasons) { + + // support existing custom DecisionServices + if (isMethodOverriden("getVariation", Experiment.class, String.class, Map.class, ProjectConfig.class)) { + return getVariation(experiment, userId, filteredAttributes, projectConfig); + } + + return getVariationCore(experiment, userId, filteredAttributes, projectConfig, options, reasons); + } + + @Nullable + public Variation getVariation(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + return getVariationCore(experiment, userId, filteredAttributes, projectConfig, null, null); + } + + @Nullable + public Variation getVariationCore(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { if (!ExperimentUtils.isExperimentActive(experiment, options, reasons)) { return null; } @@ -159,23 +183,6 @@ public Variation getVariation(@Nonnull Experiment experiment, return null; } - /** - * Get a {@link Variation} of an {@link Experiment} for a user to be allocated into. - * - * @param experiment The Experiment the user will be bucketed into. - * @param userId The userId of the user. - * @param filteredAttributes The user's attributes. This should be filtered to just attributes in the Datafile. - * @param projectConfig The current projectConfig - * @return The {@link Variation} the user is allocated into. - */ - @Nullable - public Variation getVariation(@Nonnull Experiment experiment, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { - return getVariation(experiment, userId, filteredAttributes, projectConfig, null, null); - } - /** * Get the variation the user is bucketed into for the FeatureFlag * @@ -194,6 +201,30 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, @Nonnull ProjectConfig projectConfig, @Nullable List options, @Nullable DecisionReasons reasons) { + // support existing custom DecisionServices + if (isMethodOverriden("getVariationForFeature", FeatureFlag.class, String.class, Map.class, ProjectConfig.class)) { + return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig); + } + + return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig, options, reasons); + } + + @Nonnull + public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + + return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig,null, null); + } + + @Nonnull + public FeatureDecision getVariationForFeatureCore(@Nonnull FeatureFlag featureFlag, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig, + @Nullable List options, + @Nullable DecisionReasons reasons) { if (!featureFlag.getExperimentIds().isEmpty()) { for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); @@ -217,25 +248,6 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, return featureDecision; } - /** - * Get the variation the user is bucketed into for the FeatureFlag - * - * @param featureFlag The feature flag the user wants to access. - * @param userId User Identifier - * @param filteredAttributes A map of filtered attributes. - * @param projectConfig The current projectConfig - * @return {@link FeatureDecision} - */ - @Nonnull - public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { - - return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig,null, null); - } - - /** * Try to bucket the user into a rollout rule. * Evaluate the user for rules in priority order by seeing if the user satisfies the audience. @@ -299,6 +311,14 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag return new FeatureDecision(null, null, null); } + @Nonnull + FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + return getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig, null, null); + } + /** * Get the variation the user has been whitelisted into. * @@ -330,6 +350,13 @@ Variation getWhitelistedVariation(@Nonnull Experiment experiment, return null; } + @Nullable + Variation getWhitelistedVariation(@Nonnull Experiment experiment, + @Nonnull String userId) { + + return getWhitelistedVariation(experiment, userId, null, null); + } + /** * Get the {@link Variation} that has been stored for the user in the {@link UserProfileService} implementation. * @@ -348,7 +375,7 @@ Variation getStoredVariation(@Nonnull Experiment experiment, @Nullable List options, @Nullable DecisionReasons reasons) { - if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return null; + if (options != null && options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return null; // ---------- Check User Profile for Sticky Bucketing ---------- // If a user profile instance is present then check it for a saved variation @@ -379,6 +406,13 @@ Variation getStoredVariation(@Nonnull Experiment experiment, } } + @Nullable + Variation getStoredVariation(@Nonnull Experiment experiment, + @Nonnull UserProfile userProfile, + @Nonnull ProjectConfig projectConfig) { + return getStoredVariation(experiment, userProfile, projectConfig, null, null); + } + /** * Save a {@link Variation} of an {@link Experiment} for a user in the {@link UserProfileService}. * @@ -394,7 +428,7 @@ void saveVariation(@Nonnull Experiment experiment, @Nullable List options, @Nullable DecisionReasons reasons) { - if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return; + if (options != null && options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return; // only save if the user has implemented a user profile service if (userProfileService != null) { @@ -421,6 +455,12 @@ void saveVariation(@Nonnull Experiment experiment, } } + void saveVariation(@Nonnull Experiment experiment, + @Nonnull Variation variation, + @Nonnull UserProfile userProfile) { + saveVariation(experiment, variation, userProfile, null, null); + } + /** * Get the bucketingId of a user if a bucketingId exists in attributes, or else default to userId. * @@ -447,6 +487,11 @@ String getBucketingId(@Nonnull String userId, return bucketingId; } + String getBucketingId(@Nonnull String userId, + @Nonnull Map filteredAttributes) { + return getBucketingId(userId, filteredAttributes, null, null); + } + public ConcurrentHashMap> getForcedVariationMapping() { return forcedVariationMapping; } @@ -536,6 +581,26 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, @Nullable List options, @Nullable DecisionReasons reasons) { + // support existing custom DecisionServices + if (isMethodOverriden("getForcedVariation", Experiment.class, String.class)) { + return getForcedVariation(experiment, userId); + } + + return getForcedVariationCore(experiment, userId, options, reasons); + } + + @Nullable + public Variation getForcedVariation(@Nonnull Experiment experiment, + @Nonnull String userId) { + return getForcedVariationCore(experiment, userId, null, null); + } + + @Nullable + public Variation getForcedVariationCore(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nullable List options, + @Nullable DecisionReasons reasons) { + // if the user id is invalid, return false. if (!validateUserId(userId)) { return null; @@ -558,20 +623,6 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, return null; } - /** - * Gets the forced variation for a given user and experiment. - * - * @param experiment The experiment forced. - * @param userId The user ID to be used for bucketing. - * @return The variation the user was bucketed into. This value can be null if the - * forced variation fails. - */ - @Nullable - public Variation getForcedVariation(@Nonnull Experiment experiment, - @Nonnull String userId) { - return getForcedVariation(experiment, userId, null, null); - } - /** * Helper function to check that the provided userId is valid * @@ -611,4 +662,12 @@ public static void logDebug(Logger logger, DecisionReasons reasons, String forma if (reasons != null) reasons.addInfo(message); } + Boolean isMethodOverriden(String methodName, Class... params) { + try { + return getClass() != DecisionService.class && getClass().getDeclaredMethod(methodName, params) != null; + } catch (NoSuchMethodException e) { + return false; + } + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java index c1d40b8a3..68628516d 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java @@ -138,7 +138,7 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, Boolean result = implicitOr.evaluate(projectConfig, attributes); - DecisionService.logInfo(logger, reasons, "Audiences for %s \"%s\" collectively evaluated to %b.", loggingEntityType, loggingKey, result); + DecisionService.logInfo(logger, reasons, "Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); return result; } @@ -166,7 +166,7 @@ public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectC try { Boolean result = conditions.evaluate(projectConfig, attributes); - DecisionService.logInfo(logger, reasons,"Audiences for %s \"%s\" collectively evaluated to %b.", loggingEntityType, loggingKey, result); + DecisionService.logInfo(logger, reasons,"Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); return result; } catch (Exception e) { DecisionService.logError(logger, reasons,"Condition invalid: %s", e.getMessage()); diff --git a/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java b/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java index ad95a8ded..d98b12e41 100644 --- a/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java +++ b/core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java @@ -398,7 +398,7 @@ public FlagDecisionNotificationBuilder withEnabled(Boolean enabled) { } public FlagDecisionNotificationBuilder withVariables(Object variables) { - this.enabled = enabled; + this.variables = variables; return this; } diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java index 6cb8c6acb..7230b7291 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java @@ -1,3 +1,19 @@ +/** + * + * Copyright 2020, 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.optimizelyusercontext; import java.util.ArrayList; diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java index 1fd7ccabd..dce7f763e 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java @@ -50,7 +50,7 @@ public OptimizelyDecision(@Nullable String variationKey, @Nullable OptimizelyJSON variables, @Nullable String ruleKey, @Nonnull String flagKey, - @Nullable OptimizelyUserContext userContext, + @Nonnull OptimizelyUserContext userContext, @Nonnull List reasons) { this.variationKey = variationKey; this.enabled = enabled; diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index 9e8782bbe..b0c682db2 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -21,9 +21,6 @@ import com.optimizely.ab.bucketing.FeatureDecision; import com.optimizely.ab.config.*; import com.optimizely.ab.notification.DecisionNotification; -import com.optimizely.ab.notification.FeatureTestSourceInfo; -import com.optimizely.ab.notification.RolloutSourceInfo; -import com.optimizely.ab.notification.SourceInfo; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -46,8 +43,6 @@ public class OptimizelyUserContext { public final static String SDK_NOT_READY = "Optimizely SDK not configured properly yet"; public final static String FLAG_KEY_INVALID = "Flag key \"%s\" is not in datafile."; public final static String VARIABLE_VALUE_INVALID = "Variable value for key \"%s\" is invalid or wrong type."; - public final static String OPTIMIZELY_JSON_ERROR = "Invalid variables for OptimizelyJSON."; - public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull String userId, @@ -119,9 +114,6 @@ public OptimizelyDecision decide(@Nonnull String key, allOptions, decisionReasons); - FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT; - SourceInfo sourceInfo = new RolloutSourceInfo(); - if (flagDecision.variation != null) { if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { if (!allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) { @@ -133,9 +125,6 @@ public OptimizelyDecision decide(@Nonnull String key, flagDecision.variation); sentEvent = true; } - - decisionSource = flagDecision.decisionSource; - sourceInfo = new FeatureTestSourceInfo(flagDecision.experiment.getKey(), flagDecision.variation.getKey()); } else { String message = String.format("The user \"%s\" is not included in an experiment for flag \"%s\".", userId, key); logger.info(message); @@ -156,9 +145,6 @@ public OptimizelyDecision decide(@Nonnull String key, } OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); - if (optimizelyJSON == null) { - decisionReasons.addError(OPTIMIZELY_JSON_ERROR); - } List reasonsToReport = decisionReasons.toReport(allOptions); String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; @@ -205,7 +191,7 @@ public OptimizelyDecision decide(String key) { *
        *
      • If the SDK finds an error for a key, the response will include a decision for the key showing reasons for the error. *
      • The SDK will always return key-mapped decisions. When it can not process requests, it’ll return an empty map after logging the errors. - *
          + *
        * @param keys An array of flag keys for which decisions will be made. * @param options An array of options for decision-making. * @return All decision results mapped by flag keys. diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 2aced2256..d77d54875 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -378,7 +378,7 @@ public void activateForNullVariation() throws Exception { Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); Map testUserAttributes = Collections.singletonMap("browser_type", "chromey"); - when(mockBucketer.bucket(activatedExperiment, testBucketingId, validProjectConfig)).thenReturn(null); + when(mockBucketer.bucket(activatedExperiment, testBucketingId, validProjectConfig, null, null)).thenReturn(null); logbackVerifier.expectMessage(Level.INFO, "Not activating user \"userId\" for experiment \"" + activatedExperiment.getKey() + "\"."); @@ -937,7 +937,7 @@ public void activateWithInvalidDatafile() throws Exception { assertNull(expectedVariation); // make sure we didn't even attempt to bucket the user - verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } //======== track tests ========// @@ -1238,7 +1238,7 @@ public void trackWithInvalidDatafile() throws Exception { optimizely.track("event_with_launched_and_running_experiments", genericUserId); // make sure we didn't even attempt to bucket the user or fire any conversion events - verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } @@ -1255,7 +1255,7 @@ public void getVariation() throws Exception { Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); - when(mockBucketer.bucket(activatedExperiment, testUserId, validProjectConfig)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(activatedExperiment, testUserId, validProjectConfig, null, null)).thenReturn(bucketedVariation); Map testUserAttributes = new HashMap<>(); testUserAttributes.put("browser_type", "chrome"); @@ -1265,7 +1265,7 @@ public void getVariation() throws Exception { testUserAttributes); // verify that the bucketing algorithm was called correctly - verify(mockBucketer).bucket(activatedExperiment, testUserId, validProjectConfig); + verify(mockBucketer).bucket(activatedExperiment, testUserId, validProjectConfig, null, null); assertThat(actualVariation, is(bucketedVariation)); // verify that we didn't attempt to dispatch an event @@ -1286,13 +1286,13 @@ public void getVariationWithExperimentKey() throws Exception { .withConfig(noAudienceProjectConfig) .build(); - when(mockBucketer.bucket(activatedExperiment, testUserId, noAudienceProjectConfig)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(activatedExperiment, testUserId, noAudienceProjectConfig, null, null)).thenReturn(bucketedVariation); // activate the experiment Variation actualVariation = optimizely.getVariation(activatedExperiment.getKey(), testUserId); // verify that the bucketing algorithm was called correctly - verify(mockBucketer).bucket(activatedExperiment, testUserId, noAudienceProjectConfig); + verify(mockBucketer).bucket(activatedExperiment, testUserId, noAudienceProjectConfig, null, null); assertThat(actualVariation, is(bucketedVariation)); // verify that we didn't attempt to dispatch an event @@ -1347,7 +1347,7 @@ public void getVariationWithAudiences() throws Exception { Experiment experiment = validProjectConfig.getExperiments().get(0); Variation bucketedVariation = experiment.getVariations().get(0); - when(mockBucketer.bucket(experiment, testUserId, validProjectConfig)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(experiment, testUserId, validProjectConfig, null, null)).thenReturn(bucketedVariation); Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); @@ -1356,7 +1356,7 @@ public void getVariationWithAudiences() throws Exception { Variation actualVariation = optimizely.getVariation(experiment.getKey(), testUserId, testUserAttributes); - verify(mockBucketer).bucket(experiment, testUserId, validProjectConfig); + verify(mockBucketer).bucket(experiment, testUserId, validProjectConfig, null, null); assertThat(actualVariation, is(bucketedVariation)); } @@ -1397,7 +1397,7 @@ public void getVariationNoAudiences() throws Exception { Experiment experiment = noAudienceProjectConfig.getExperiments().get(0); Variation bucketedVariation = experiment.getVariations().get(0); - when(mockBucketer.bucket(experiment, testUserId, noAudienceProjectConfig)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(experiment, testUserId, noAudienceProjectConfig, null, null)).thenReturn(bucketedVariation); Optimizely optimizely = optimizelyBuilder .withConfig(noAudienceProjectConfig) @@ -1406,7 +1406,7 @@ public void getVariationNoAudiences() throws Exception { Variation actualVariation = optimizely.getVariation(experiment.getKey(), testUserId); - verify(mockBucketer).bucket(experiment, testUserId, noAudienceProjectConfig); + verify(mockBucketer).bucket(experiment, testUserId, noAudienceProjectConfig, null, null); assertThat(actualVariation, is(bucketedVariation)); } @@ -1464,7 +1464,7 @@ public void getVariationForGroupExperimentWithMatchingAttributes() throws Except attributes.put("browser_type", "chrome"); } - when(mockBucketer.bucket(experiment, "user", validProjectConfig)).thenReturn(variation); + when(mockBucketer.bucket(experiment,"user", validProjectConfig, null, null)).thenReturn(variation); Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); @@ -1522,7 +1522,7 @@ public void getVariationWithInvalidDatafile() throws Exception { assertNull(variation); // make sure we didn't even attempt to bucket the user - verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } //======== Notification listeners ========// diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index 2a3030314..c6d2b4a1c 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -15,18 +15,12 @@ ***************************************************************************/ package com.optimizely.ab.bucketing; -import com.optimizely.ab.config.Experiment; -import com.optimizely.ab.config.FeatureFlag; -import com.optimizely.ab.config.ProjectConfig; -import com.optimizely.ab.config.DatafileProjectConfigTestUtils; -import com.optimizely.ab.config.Rollout; -import com.optimizely.ab.config.TrafficAllocation; -import com.optimizely.ab.config.ValidProjectConfigV4; -import com.optimizely.ab.config.Variation; +import ch.qos.logback.classic.Level; +import com.optimizely.ab.config.*; import com.optimizely.ab.error.ErrorHandler; -import com.optimizely.ab.internal.LogbackVerifier; - import com.optimizely.ab.internal.ControlAttribute; +import com.optimizely.ab.internal.LogbackVerifier; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -39,42 +33,13 @@ import java.util.List; import java.util.Map; -import ch.qos.logback.classic.Level; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; - -import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.noAudienceProjectConfigV3; -import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.validProjectConfigV3; -import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.validProjectConfigV4; -import static com.optimizely.ab.config.ValidProjectConfigV4.ATTRIBUTE_HOUSE_KEY; -import static com.optimizely.ab.config.ValidProjectConfigV4.ATTRIBUTE_NATIONALITY_KEY; -import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_ENGLISH_CITIZENS_VALUE; -import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_GRYFFINDOR_VALUE; -import static com.optimizely.ab.config.ValidProjectConfigV4.FEATURE_FLAG_MULTI_VARIATE_FEATURE; -import static com.optimizely.ab.config.ValidProjectConfigV4.FEATURE_FLAG_SINGLE_VARIABLE_INTEGER; -import static com.optimizely.ab.config.ValidProjectConfigV4.FEATURE_MULTI_VARIATE_FEATURE_KEY; -import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_2; -import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE; -import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION; +import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.*; +import static com.optimizely.ab.config.ValidProjectConfigV4.*; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyMapOf; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.atMost; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.junit.Assert.*; +import static org.mockito.Matchers.*; +import static org.mockito.Mockito.*; public class DecisionServiceTest { @@ -130,8 +95,8 @@ public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { // no attributes provided for a experiment that has an audience assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap(), validProjectConfig), is(expectedVariation)); - verify(decisionService).getWhitelistedVariation(experiment, whitelistedUserId); - verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), any(ProjectConfig.class)); + verify(decisionService).getWhitelistedVariation(experiment, whitelistedUserId, null, null); + verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -153,7 +118,7 @@ public void getForcedVariationBeforeWhitelisting() throws Exception { assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap(), validProjectConfig), is(expectedVariation)); //verify(decisionService).getForcedVariation(experiment.getKey(), whitelistedUserId); - verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), any(ProjectConfig.class)); + verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), any(ProjectConfig.class), anyObject(), anyObject()); assertEquals(decisionService.getWhitelistedVariation(experiment, whitelistedUserId), whitelistVariation); assertTrue(decisionService.setForcedVariation(experiment, whitelistedUserId, null)); assertNull(decisionService.getForcedVariation(experiment, whitelistedUserId)); @@ -177,7 +142,7 @@ public void getVariationForcedPrecedesAudienceEval() throws Exception { // no attributes provided for a experiment that has an audience assertThat(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap(), validProjectConfig), is(expectedVariation)); - verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), eq(validProjectConfig)); + verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), eq(validProjectConfig), anyObject(), anyObject()); assertEquals(decisionService.setForcedVariation(experiment, genericUserId, null), true); assertNull(decisionService.getForcedVariation(experiment, genericUserId)); } @@ -322,14 +287,18 @@ public void getVariationForFeatureReturnsNullWhenItGetsNoVariationsForExperiment any(Experiment.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // do not bucket to any rollouts doReturn(new FeatureDecision(null, null, null)).when(decisionService).getVariationForFeatureInRollout( any(FeatureFlag.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // try to get a variation back from the decision service for the feature flag @@ -363,14 +332,18 @@ public void getVariationForFeatureReturnsVariationReturnedFromGetVariation() { eq(ValidProjectConfigV4.EXPERIMENT_MUTEX_GROUP_EXPERIMENT_1), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); doReturn(ValidProjectConfigV4.VARIATION_MUTEX_GROUP_EXP_2_VAR_1).when(decisionService).getVariation( eq(ValidProjectConfigV4.EXPERIMENT_MUTEX_GROUP_EXPERIMENT_2), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); FeatureDecision featureDecision = decisionService.getVariationForFeature( @@ -409,7 +382,9 @@ public void getVariationForFeatureReturnsVariationFromExperimentBeforeRollout() eq(featureExperiment), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // return variation for rollout @@ -418,7 +393,9 @@ public void getVariationForFeatureReturnsVariationFromExperimentBeforeRollout() eq(featureFlag), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // make sure we get the right variation back @@ -436,7 +413,9 @@ public void getVariationForFeatureReturnsVariationFromExperimentBeforeRollout() any(FeatureFlag.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // make sure we ask for experiment bucketing once @@ -444,7 +423,9 @@ public void getVariationForFeatureReturnsVariationFromExperimentBeforeRollout() any(Experiment.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); } @@ -469,7 +450,9 @@ public void getVariationForFeatureReturnsVariationFromRolloutWhenExperimentFails eq(featureExperiment), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // return variation for rollout @@ -478,7 +461,9 @@ public void getVariationForFeatureReturnsVariationFromRolloutWhenExperimentFails eq(featureFlag), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // make sure we get the right variation back @@ -496,7 +481,9 @@ public void getVariationForFeatureReturnsVariationFromRolloutWhenExperimentFails any(FeatureFlag.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); // make sure we ask for experiment bucketing once @@ -504,7 +491,9 @@ public void getVariationForFeatureReturnsVariationFromRolloutWhenExperimentFails any(Experiment.class), anyString(), anyMapOf(String.class, String.class), - any(ProjectConfig.class) + any(ProjectConfig.class), + anyObject(), + anyObject() ); logbackVerifier.expectMessage( @@ -550,7 +539,7 @@ public void getVariationForFeatureInRolloutReturnsNullWhenFeatureIsNotAttachedTo @Test public void getVariationForFeatureInRolloutReturnsNullWhenUserIsExcludedFromAllTraffic() { Bucketer mockBucketer = mock(Bucketer.class); - when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class))).thenReturn(null); + when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(null); DecisionService decisionService = new DecisionService( mockBucketer, @@ -572,7 +561,7 @@ public void getVariationForFeatureInRolloutReturnsNullWhenUserIsExcludedFromAllT // with fall back bucketing, the user has at most 2 chances to get bucketed with traffic allocation // one chance with the audience rollout rule // one chance with the everyone else rule - verify(mockBucketer, atMost(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, atMost(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -583,7 +572,7 @@ public void getVariationForFeatureInRolloutReturnsNullWhenUserIsExcludedFromAllT @Test public void getVariationForFeatureInRolloutReturnsNullWhenUserFailsAllAudiencesAndTraffic() { Bucketer mockBucketer = mock(Bucketer.class); - when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class))).thenReturn(null); + when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(null); DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null); @@ -597,7 +586,7 @@ public void getVariationForFeatureInRolloutReturnsNullWhenUserFailsAllAudiencesA assertNull(featureDecision.decisionSource); // user is only bucketed once for the everyone else rule - verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -611,7 +600,7 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsAllAudie Rollout rollout = ROLLOUT_2; Experiment everyoneElseRule = rollout.getExperiments().get(rollout.getExperiments().size() - 1); Variation expectedVariation = everyoneElseRule.getVariations().get(0); - when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class))).thenReturn(expectedVariation); + when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(expectedVariation); DecisionService decisionService = new DecisionService( mockBucketer, @@ -637,7 +626,7 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsAllAudie assertEquals(FeatureDecision.DecisionSource.ROLLOUT, featureDecision.decisionSource); // verify user is only bucketed once for everyone else rule - verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -652,8 +641,8 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTrafficI Rollout rollout = ROLLOUT_2; Experiment everyoneElseRule = rollout.getExperiments().get(rollout.getExperiments().size() - 1); Variation expectedVariation = everyoneElseRule.getVariations().get(0); - when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class))).thenReturn(null); - when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class))).thenReturn(expectedVariation); + when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(null); + when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(expectedVariation); DecisionService decisionService = new DecisionService( mockBucketer, @@ -675,7 +664,7 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTrafficI logbackVerifier.expectMessage(Level.DEBUG, "User \"genericUserId\" meets conditions for targeting rule \"Everyone Else\"."); // verify user is only bucketed once for everyone else rule - verify(mockBucketer, times(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, times(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -694,9 +683,9 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTrafficI Variation englishCitizenVariation = englishCitizensRule.getVariations().get(0); Experiment everyoneElseRule = rollout.getExperiments().get(rollout.getExperiments().size() - 1); Variation expectedVariation = everyoneElseRule.getVariations().get(0); - when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class))).thenReturn(null); - when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class))).thenReturn(expectedVariation); - when(mockBucketer.bucket(eq(englishCitizensRule), anyString(), any(ProjectConfig.class))).thenReturn(englishCitizenVariation); + when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(null); + when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(expectedVariation); + when(mockBucketer.bucket(eq(englishCitizensRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(englishCitizenVariation); DecisionService decisionService = new DecisionService( mockBucketer, @@ -721,7 +710,7 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTrafficI assertEquals(FeatureDecision.DecisionSource.ROLLOUT, featureDecision.decisionSource); // verify user is only bucketed once for everyone else rule - verify(mockBucketer, times(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, times(2)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } /** @@ -737,9 +726,9 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTargetin Variation englishCitizenVariation = englishCitizensRule.getVariations().get(0); Experiment everyoneElseRule = rollout.getExperiments().get(rollout.getExperiments().size() - 1); Variation everyoneElseVariation = everyoneElseRule.getVariations().get(0); - when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class))).thenReturn(null); - when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class))).thenReturn(everyoneElseVariation); - when(mockBucketer.bucket(eq(englishCitizensRule), anyString(), any(ProjectConfig.class))).thenReturn(englishCitizenVariation); + when(mockBucketer.bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(null); + when(mockBucketer.bucket(eq(everyoneElseRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(everyoneElseVariation); + when(mockBucketer.bucket(eq(englishCitizensRule), anyString(), any(ProjectConfig.class), anyObject(), anyObject())).thenReturn(englishCitizenVariation); DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null); @@ -759,7 +748,7 @@ public void getVariationForFeatureInRolloutReturnsVariationWhenUserFailsTargetin logbackVerifier.expectMessage(Level.DEBUG, "Audience \"4194404272\" evaluated to true."); logbackVerifier.expectMessage(Level.INFO, "Audiences for rule \"3\" collectively evaluated to true"); // verify user is only bucketed once for everyone else rule - verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class)); + verify(mockBucketer, times(1)).bucket(any(Experiment.class), anyString(), any(ProjectConfig.class), anyObject(), anyObject()); } //========= white list tests ==========/ @@ -912,7 +901,7 @@ public void getVariationSavesBucketedVariationIntoUserProfile() throws Exception Collections.singletonMap(experiment.getId(), decision)); Bucketer mockBucketer = mock(Bucketer.class); - when(mockBucketer.bucket(experiment, userProfileId, noAudienceProjectConfig)).thenReturn(variation); + when(mockBucketer.bucket(experiment, userProfileId, noAudienceProjectConfig, null, null)).thenReturn(variation); DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, userProfileService); @@ -974,7 +963,7 @@ public void getVariationSavesANewUserProfile() throws Exception { UserProfileService userProfileService = mock(UserProfileService.class); DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, userProfileService); - when(bucketer.bucket(experiment, userProfileId, noAudienceProjectConfig)).thenReturn(variation); + when(bucketer.bucket(experiment, userProfileId, noAudienceProjectConfig, null, null)).thenReturn(variation); when(userProfileService.lookup(userProfileId)).thenReturn(null); assertEquals(variation, decisionService.getVariation(experiment, userProfileId, Collections.emptyMap(), noAudienceProjectConfig)); @@ -988,7 +977,7 @@ public void getVariationBucketingId() throws Exception { Experiment experiment = validProjectConfig.getExperiments().get(0); Variation expectedVariation = experiment.getVariations().get(0); - when(bucketer.bucket(experiment, "bucketId", validProjectConfig)).thenReturn(expectedVariation); + when(bucketer.bucket(experiment, "bucketId", validProjectConfig, null, null)).thenReturn(expectedVariation); Map attr = new HashMap(); attr.put(ControlAttribute.BUCKETING_ATTRIBUTE.toString(), "bucketId"); @@ -1012,8 +1001,8 @@ public void getVariationForRolloutWithBucketingId() { attributes.put(ControlAttribute.BUCKETING_ATTRIBUTE.toString(), bucketingId); Bucketer bucketer = mock(Bucketer.class); - when(bucketer.bucket(rolloutRuleExperiment, userId, v4ProjectConfig)).thenReturn(null); - when(bucketer.bucket(rolloutRuleExperiment, bucketingId, v4ProjectConfig)).thenReturn(rolloutVariation); + when(bucketer.bucket(rolloutRuleExperiment, userId, v4ProjectConfig, null, null)).thenReturn(null); + when(bucketer.bucket(rolloutRuleExperiment, bucketingId, v4ProjectConfig, null, null)).thenReturn(rolloutVariation); DecisionService decisionService = spy(new DecisionService( bucketer, From e3cedab51f6af0f3704fa700a2a2a73a5bb2b693 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 8 Oct 2020 17:13:29 -0700 Subject: [PATCH 05/17] add more decide tests --- .../OptimizelyDecision.java | 18 ++ .../OptimizelyUserContext.java | 17 +- .../OptimizelyUserContextTest.java | 306 +++++++++++++++++- 3 files changed, 334 insertions(+), 7 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java index dce7f763e..9d7d67ff1 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java @@ -112,4 +112,22 @@ public boolean hasFailed() { return variationKey == null; } + @Override + public boolean equals(Object obj) { + if (obj == null || getClass() != obj.getClass()) return false; + if (obj == this) return true; + OptimizelyDecision d = (OptimizelyDecision) obj; + return equals(variationKey, d.getVariationKey()) && + equals(enabled, d.getEnabled()) && + equals(variables.toMap(), d.getVariables().toMap()) && + equals(ruleKey, d.getRuleKey()) && + equals(flagKey, d.getFlagKey()) && + equals(userContext, d.getUserContext()) && + equals(reasons, d.getReasons()); + } + + private static boolean equals(Object a, Object b) { + return a == b || (a != null && a.equals(b)); + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index b0c682db2..3119913f0 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -101,9 +101,10 @@ public OptimizelyDecision decide(@Nonnull String key, } List allOptions = getAllOptions(options); - DecisionReasons decisionReasons = new DecisionReasons(); Boolean sentEvent = false; Boolean flagEnabled = false; + DecisionReasons decisionReasons = new DecisionReasons(); + Boolean includeReasons = allOptions.contains(OptimizelyDecideOption.INCLUDE_REASONS); Map copiedAttributes = copyAttributes(); FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( @@ -112,7 +113,7 @@ public OptimizelyDecision decide(@Nonnull String key, copiedAttributes, projectConfig, allOptions, - decisionReasons); + includeReasons ? decisionReasons : null); if (flagDecision.variation != null) { if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { @@ -141,7 +142,7 @@ public OptimizelyDecision decide(@Nonnull String key, flag, flagDecision.variation, flagEnabled, - decisionReasons); + includeReasons ? decisionReasons : null); } OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); @@ -330,4 +331,14 @@ private Map getDecisionVariableMap(FeatureFlag flag, return valuesMap; } + @Override + public boolean equals(Object obj) { + if (obj == null || getClass() != obj.getClass()) return false; + if (obj == this) return true; + OptimizelyUserContext userContext = (OptimizelyUserContext) obj; + return userId.equals(userContext.getUserId()) && + attributes.equals(userContext.getAttributes()) && + optimizely.equals(userContext.getOptimizely()); + } + } diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index 3f08873f4..0418eacf7 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -19,6 +19,7 @@ import com.google.common.base.Charsets; import com.google.common.io.Resources; import com.optimizely.ab.Optimizely; +import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.junit.Before; import org.junit.Test; @@ -29,6 +30,8 @@ import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_GRYFFINDOR_VALUE; import static junit.framework.TestCase.assertEquals; import static junit.framework.TestCase.assertTrue; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; public class OptimizelyUserContextTest { @@ -45,7 +48,7 @@ public void setUp() throws Exception { } @Test - public void testOptimizelyUserContext_withAttributes() { + public void optimizelyUserContext_withAttributes() { Map attributes = Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); OptimizelyUserContext user = new OptimizelyUserContext(optimizely, userId, attributes); @@ -55,7 +58,7 @@ public void testOptimizelyUserContext_withAttributes() { } @Test - public void testOptimizelyUserContext_noAttributes() { + public void optimizelyUserContext_noAttributes() { OptimizelyUserContext user = new OptimizelyUserContext(optimizely, userId); assertEquals(user.getOptimizely(), optimizely); @@ -64,7 +67,7 @@ public void testOptimizelyUserContext_noAttributes() { } @Test - public void testSetAttribute() { + public void setAttribute() { Map attributes = Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); OptimizelyUserContext user = new OptimizelyUserContext(optimizely, userId, attributes); @@ -84,7 +87,7 @@ public void testSetAttribute() { } @Test - public void testSetAttribute_override() { + public void setAttribute_override() { Map attributes = Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); OptimizelyUserContext user = new OptimizelyUserContext(optimizely, userId, attributes); @@ -98,4 +101,299 @@ public void testSetAttribute_override() { assertEquals(newAttributes.get(ATTRIBUTE_HOUSE_KEY), "v2"); } + // decide + + @Test + public void decide() { + String flagKey = "feature_2"; + OptimizelyJSON variablesExpected = optimizely.getAllFeatureVariables(flagKey, userId); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertEquals(decision.getVariationKey(), "variation_with_traffic"); + assertTrue(decision.getEnabled()); + assertEquals(decision.getVariables().toMap(), variablesExpected.toMap()); + assertEquals(decision.getFlagKey(), flagKey); + assertEquals(decision.getUserContext(), user); + assertTrue(decision.getReasons().isEmpty()); + } + + // decideAll + + @Test + public void decideAll_oneFeature() { + String flagKey = "feature_2"; + String[] flagKeys = {flagKey}; + OptimizelyJSON variablesExpected = optimizely.getAllFeatureVariables(flagKey, userId); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + Map decisions = user.decideAll(flagKeys); + + assertTrue(decisions.size() == 1); + OptimizelyDecision decision = decisions.get(flagKey); + + OptimizelyDecision expDecision = new OptimizelyDecision( + "variation_with_traffic", + true, + variablesExpected, + null, + flagKey, + user, + Collections.emptyList()); + assertEquals(decision, expDecision); + } + + @Test + public void decideAll_twoFeatures() { + String flagKey1 = "feature_1"; + String flagKey2 = "feature_2"; + + String[] flagKeys = {flagKey1, flagKey2}; + OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); + OptimizelyJSON variablesExpected2 = optimizely.getAllFeatureVariables(flagKey2, userId); + + OptimizelyUserContext user = optimizely.createUserContext(userId, Collections.singletonMap("gender", "f")); + Map decisions = user.decideAll(flagKeys); + + assertTrue(decisions.size() == 2); + + assertEquals( + decisions.get(flagKey1), + new OptimizelyDecision("a", + true, + variablesExpected1, + null, + flagKey1, + user, + Collections.emptyList())); + assertEquals( + decisions.get(flagKey2), + new OptimizelyDecision("variation_with_traffic", + true, + variablesExpected2, + null, + flagKey2, + user, + Collections.emptyList())); + } + + @Test + public void decideAll_allFeatures() { + String flagKey1 = "feature_1"; + String flagKey2 = "feature_2"; + String flagKey3 = "feature_3"; + + OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); + OptimizelyJSON variablesExpected2 = optimizely.getAllFeatureVariables(flagKey2, userId); + OptimizelyJSON variablesExpected3 = new OptimizelyJSON(Collections.emptyMap()); + + OptimizelyUserContext user = optimizely.createUserContext(userId, Collections.singletonMap("gender", "f")); + Map decisions = user.decideAll(); + + assertTrue(decisions.size() == 3); + + assertEquals( + decisions.get(flagKey1), + new OptimizelyDecision( + "a", + true, + variablesExpected1, + null, + flagKey1, + user, + Collections.emptyList())); + assertEquals( + decisions.get(flagKey2), + new OptimizelyDecision( + "variation_with_traffic", + true, + variablesExpected2, + null, + flagKey2, + user, + Collections.emptyList())); + assertEquals( + decisions.get(flagKey3), + new OptimizelyDecision( + null, + false, + variablesExpected3, + null, + flagKey3, + user, + Collections.emptyList())); + } + + @Test + public void decideAll_allFeatures_enabledOnly() { + String flagKey1 = "feature_1"; + OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); + + OptimizelyUserContext user = optimizely.createUserContext(userId, Collections.singletonMap("gender", "f")); + OptimizelyDecideOption[] decideOptions = {OptimizelyDecideOption.ENABLED_FLAGS_ONLY}; + Map decisions = user.decideAll(decideOptions); + + assertTrue(decisions.size() == 2); + + assertEquals( + decisions.get(flagKey1), + new OptimizelyDecision( + "a", + true, + variablesExpected1, + null, + flagKey1, + user, + Collections.emptyList())); + } + + // send events + + @Test + public void decide_sendEvent() { + + } + + @Test + public void decide_doNotSendEvent() { + + } + + // options + + @Test + public void decideOptions_disbleTracking() { + } + + @Test + public void decideOptions_useUPSbyDefault() { + } + + @Test + public void decideOptions_bypassUPS_doNotUpdateUPS() { + } + + @Test + public void decideOptions_bypassUPS_doNotReadUPS() { + } + + @Test + public void decideOptions_excludeVariables() { + } + + @Test + public void decideOptions_defaultDecideOption() { + } + + // errors + + @Test + public void decide_sdkNotReady() { + String flagKey = "feature_1"; + + Optimizely optimizely = new Optimizely.Builder().build(); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertNull(decision.getVariationKey()); + assertFalse(decision.getEnabled()); + assertNull(decision.getVariables()); + assertEquals(decision.getFlagKey(), flagKey); + assertEquals(decision.getUserContext(), user); + + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(1), OptimizelyUserContext.SDK_NOT_READY); + } + + @Test + public void decide_invalidFeatureKey() { + + } + + @Test + public void decideAll_sdkNotReady() { + + } + + @Test + public void decideAll_errorDecisionIncluded() { + + } + + // reasons (errors) + + @Test + public void decideReasons_sdkNotReady() { + + } + + @Test + public void decideReasons_featureKeyInvalid() { + + } + + @Test + public void decideReasons_variableValueInvalid() { + + } + + // reasons (logs with includeReasons) + + @Test + public void decideReasons_conditionNoMatchingAudience() {} + @Test + public void decideReasons_conditionInvalidFormat() {} + @Test + public void decideReasons_evaluateAttributeInvalidCondition() {} + @Test + public void decideReasons_evaluateAttributeInvalidType() {} + @Test + public void decideReasons_evaluateAttributeValueOutOfRange() {} + @Test + public void decideReasons_userAttributeInvalidType() {} + @Test + public void decideReasons_userAttributeInvalidMatch() {} + @Test + public void decideReasons_userAttributeNilValue() {} + @Test + public void decideReasons_userAttributeInvalidName() {} + @Test + public void decideReasons_missingAttributeValue() {} + + @Test + public void decideReasons_experimentNotRunning() {} + @Test + public void decideReasons_gotVariationFromUserProfile() {} + @Test + public void decideReasons_forcedVariationFound() {} + @Test + public void decideReasons_forcedVariationFoundButInvalid() {} + @Test + public void decideReasons_userMeetsConditionsForTargetingRule() {} + @Test + public void decideReasons_userDoesntMeetConditionsForTargetingRule() {} + @Test + public void decideReasons_userBucketedIntoTargetingRule() {} + @Test + public void decideReasons_userBucketedIntoEveryoneTargetingRule() {} + @Test + public void decideReasons_userNotBucketedIntoTargetingRule() {} + @Test + public void decideReasons_userBucketedIntoVariationInExperiment() {} + @Test + public void decideReasons_userNotBucketedIntoVariation() {} + @Test + public void decideReasons_userBucketedIntoInvalidVariation() {} + @Test + public void decideReasons_userBucketedIntoExperimentInGroup() {} + @Test + public void decideReasons_userNotBucketedIntoExperimentInGroup() {} + @Test + public void decideReasons_userNotBucketedIntoAnyExperimentInGroup() {} + @Test + public void decideReasons_userBucketedIntoInvalidExperiment() {} + @Test + public void decideReasons_userNotInExperiment() {} } From 2c83fa27229f70dd08ecb66dfd237b9735bc0b05 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 9 Oct 2020 09:54:49 -0700 Subject: [PATCH 06/17] add more tests --- .../ab/optimizelyjson/OptimizelyJSON.java | 15 +++++ .../OptimizelyDecision.java | 15 ++++- .../OptimizelyUserContext.java | 7 +++ .../OptimizelyUserContextTest.java | 57 ++++++++++++++++++- 4 files changed, 90 insertions(+), 4 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java b/core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java index 811999e24..2815dea6d 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java @@ -157,5 +157,20 @@ private T getValueInternal(@Nullable Object object, Class clazz) { return null; } + @Override + public boolean equals(Object obj) { + if (obj == null || getClass() != obj.getClass()) return false; + if (obj == this) return true; + if (toMap() == null) return false; + + return toMap().equals(((OptimizelyJSON) obj).toMap()); + } + + @Override + public int hashCode() { + int hash = toMap() != null ? toMap().hashCode() : 0; + return hash; + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java index 9d7d67ff1..2eafd4001 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java @@ -119,7 +119,7 @@ public boolean equals(Object obj) { OptimizelyDecision d = (OptimizelyDecision) obj; return equals(variationKey, d.getVariationKey()) && equals(enabled, d.getEnabled()) && - equals(variables.toMap(), d.getVariables().toMap()) && + equals(variables, d.getVariables()) && equals(ruleKey, d.getRuleKey()) && equals(flagKey, d.getFlagKey()) && equals(userContext, d.getUserContext()) && @@ -130,4 +130,17 @@ private static boolean equals(Object a, Object b) { return a == b || (a != null && a.equals(b)); } + @Override + public int hashCode() { + int hash = variationKey != null ? variationKey.hashCode() : 0; + hash = 31 * hash + (enabled ? 1 : 0); + hash = 31 * hash + (variables != null ? variables.hashCode() : 0); + hash = 31 * hash + (ruleKey != null ? ruleKey.hashCode() : 0); + hash = 31 * hash + flagKey.hashCode(); + hash = 31 * hash + userContext.hashCode(); + hash = 31 * hash + reasons.hashCode(); + return hash; + } + + } diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index 3119913f0..e90041d49 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -341,4 +341,11 @@ public boolean equals(Object obj) { optimizely.equals(userContext.getOptimizely()); } + @Override + public int hashCode() { + int hash = userId.hashCode(); + hash = 31 * hash + attributes.hashCode(); + hash = 31 * hash + optimizely.hashCode(); + return hash; + } } diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index 0418eacf7..c9945822d 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -293,7 +293,6 @@ public void decide_sdkNotReady() { String flagKey = "feature_1"; Optimizely optimizely = new Optimizely.Builder().build(); - OptimizelyUserContext user = optimizely.createUserContext(userId); OptimizelyDecision decision = user.decide(flagKey); @@ -304,39 +303,91 @@ public void decide_sdkNotReady() { assertEquals(decision.getUserContext(), user); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(1), OptimizelyUserContext.SDK_NOT_READY); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.SDK_NOT_READY); } @Test public void decide_invalidFeatureKey() { + String flagKey = "invalid_key"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + assertNull(decision.getVariationKey()); + assertFalse(decision.getEnabled()); + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); } @Test public void decideAll_sdkNotReady() { + String[] flagKeys = {"feature_1"}; + + Optimizely optimizely = new Optimizely.Builder().build(); + OptimizelyUserContext user = optimizely.createUserContext(userId); + Map decisions = user.decideAll(flagKeys); + assertEquals(decisions.size(), 0); } @Test public void decideAll_errorDecisionIncluded() { + String flagKey1 = "feature_2"; + String flagKey2 = "invalid_key"; + String[] flagKeys = {flagKey1, flagKey2}; + OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + Map decisions = user.decideAll(flagKeys); + + assertEquals(decisions.size(), 2); + + assertEquals( + decisions.get(flagKey1), + new OptimizelyDecision( + "variation_with_traffic", + true, + variablesExpected1, + null, + flagKey1, + user, + Collections.emptyList())); + assertEquals( + decisions.get(flagKey2), + OptimizelyDecision.createErrorDecision( + flagKey2, + user, + OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey2))); } // reasons (errors) @Test public void decideReasons_sdkNotReady() { + String flagKey = "feature_1"; + Optimizely optimizely = new Optimizely.Builder().build(); + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.SDK_NOT_READY); } @Test public void decideReasons_featureKeyInvalid() { + String flagKey = "invalid_key"; + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); } @Test public void decideReasons_variableValueInvalid() { - } // reasons (logs with includeReasons) From bb9f4dc2e93c32a5d4044cd0ba97eee5d5f70fe6 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Mon, 12 Oct 2020 15:10:32 -0700 Subject: [PATCH 07/17] change ruleKey to have a copy of experimentKey --- .../OptimizelyUserContext.java | 4 ++-- .../OptimizelyUserContextTest.java | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index e90041d49..39d99d8a9 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -149,8 +149,8 @@ public OptimizelyDecision decide(@Nonnull String key, List reasonsToReport = decisionReasons.toReport(allOptions); String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; - // TODO: add ruleKey values when available later. - String ruleKey = null; + // TODO: add ruleKey values when available later. use a copy of experimentKey until then. + String ruleKey = flagDecision.experiment != null ? flagDecision.experiment.getKey() : null; DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder() .withUserId(userId) diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index c9945822d..625529696 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -137,7 +137,7 @@ public void decideAll_oneFeature() { "variation_with_traffic", true, variablesExpected, - null, + "exp_no_audience", flagKey, user, Collections.emptyList()); @@ -163,7 +163,7 @@ public void decideAll_twoFeatures() { new OptimizelyDecision("a", true, variablesExpected1, - null, + "exp_with_audience", flagKey1, user, Collections.emptyList())); @@ -172,7 +172,7 @@ public void decideAll_twoFeatures() { new OptimizelyDecision("variation_with_traffic", true, variablesExpected2, - null, + "exp_no_audience", flagKey2, user, Collections.emptyList())); @@ -199,7 +199,7 @@ public void decideAll_allFeatures() { "a", true, variablesExpected1, - null, + "exp_with_audience", flagKey1, user, Collections.emptyList())); @@ -209,7 +209,7 @@ public void decideAll_allFeatures() { "variation_with_traffic", true, variablesExpected2, - null, + "exp_no_audience", flagKey2, user, Collections.emptyList())); @@ -242,7 +242,7 @@ public void decideAll_allFeatures_enabledOnly() { "a", true, variablesExpected1, - null, + "exp_with_audience", flagKey1, user, Collections.emptyList())); @@ -349,7 +349,7 @@ public void decideAll_errorDecisionIncluded() { "variation_with_traffic", true, variablesExpected1, - null, + "exp_no_audience", flagKey1, user, Collections.emptyList())); From 3223a640e4d599bdbdad7dd4b54fec54d4ad1575 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 13 Oct 2020 16:56:27 -0700 Subject: [PATCH 08/17] add all tests (except for reasons) --- .../java/com/optimizely/ab/Optimizely.java | 18 +- .../OptimizelyUserContext.java | 10 +- .../optimizely/ab/OptimizelyBuilderTest.java | 8 +- .../OptimizelyUserContextTest.java | 244 +++++++++++++++++- .../config/decide-project-config.json | 4 + 5 files changed, 253 insertions(+), 31 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 1f874b7ac..1bb448212 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -87,7 +87,7 @@ public class Optimizely implements AutoCloseable { @VisibleForTesting final ErrorHandler errorHandler; - public final OptimizelyDecideOption[] defaultDecideOptions; + public final List defaultDecideOptions; private final ProjectConfigManager projectConfigManager; @@ -108,7 +108,7 @@ private Optimizely(@Nonnull EventHandler eventHandler, @Nonnull ProjectConfigManager projectConfigManager, @Nullable OptimizelyConfigManager optimizelyConfigManager, @Nonnull NotificationCenter notificationCenter, - @Nonnull OptimizelyDecideOption[] defaultDecideOptions + @Nonnull List defaultDecideOptions ) { this.eventHandler = eventHandler; this.eventProcessor = eventProcessor; @@ -1217,7 +1217,7 @@ public static class Builder { private OptimizelyConfigManager optimizelyConfigManager; private UserProfileService userProfileService; private NotificationCenter notificationCenter; - private OptimizelyDecideOption[] defaultDecideOptions; + private List defaultDecideOptions; // For backwards compatibility private AtomicProjectConfigManager fallbackConfigManager = new AtomicProjectConfigManager(); @@ -1289,6 +1289,11 @@ public Builder withDatafile(String datafile) { return this; } + public Builder withDefaultDecideOptions(OptimizelyDecideOption[] options) { + this.defaultDecideOptions = new ArrayList<>(Arrays.asList(options)); + return this; + } + // Helper functions for making testing easier protected Builder withBucketing(Bucketer bucketer) { this.bucketer = bucketer; @@ -1305,11 +1310,6 @@ protected Builder withDecisionService(DecisionService decisionService) { return this; } - protected Builder withDefaultDecideOptions(OptimizelyDecideOption[] options) { - this.defaultDecideOptions = options; - return this; - } - public Optimizely build() { if (errorHandler == null) { @@ -1363,7 +1363,7 @@ public Optimizely build() { } if (defaultDecideOptions == null) { - defaultDecideOptions = new OptimizelyDecideOption[0]; + defaultDecideOptions = new ArrayList<>(); } return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions); diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index 39d99d8a9..67cfa8557 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -40,8 +40,8 @@ public class OptimizelyUserContext { private static final Logger logger = LoggerFactory.getLogger(OptimizelyUserContext.class); - public final static String SDK_NOT_READY = "Optimizely SDK not configured properly yet"; - public final static String FLAG_KEY_INVALID = "Flag key \"%s\" is not in datafile."; + public final static String SDK_NOT_READY = "Optimizely SDK not configured properly yet."; + public final static String FLAG_KEY_INVALID = "No flag was found for key \"%s\"."; public final static String VARIABLE_VALUE_INVALID = "Variable value for key \"%s\" is invalid or wrong type."; public OptimizelyUserContext(@Nonnull Optimizely optimizely, @@ -291,9 +291,9 @@ private Map copyAttributes() { } private List getAllOptions(OptimizelyDecideOption[] options) { - List allOptions = new ArrayList(Arrays.asList(optimizely.defaultDecideOptions)); - allOptions.addAll(Arrays.asList(options)); - return allOptions; + List copiedOptions = new ArrayList(optimizely.defaultDecideOptions); + copiedOptions.addAll(Arrays.asList(options)); + return copiedOptions; } public static String getFlagKeyInvalidMessage(String flagKey) { diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java index 6779830da..45c9511f9 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java @@ -182,14 +182,14 @@ public void withDefaultDecideOptions() throws Exception { Optimizely optimizelyClient = Optimizely.builder(validConfigJsonV4(), mockEventHandler) .build(); - assertEquals(optimizelyClient.defaultDecideOptions.length, 0); + assertEquals(optimizelyClient.defaultDecideOptions.size(), 0); optimizelyClient = Optimizely.builder(validConfigJsonV4(), mockEventHandler) .withDefaultDecideOptions(options) .build(); - assertEquals(optimizelyClient.defaultDecideOptions[0], OptimizelyDecideOption.DISABLE_DECISION_EVENT); - assertEquals(optimizelyClient.defaultDecideOptions[1], OptimizelyDecideOption.ENABLED_FLAGS_ONLY); - assertEquals(optimizelyClient.defaultDecideOptions[2], OptimizelyDecideOption.EXCLUDE_VARIABLES); + assertEquals(optimizelyClient.defaultDecideOptions.get(0), OptimizelyDecideOption.DISABLE_DECISION_EVENT); + assertEquals(optimizelyClient.defaultDecideOptions.get(1), OptimizelyDecideOption.ENABLED_FLAGS_ONLY); + assertEquals(optimizelyClient.defaultDecideOptions.get(2), OptimizelyDecideOption.EXCLUDE_VARIABLES); } } diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index 625529696..18d3730c3 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -18,29 +18,44 @@ import com.google.common.base.Charsets; import com.google.common.io.Resources; +import com.optimizely.ab.EventHandlerRule; import com.optimizely.ab.Optimizely; +import com.optimizely.ab.bucketing.UserProfileService; +import com.optimizely.ab.event.ForwardingEventProcessor; +import com.optimizely.ab.notification.NotificationCenter; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import java.util.Collections; +import java.util.HashMap; +import java.util.List; import java.util.Map; import static com.optimizely.ab.config.ValidProjectConfigV4.ATTRIBUTE_HOUSE_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_GRYFFINDOR_VALUE; +import static com.optimizely.ab.notification.DecisionNotification.ExperimentDecisionNotificationBuilder.VARIATION_KEY; +import static com.optimizely.ab.notification.DecisionNotification.FlagDecisionNotificationBuilder.*; import static junit.framework.TestCase.assertEquals; import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.*; public class OptimizelyUserContextTest { + @Rule + public EventHandlerRule eventHandler = new EventHandlerRule(); public Optimizely optimizely; + public String datafile; public String userId = "tester"; + boolean isListenerCalled = false; @Before public void setUp() throws Exception { - String datafile = Resources.toString(Resources.getResource("config/decide-project-config.json"), Charsets.UTF_8); + datafile = Resources.toString(Resources.getResource("config/decide-project-config.json"), Charsets.UTF_8); optimizely = new Optimizely.Builder() .withDatafile(datafile) @@ -114,6 +129,7 @@ public void decide() { assertEquals(decision.getVariationKey(), "variation_with_traffic"); assertTrue(decision.getEnabled()); assertEquals(decision.getVariables().toMap(), variablesExpected.toMap()); + assertEquals(decision.getRuleKey(), "exp_no_audience"); assertEquals(decision.getFlagKey(), flagKey); assertEquals(decision.getUserContext(), user); assertTrue(decision.getReasons().isEmpty()); @@ -122,7 +138,7 @@ public void decide() { // decideAll @Test - public void decideAll_oneFeature() { + public void decideAll_oneFlag() { String flagKey = "feature_2"; String[] flagKeys = {flagKey}; OptimizelyJSON variablesExpected = optimizely.getAllFeatureVariables(flagKey, userId); @@ -145,7 +161,7 @@ public void decideAll_oneFeature() { } @Test - public void decideAll_twoFeatures() { + public void decideAll_twoFlags() { String flagKey1 = "feature_1"; String flagKey2 = "feature_2"; @@ -179,7 +195,7 @@ public void decideAll_twoFeatures() { } @Test - public void decideAll_allFeatures() { + public void decideAll_allFlags() { String flagKey1 = "feature_1"; String flagKey2 = "feature_2"; String flagKey3 = "feature_3"; @@ -226,7 +242,7 @@ public void decideAll_allFeatures() { } @Test - public void decideAll_allFeatures_enabledOnly() { + public void decideAll_allFlags_enabledFlagsOnly() { String flagKey1 = "feature_1"; OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); @@ -248,42 +264,227 @@ public void decideAll_allFeatures_enabledOnly() { Collections.emptyList())); } + // trackEvent + + @Test + public void trackEvent() { + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withEventProcessor(new ForwardingEventProcessor(eventHandler, null)) + .build(); + + Map attributes = Collections.singletonMap("gender", "f"); + String eventKey = "event1"; + Map eventTags = Collections.singletonMap("name", "carrot"); + OptimizelyUserContext user = optimizely.createUserContext(userId, attributes); + user.trackEvent(eventKey, eventTags); + + eventHandler.expectConversion(eventKey, userId, attributes, eventTags); + } + + @Test + public void trackEvent_noEventTags() { + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withEventProcessor(new ForwardingEventProcessor(eventHandler, null)) + .build(); + + Map attributes = Collections.singletonMap("gender", "f"); + String eventKey = "event1"; + OptimizelyUserContext user = optimizely.createUserContext(userId, attributes); + user.trackEvent(eventKey); + + eventHandler.expectConversion(eventKey, userId, attributes); + } + + @Test + public void trackEvent_emptyAttributes() { + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withEventProcessor(new ForwardingEventProcessor(eventHandler, null)) + .build(); + + String eventKey = "event1"; + Map eventTags = Collections.singletonMap("name", "carrot"); + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.trackEvent(eventKey, eventTags); + + eventHandler.expectConversion(eventKey, userId, Collections.emptyMap(), eventTags); + } + // send events @Test public void decide_sendEvent() { + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withEventProcessor(new ForwardingEventProcessor(eventHandler, null)) + .build(); + + String flagKey = "feature_2"; + String experimentId = "10420810910"; + String variationId = "10418551353"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertEquals(decision.getVariationKey(), "variation_with_traffic"); + eventHandler.expectImpression(experimentId, variationId, userId); } @Test public void decide_doNotSendEvent() { + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withEventProcessor(new ForwardingEventProcessor(eventHandler, null)) + .build(); + String flagKey = "feature_2"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.DISABLE_DECISION_EVENT}); + + assertEquals(decision.getVariationKey(), "variation_with_traffic"); } - // options + // notifications @Test - public void decideOptions_disbleTracking() { + public void decisionNotification() { + String flagKey = "feature_2"; + String variationKey = "variation_with_traffic"; + boolean enabled = true; + OptimizelyJSON variables = optimizely.getAllFeatureVariables(flagKey, userId); + String ruleKey = "exp_no_audience"; + List reasons = Collections.emptyList(); + + final Map testDecisionInfoMap = new HashMap<>(); + testDecisionInfoMap.put(FLAG_KEY, flagKey); + testDecisionInfoMap.put(VARIATION_KEY, variationKey); + testDecisionInfoMap.put(ENABLED, enabled); + testDecisionInfoMap.put(VARIABLES, variables.toMap()); + testDecisionInfoMap.put(RULE_KEY, ruleKey); + testDecisionInfoMap.put(REASONS, reasons); + + Map attributes = Collections.singletonMap("gender", "f"); + OptimizelyUserContext user = optimizely.createUserContext(userId, attributes); + + optimizely.addDecisionNotificationHandler( + decisionNotification -> { + Assert.assertEquals(decisionNotification.getType(), NotificationCenter.DecisionNotificationType.FLAG.toString()); + Assert.assertEquals(decisionNotification.getUserId(), userId); + Assert.assertEquals(decisionNotification.getAttributes(), attributes); + Assert.assertEquals(decisionNotification.getDecisionInfo(), testDecisionInfoMap); + isListenerCalled = true; + }); + + isListenerCalled = false; + testDecisionInfoMap.put(DECISION_EVENT_DISPATCHED, true); + user.decide(flagKey); + assertTrue(isListenerCalled); + + isListenerCalled = false; + testDecisionInfoMap.put(DECISION_EVENT_DISPATCHED, false); + user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.DISABLE_DECISION_EVENT}); + assertTrue(isListenerCalled); } + // options + @Test - public void decideOptions_useUPSbyDefault() { + public void decideOptions_bypassUPS() throws Exception { + String flagKey = "feature_2"; // embedding experiment: "exp_no_audience" + String experimentId = "10420810910"; // "exp_no_audience" + String variationId1 = "10418551353"; + String variationId2 = "10418510624"; + String variationKey1 = "variation_with_traffic"; + String variationKey2 = "variation_no_traffic"; + + UserProfileService ups = mock(UserProfileService.class); + when(ups.lookup(userId)).thenReturn(createUserProfileMap(experimentId, variationId2)); + + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withUserProfileService(ups) + .build(); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + // should return variationId2 set by UPS + assertEquals(decision.getVariationKey(), variationKey2); + + decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE}); + // should ignore variationId2 set by UPS and return variationId1 + assertEquals(decision.getVariationKey(), variationKey1); + // also should not save either + verify(ups, never()).save(anyObject()); } @Test - public void decideOptions_bypassUPS_doNotUpdateUPS() { + public void decideOptions_excludeVariables() { + String flagKey = "feature_1"; + OptimizelyUserContext user = optimizely.createUserContext(userId); + + OptimizelyDecision decision = user.decide(flagKey); + assertTrue(decision.getVariables().toMap().size() > 0); + + decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.EXCLUDE_VARIABLES}); + assertTrue(decision.getVariables().toMap().size() == 0); } @Test - public void decideOptions_bypassUPS_doNotReadUPS() { + public void decideOptions_includeReasons() { + OptimizelyUserContext user = optimizely.createUserContext(userId); + + String flagKey = "invalid_key"; + OptimizelyDecision decision = user.decide(flagKey); + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + + decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + assertEquals(decision.getReasons().size(), 1); + assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + + flagKey = "feature_1"; + decision = user.decide(flagKey); + assertEquals(decision.getReasons().size(), 0); + + decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + assertTrue(decision.getReasons().size() > 0); } - @Test - public void decideOptions_excludeVariables() { + public void decideOptions_disableDispatchEvent() { + // tested already with decide_doNotSendEvent() above + } + + public void decideOptions_enabledFlagsOnly() { + // tested already with decideAll_allFlags_enabledFlagsOnly() above } @Test - public void decideOptions_defaultDecideOption() { + public void decideOptions_defaultDecideOptions() { + OptimizelyDecideOption[] options = { + OptimizelyDecideOption.EXCLUDE_VARIABLES + }; + + optimizely = Optimizely.builder() + .withDatafile(datafile) + .withDefaultDecideOptions(options) + .build(); + + String flagKey = "feature_1"; + OptimizelyUserContext user = optimizely.createUserContext(userId); + + // should be excluded by DefaultDecideOption + OptimizelyDecision decision = user.decide(flagKey); + assertTrue(decision.getVariables().toMap().size() == 0); + + decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS, OptimizelyDecideOption.EXCLUDE_VARIABLES}); + // other options should work as well + assertTrue(decision.getReasons().size() > 0); + // redundant setting ignored + assertTrue(decision.getVariables().toMap().size() == 0); } // errors @@ -447,4 +648,21 @@ public void decideReasons_userNotBucketedIntoAnyExperimentInGroup() {} public void decideReasons_userBucketedIntoInvalidExperiment() {} @Test public void decideReasons_userNotInExperiment() {} + + // utils + + Map createUserProfileMap(String experimentId, String variationId) { + Map userProfileMap = new HashMap(); + userProfileMap.put(UserProfileService.userIdKey, userId); + + Map decisionMap = new HashMap(1); + decisionMap.put(UserProfileService.variationIdKey, variationId); + + Map> decisionsMap = new HashMap>(); + decisionsMap.put(experimentId, decisionMap); + userProfileMap.put(UserProfileService.experimentBucketMapKey, decisionsMap); + + return userProfileMap; + } + } diff --git a/core-api/src/test/resources/config/decide-project-config.json b/core-api/src/test/resources/config/decide-project-config.json index 28f45db01..d6b53bdc0 100644 --- a/core-api/src/test/resources/config/decide-project-config.json +++ b/core-api/src/test/resources/config/decide-project-config.json @@ -314,6 +314,10 @@ } ], "attributes": [ + { + "id": "10401066117", + "key": "gender" + }, { "id": "10401066170", "key": "testvar" From 77929269fc4f886ec447d7a24b1516f7e8fcd02e Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 14 Oct 2020 09:42:40 -0700 Subject: [PATCH 09/17] add more tests --- .../OptimizelyUserContextTest.java | 172 +++++++++++++++--- 1 file changed, 151 insertions(+), 21 deletions(-) diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index 18d3730c3..41cdd3f70 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -21,6 +21,8 @@ import com.optimizely.ab.EventHandlerRule; import com.optimizely.ab.Optimizely; import com.optimizely.ab.bucketing.UserProfileService; +import com.optimizely.ab.config.Experiment; +import com.optimizely.ab.config.parser.ConfigParseException; import com.optimizely.ab.event.ForwardingEventProcessor; import com.optimizely.ab.notification.NotificationCenter; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; @@ -29,10 +31,7 @@ import org.junit.Rule; import org.junit.Test; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import static com.optimizely.ab.config.ValidProjectConfigV4.ATTRIBUTE_HOUSE_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.AUDIENCE_GRYFFINDOR_VALUE; @@ -594,7 +593,9 @@ public void decideReasons_variableValueInvalid() { // reasons (logs with includeReasons) @Test - public void decideReasons_conditionNoMatchingAudience() {} + public void decideReasons_conditionNoMatchingAudience() throws ConfigParseException { + } + @Test public void decideReasons_conditionInvalidFormat() {} @Test @@ -616,38 +617,159 @@ public void decideReasons_missingAttributeValue() {} @Test public void decideReasons_experimentNotRunning() {} + @Test - public void decideReasons_gotVariationFromUserProfile() {} + public void decideReasons_gotVariationFromUserProfile() throws Exception { + String flagKey = "feature_2"; // embedding experiment: "exp_no_audience" + String experimentId = "10420810910"; // "exp_no_audience" + String experimentKey = "exp_no_audience"; + String variationId2 = "10418510624"; + String variationKey2 = "variation_no_traffic"; + + UserProfileService ups = mock(UserProfileService.class); + when(ups.lookup(userId)).thenReturn(createUserProfileMap(experimentId, variationId2)); + + optimizely = new Optimizely.Builder() + .withDatafile(datafile) + .withUserProfileService(ups) + .build(); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format( + "Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", variationKey2, experimentKey, userId) + )); + } + @Test - public void decideReasons_forcedVariationFound() {} + public void decideReasons_forcedVariationFound() { + + } + @Test - public void decideReasons_forcedVariationFoundButInvalid() {} + public void decideReasons_forcedVariationFoundButInvalid() { + + } + @Test - public void decideReasons_userMeetsConditionsForTargetingRule() {} + public void decideReasons_userMeetsConditionsForTargetingRule() { + String flagKey = "feature_1"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.setAttribute("country", "US"); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format( + "The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, flagKey) + )); + } + @Test - public void decideReasons_userDoesntMeetConditionsForTargetingRule() {} + public void decideReasons_userDoesntMeetConditionsForTargetingRule() { + String flagKey = "feature_1"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.setAttribute("country", "CA"); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("User \"%s\" does not meet conditions for targeting rule \"%d\".", userId, 1) + )); + } + @Test - public void decideReasons_userBucketedIntoTargetingRule() {} + public void decideReasons_userBucketedIntoTargetingRule() { + String flagKey = "feature_1"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.setAttribute("country", "US"); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, flagKey) + )); + } + @Test - public void decideReasons_userBucketedIntoEveryoneTargetingRule() {} + public void decideReasons_userBucketedIntoEveryoneTargetingRule() { + String flagKey = "feature_1"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.setAttribute("country", "KO"); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("User \"%s\" meets conditions for targeting rule \"Everyone Else\".", userId) + )); + } + @Test - public void decideReasons_userNotBucketedIntoTargetingRule() {} + public void decideReasons_userNotBucketedIntoTargetingRule() { + String flagKey = "feature_1"; + String experimentKey = "3332020494"; // experimentKey of rollout[2] + + OptimizelyUserContext user = optimizely.createUserContext(userId); + user.setAttribute("browser", "safari"); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", userId, experimentKey) + )); + } + @Test - public void decideReasons_userBucketedIntoVariationInExperiment() {} + public void decideReasons_userBucketedIntoVariationInExperiment() { + String flagKey = "feature_2"; + String experimentKey = "exp_no_audience"; + String variationKey = "variation_with_traffic"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"%s\" is in variation \"%s\" of experiment \"%s\".", userId, variationKey, experimentKey) + )); + } + @Test - public void decideReasons_userNotBucketedIntoVariation() {} + public void decideReasons_userNotBucketedIntoVariation() { + } + @Test - public void decideReasons_userBucketedIntoInvalidVariation() {} + public void decideReasons_userBucketedIntoInvalidVariation() { + } + @Test - public void decideReasons_userBucketedIntoExperimentInGroup() {} + public void decideReasons_userBucketedIntoExperimentInGroup() { + + } @Test - public void decideReasons_userNotBucketedIntoExperimentInGroup() {} + public void decideReasons_userNotBucketedIntoExperimentInGroup() { + + } @Test - public void decideReasons_userNotBucketedIntoAnyExperimentInGroup() {} + public void decideReasons_userNotBucketedIntoAnyExperimentInGroup() { + + } @Test - public void decideReasons_userBucketedIntoInvalidExperiment() {} + public void decideReasons_userBucketedIntoInvalidExperiment() { + + } @Test - public void decideReasons_userNotInExperiment() {} + public void decideReasons_userNotInExperiment() { + String flagKey = "feature_1"; + String experimentKey = "exp_with_audience"; + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + + assertTrue(decision.getReasons().contains( + String.format("User \"%s\" does not meet conditions to be in experiment \"%s\".", userId, experimentKey) + )); + } // utils @@ -665,4 +787,12 @@ Map createUserProfileMap(String experimentId, String variationId return userProfileMap; } + void setAudienceForFeatureTest(String featureKey, String audienceId) throws ConfigParseException { + String experimentId = optimizely.getProjectConfig().getFeatureKeyMapping().get(featureKey).getExperimentIds().get(0); + Experiment experimentReal = optimizely.getProjectConfig().getExperimentIdMapping().get(experimentId); + + Experiment experiment = spy(experimentReal); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + } + } From 4bfa1f7711136ce696c8379a55d41e5d1c166edb Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Wed, 14 Oct 2020 15:17:41 -0700 Subject: [PATCH 10/17] fix conflicts --- .../OptimizelyUserContext.java | 10 ++-- .../OptimizelyUserContextTest.java | 55 +++++++++---------- 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java index 957f4d962..ef7cc57ae 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java @@ -84,7 +84,7 @@ public void setAttribute(@Nonnull String key, @Nonnull Object value) { *
      • If the SDK finds an error, it’ll return a decision with null for variationKey. The decision will include an error message in reasons. *
      * @param key A flag key for which a decision will be made. - * @param options An array of options for decision-making. + * @param options A list of options for decision-making. * @return A decision result. */ public OptimizelyDecision decide(@Nonnull String key, @@ -193,8 +193,8 @@ public OptimizelyDecision decide(String key) { *
    • If the SDK finds an error for a key, the response will include a decision for the key showing reasons for the error. *
    • The SDK will always return key-mapped decisions. When it can not process requests, it’ll return an empty map after logging the errors. *
    - * @param keys An array of flag keys for which decisions will be made. - * @param options An array of options for decision-making. + * @param keys A list of flag keys for which decisions will be made. + * @param options A list of options for decision-making. * @return All decision results mapped by flag keys. */ public Map decideForKeys(@Nonnull List keys, @@ -224,7 +224,7 @@ public Map decideForKeys(@Nonnull List keys, /** * Returns a key-map of decision results for multiple flag keys and a user context. * - * @param keys An array of flag keys for which decisions will be made. + * @param keys A list of flag keys for which decisions will be made. * @return All decision results mapped by flag keys. */ public Map decideForKeys(@Nonnull List keys) { @@ -234,7 +234,7 @@ public Map decideForKeys(@Nonnull List keys) /** * Returns a key-map of decision results ({@link OptimizelyDecision}) for all active flag keys. * - * @param options An array of options for decision-making. + * @param options A list of options for decision-making. * @return All decision results mapped by flag keys. */ public Map decideAll(@Nonnull List options) { diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index 41cdd3f70..c207c5fe9 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -139,11 +139,11 @@ public void decide() { @Test public void decideAll_oneFlag() { String flagKey = "feature_2"; - String[] flagKeys = {flagKey}; + List flagKeys = Arrays.asList(flagKey); OptimizelyJSON variablesExpected = optimizely.getAllFeatureVariables(flagKey, userId); OptimizelyUserContext user = optimizely.createUserContext(userId); - Map decisions = user.decideAll(flagKeys); + Map decisions = user.decideForKeys(flagKeys); assertTrue(decisions.size() == 1); OptimizelyDecision decision = decisions.get(flagKey); @@ -164,12 +164,12 @@ public void decideAll_twoFlags() { String flagKey1 = "feature_1"; String flagKey2 = "feature_2"; - String[] flagKeys = {flagKey1, flagKey2}; + List flagKeys = Arrays.asList(flagKey1, flagKey2); OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); OptimizelyJSON variablesExpected2 = optimizely.getAllFeatureVariables(flagKey2, userId); OptimizelyUserContext user = optimizely.createUserContext(userId, Collections.singletonMap("gender", "f")); - Map decisions = user.decideAll(flagKeys); + Map decisions = user.decideForKeys(flagKeys); assertTrue(decisions.size() == 2); @@ -246,8 +246,7 @@ public void decideAll_allFlags_enabledFlagsOnly() { OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); OptimizelyUserContext user = optimizely.createUserContext(userId, Collections.singletonMap("gender", "f")); - OptimizelyDecideOption[] decideOptions = {OptimizelyDecideOption.ENABLED_FLAGS_ONLY}; - Map decisions = user.decideAll(decideOptions); + Map decisions = user.decideAll(Arrays.asList(OptimizelyDecideOption.ENABLED_FLAGS_ONLY)); assertTrue(decisions.size() == 2); @@ -342,7 +341,7 @@ public void decide_doNotSendEvent() { String flagKey = "feature_2"; OptimizelyUserContext user = optimizely.createUserContext(userId); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.DISABLE_DECISION_EVENT}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.DISABLE_DECISION_EVENT)); assertEquals(decision.getVariationKey(), "variation_with_traffic"); } @@ -385,7 +384,7 @@ public void decisionNotification() { isListenerCalled = false; testDecisionInfoMap.put(DECISION_EVENT_DISPATCHED, false); - user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.DISABLE_DECISION_EVENT}); + user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.DISABLE_DECISION_EVENT)); assertTrue(isListenerCalled); } @@ -413,7 +412,7 @@ public void decideOptions_bypassUPS() throws Exception { // should return variationId2 set by UPS assertEquals(decision.getVariationKey(), variationKey2); - decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE}); + decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)); // should ignore variationId2 set by UPS and return variationId1 assertEquals(decision.getVariationKey(), variationKey1); // also should not save either @@ -428,7 +427,7 @@ public void decideOptions_excludeVariables() { OptimizelyDecision decision = user.decide(flagKey); assertTrue(decision.getVariables().toMap().size() > 0); - decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.EXCLUDE_VARIABLES}); + decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.EXCLUDE_VARIABLES)); assertTrue(decision.getVariables().toMap().size() == 0); } @@ -441,7 +440,7 @@ public void decideOptions_includeReasons() { assertEquals(decision.getReasons().size(), 1); assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); - decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertEquals(decision.getReasons().size(), 1); assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); @@ -449,7 +448,7 @@ public void decideOptions_includeReasons() { decision = user.decide(flagKey); assertEquals(decision.getReasons().size(), 0); - decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().size() > 0); } @@ -463,9 +462,9 @@ public void decideOptions_enabledFlagsOnly() { @Test public void decideOptions_defaultDecideOptions() { - OptimizelyDecideOption[] options = { + List options = Arrays.asList( OptimizelyDecideOption.EXCLUDE_VARIABLES - }; + ); optimizely = Optimizely.builder() .withDatafile(datafile) @@ -479,7 +478,7 @@ public void decideOptions_defaultDecideOptions() { OptimizelyDecision decision = user.decide(flagKey); assertTrue(decision.getVariables().toMap().size() == 0); - decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS, OptimizelyDecideOption.EXCLUDE_VARIABLES}); + decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS, OptimizelyDecideOption.EXCLUDE_VARIABLES)); // other options should work as well assertTrue(decision.getReasons().size() > 0); // redundant setting ignored @@ -521,11 +520,11 @@ public void decide_invalidFeatureKey() { @Test public void decideAll_sdkNotReady() { - String[] flagKeys = {"feature_1"}; + List flagKeys = Arrays.asList("feature_1"); Optimizely optimizely = new Optimizely.Builder().build(); OptimizelyUserContext user = optimizely.createUserContext(userId); - Map decisions = user.decideAll(flagKeys); + Map decisions = user.decideForKeys(flagKeys); assertEquals(decisions.size(), 0); } @@ -535,11 +534,11 @@ public void decideAll_errorDecisionIncluded() { String flagKey1 = "feature_2"; String flagKey2 = "invalid_key"; - String[] flagKeys = {flagKey1, flagKey2}; + List flagKeys = Arrays.asList(flagKey1, flagKey2); OptimizelyJSON variablesExpected1 = optimizely.getAllFeatureVariables(flagKey1, userId); OptimizelyUserContext user = optimizely.createUserContext(userId); - Map decisions = user.decideAll(flagKeys); + Map decisions = user.decideForKeys(flagKeys); assertEquals(decisions.size(), 2); @@ -617,7 +616,7 @@ public void decideReasons_missingAttributeValue() {} @Test public void decideReasons_experimentNotRunning() {} - + @Test public void decideReasons_gotVariationFromUserProfile() throws Exception { String flagKey = "feature_2"; // embedding experiment: "exp_no_audience" @@ -635,7 +634,7 @@ public void decideReasons_gotVariationFromUserProfile() throws Exception { .build(); OptimizelyUserContext user = optimizely.createUserContext(userId); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format( @@ -659,7 +658,7 @@ public void decideReasons_userMeetsConditionsForTargetingRule() { OptimizelyUserContext user = optimizely.createUserContext(userId); user.setAttribute("country", "US"); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format( @@ -673,7 +672,7 @@ public void decideReasons_userDoesntMeetConditionsForTargetingRule() { OptimizelyUserContext user = optimizely.createUserContext(userId); user.setAttribute("country", "CA"); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("User \"%s\" does not meet conditions for targeting rule \"%d\".", userId, 1) @@ -686,7 +685,7 @@ public void decideReasons_userBucketedIntoTargetingRule() { OptimizelyUserContext user = optimizely.createUserContext(userId); user.setAttribute("country", "US"); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, flagKey) @@ -699,7 +698,7 @@ public void decideReasons_userBucketedIntoEveryoneTargetingRule() { OptimizelyUserContext user = optimizely.createUserContext(userId); user.setAttribute("country", "KO"); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("User \"%s\" meets conditions for targeting rule \"Everyone Else\".", userId) @@ -713,7 +712,7 @@ public void decideReasons_userNotBucketedIntoTargetingRule() { OptimizelyUserContext user = optimizely.createUserContext(userId); user.setAttribute("browser", "safari"); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", userId, experimentKey) @@ -727,7 +726,7 @@ public void decideReasons_userBucketedIntoVariationInExperiment() { String variationKey = "variation_with_traffic"; OptimizelyUserContext user = optimizely.createUserContext(userId); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("User with bucketingId \"%s\" is in variation \"%s\" of experiment \"%s\".", userId, variationKey, experimentKey) @@ -764,7 +763,7 @@ public void decideReasons_userNotInExperiment() { String experimentKey = "exp_with_audience"; OptimizelyUserContext user = optimizely.createUserContext(userId); - OptimizelyDecision decision = user.decide(flagKey, new OptimizelyDecideOption[]{OptimizelyDecideOption.INCLUDE_REASONS}); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( String.format("User \"%s\" does not meet conditions to be in experiment \"%s\".", userId, experimentKey) From fdbf75a9a4d102399a0f1e369e1a7d5488fb2dbb Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 15 Oct 2020 09:03:54 -0700 Subject: [PATCH 11/17] fix to nonnull options and reasons --- .../java/com/optimizely/ab/Optimizely.java | 17 +- .../OptimizelyUserContext.java | 25 +-- .../com/optimizely/ab/bucketing/Bucketer.java | 38 +++-- .../ab/bucketing/DecisionService.java | 147 +++++++++--------- .../ab/internal/ExperimentUtils.java | 42 ++--- .../DecisionReasons.java | 19 ++- .../OptimizelyDecideOption.java | 2 +- .../OptimizelyDecision.java | 3 +- .../optimizely/ab/OptimizelyBuilderTest.java | 2 +- .../com/optimizely/ab/OptimizelyTest.java | 21 ++- .../ab/bucketing/DecisionServiceTest.java | 12 +- .../OptimizelyDecisionTest.java | 2 + .../OptimizelyUserContextTest.java | 3 + 13 files changed, 177 insertions(+), 156 deletions(-) rename core-api/src/main/java/com/optimizely/ab/{optimizelyusercontext => }/OptimizelyUserContext.java (94%) rename core-api/src/main/java/com/optimizely/ab/{optimizelyusercontext => optimizelydecision}/DecisionReasons.java (72%) rename core-api/src/main/java/com/optimizely/ab/{optimizelyusercontext => optimizelydecision}/OptimizelyDecideOption.java (94%) rename core-api/src/main/java/com/optimizely/ab/{optimizelyusercontext => optimizelydecision}/OptimizelyDecision.java (97%) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 78762377c..26fe4a6b9 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -35,8 +35,7 @@ import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager; import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; -import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; -import com.optimizely.ab.optimizelyusercontext.OptimizelyUserContext; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -78,7 +77,7 @@ public class Optimizely implements AutoCloseable { private static final Logger logger = LoggerFactory.getLogger(Optimizely.class); - public final DecisionService decisionService; + final DecisionService decisionService; @VisibleForTesting @Deprecated final EventHandler eventHandler; @@ -226,11 +225,11 @@ private Variation activate(@Nullable ProjectConfig projectConfig, return variation; } - public void sendImpression(@Nonnull ProjectConfig projectConfig, - @Nonnull Experiment experiment, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull Variation variation) { + void sendImpression(@Nonnull ProjectConfig projectConfig, + @Nonnull Experiment experiment, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull Variation variation) { if (!experiment.isRunning()) { logger.info("Experiment has \"Launched\" status so not dispatching event during activation."); return; @@ -741,7 +740,7 @@ T getFeatureVariableValueForType(@Nonnull String featureKey, } // Helper method which takes type and variable value and convert it to object to use in Listener DecisionInfo object variable value - public Object convertStringToType(String variableValue, String type) { + Object convertStringToType(String variableValue, String type) { if (variableValue != null) { switch (type) { case FeatureVariable.DOUBLE_TYPE: diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java similarity index 94% rename from core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java rename to core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java index ef7cc57ae..36c87512f 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java @@ -14,14 +14,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab; -import com.optimizely.ab.Optimizely; -import com.optimizely.ab.UnknownEventTypeException; import com.optimizely.ab.bucketing.FeatureDecision; import com.optimizely.ab.config.*; import com.optimizely.ab.notification.DecisionNotification; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -100,11 +101,11 @@ public OptimizelyDecision decide(@Nonnull String key, return OptimizelyDecision.createErrorDecision(key, this, getFlagKeyInvalidMessage(key)); } - List allOptions = getAllOptions(options); Boolean sentEvent = false; Boolean flagEnabled = false; - DecisionReasons decisionReasons = new DecisionReasons(); + List allOptions = getAllOptions(options); Boolean includeReasons = allOptions.contains(OptimizelyDecideOption.INCLUDE_REASONS); + DecisionReasons decisionReasons = new DecisionReasons(includeReasons); Map copiedAttributes = copyAttributes(); FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( @@ -113,7 +114,7 @@ public OptimizelyDecision decide(@Nonnull String key, copiedAttributes, projectConfig, allOptions, - includeReasons ? decisionReasons : null); + decisionReasons); if (flagDecision.variation != null) { if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { @@ -142,12 +143,12 @@ public OptimizelyDecision decide(@Nonnull String key, flag, flagDecision.variation, flagEnabled, - includeReasons ? decisionReasons : null); + decisionReasons); } OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); - List reasonsToReport = decisionReasons.toReport(allOptions); + List reasonsToReport = decisionReasons.toReport(); String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; // TODO: add ruleKey values when available later. use a copy of experimentKey until then. String ruleKey = flagDecision.experiment != null ? flagDecision.experiment.getKey() : null; @@ -304,10 +305,10 @@ public static String getVariableValueInvalidMessage(String variableKey) { return String.format(VARIABLE_VALUE_INVALID, variableKey); } - private Map getDecisionVariableMap(FeatureFlag flag, - Variation variation, - Boolean featureEnabled, - DecisionReasons decisionReasons) { + private Map getDecisionVariableMap(@Nonnull FeatureFlag flag, + @Nonnull Variation variation, + @Nonnull Boolean featureEnabled, + @Nonnull DecisionReasons decisionReasons) { Map valuesMap = new HashMap(); for (FeatureVariable variable : flag.getVariables()) { String value = variable.getDefaultValue(); diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java index b45abc203..7b0aa66eb 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java @@ -19,14 +19,15 @@ import com.optimizely.ab.annotations.VisibleForTesting; import com.optimizely.ab.bucketing.internal.MurmurHash3; import com.optimizely.ab.config.*; -import com.optimizely.ab.optimizelyusercontext.DecisionReasons; -import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; +import java.util.Collections; import java.util.List; /** @@ -70,8 +71,8 @@ private String bucketToEntity(int bucketValue, List trafficAl private Experiment bucketToExperiment(@Nonnull Group group, @Nonnull String bucketingId, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // "salt" the bucket id using the group id String bucketKey = bucketingId + group.getId(); @@ -92,8 +93,8 @@ private Experiment bucketToExperiment(@Nonnull Group group, private Variation bucketToVariation(@Nonnull Experiment experiment, @Nonnull String bucketingId, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // "salt" the bucket id using the experiment id String experimentId = experiment.getId(); String experimentKey = experiment.getKey(); @@ -109,14 +110,16 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, if (bucketedVariationId != null) { Variation bucketedVariation = experiment.getVariationIdToVariationMap().get(bucketedVariationId); String variationKey = bucketedVariation.getKey(); - DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is in variation \"%s\" of experiment \"%s\".", bucketingId, variationKey, + String message = reasons.addInfoF("User with bucketingId \"%s\" is in variation \"%s\" of experiment \"%s\".", bucketingId, variationKey, experimentKey); + logger.info(message); return bucketedVariation; } // user was not bucketed to a variation - DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", bucketingId, experimentKey); + String message = reasons.addInfoF("User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", bucketingId, experimentKey); + logger.info(message); return null; } @@ -134,8 +137,8 @@ private Variation bucketToVariation(@Nonnull Experiment experiment, public Variation bucket(@Nonnull Experiment experiment, @Nonnull String bucketingId, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // support existing custom Bucketers if (isMethodOverriden("bucket", Experiment.class, String.class, ProjectConfig.class)) { @@ -157,15 +160,15 @@ public Variation bucket(@Nonnull Experiment experiment, public Variation bucket(@Nonnull Experiment experiment, @Nonnull String bucketingId, @Nonnull ProjectConfig projectConfig) { - return bucketCore(experiment, bucketingId, projectConfig, null, null); + return bucketCore(experiment, bucketingId, projectConfig, Collections.emptyList(), new DecisionReasons()); } @Nullable public Variation bucketCore(@Nonnull Experiment experiment, @Nonnull String bucketingId, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // ---------- Bucket User ---------- String groupId = experiment.getGroupId(); // check whether the experiment belongs to a group @@ -175,7 +178,8 @@ public Variation bucketCore(@Nonnull Experiment experiment, if (experimentGroup.getPolicy().equals(Group.RANDOM_POLICY)) { Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig, options, reasons); if (bucketedExperiment == null) { - DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId()); + String message = reasons.addInfoF("User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId()); + logger.info(message); return null; } else { @@ -183,13 +187,15 @@ public Variation bucketCore(@Nonnull Experiment experiment, // if the experiment a user is bucketed in within a group isn't the same as the experiment provided, // don't perform further bucketing within the experiment if (!bucketedExperiment.getId().equals(experiment.getId())) { - DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), + String message = reasons.addInfoF("User with bucketingId \"%s\" is not in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), experimentGroup.getId()); + logger.info(message); return null; } - DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), + String message = reasons.addInfoF("User with bucketingId \"%s\" is in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(), experimentGroup.getId()); + logger.info(message); } } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index 588df9cf3..bc701326b 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -20,13 +20,14 @@ import com.optimizely.ab.error.ErrorHandler; import com.optimizely.ab.internal.ControlAttribute; import com.optimizely.ab.internal.ExperimentUtils; -import com.optimizely.ab.optimizelyusercontext.DecisionReasons; -import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -92,8 +93,8 @@ public Variation getVariation(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // support existing custom DecisionServices if (isMethodOverriden("getVariation", Experiment.class, String.class, Map.class, ProjectConfig.class)) { @@ -108,7 +109,7 @@ public Variation getVariation(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig) { - return getVariationCore(experiment, userId, filteredAttributes, projectConfig, null, null); + return getVariationCore(experiment, userId, filteredAttributes, projectConfig, Collections.emptyList(), new DecisionReasons()); } @Nullable @@ -116,8 +117,8 @@ public Variation getVariationCore(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { if (!ExperimentUtils.isExperimentActive(experiment, options, reasons)) { return null; } @@ -141,14 +142,17 @@ public Variation getVariationCore(@Nonnull Experiment experiment, try { Map userProfileMap = userProfileService.lookup(userId); if (userProfileMap == null) { - logInfo(logger, reasons, "We were unable to get a user profile map from the UserProfileService."); + String message = reasons.addInfoF("We were unable to get a user profile map from the UserProfileService."); + logger.info(message); } else if (UserProfileUtils.isValidUserProfileMap(userProfileMap)) { userProfile = UserProfileUtils.convertMapToUserProfile(userProfileMap); } else { - logWarn(logger, reasons, "The UserProfileService returned an invalid map."); + String message = reasons.addInfoF("The UserProfileService returned an invalid map."); + logger.warn(message); } } catch (Exception exception) { - logError(logger, reasons, exception.getMessage()); + String message = reasons.addInfoF(exception.getMessage()); + logger.error(message); errorHandler.handleError(new OptimizelyRuntimeException(exception)); } } @@ -179,7 +183,8 @@ public Variation getVariationCore(@Nonnull Experiment experiment, return variation; } - logInfo(logger, reasons, "User \"%s\" does not meet conditions to be in experiment \"%s\".", userId, experiment.getKey()); + String message = reasons.addInfoF("User \"%s\" does not meet conditions to be in experiment \"%s\".", userId, experiment.getKey()); + logger.info(message); return null; } @@ -199,8 +204,8 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // support existing custom DecisionServices if (isMethodOverriden("getVariationForFeature", FeatureFlag.class, String.class, Map.class, ProjectConfig.class)) { return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig); @@ -215,7 +220,7 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig) { - return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig,null, null); + return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig,Collections.emptyList(), new DecisionReasons()); } @Nonnull @@ -223,8 +228,8 @@ public FeatureDecision getVariationForFeatureCore(@Nonnull FeatureFlag featureFl @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { if (!featureFlag.getExperimentIds().isEmpty()) { for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); @@ -234,16 +239,19 @@ public FeatureDecision getVariationForFeatureCore(@Nonnull FeatureFlag featureFl } } } else { - logInfo(logger, reasons, "The feature flag \"%s\" is not used in any experiments.", featureFlag.getKey()); + String message = reasons.addInfoF("The feature flag \"%s\" is not used in any experiments.", featureFlag.getKey()); + logger.info(message); } FeatureDecision featureDecision = getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig, options, reasons); if (featureDecision.variation == null) { - logInfo(logger, reasons, "The user \"%s\" was not bucketed into a rollout for feature flag \"%s\".", + String message = reasons.addInfoF("The user \"%s\" was not bucketed into a rollout for feature flag \"%s\".", userId, featureFlag.getKey()); + logger.info(message); } else { - logInfo(logger, reasons, "The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", + String message = reasons.addInfoF("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, featureFlag.getKey()); + logger.info(message); } return featureDecision; } @@ -266,17 +274,19 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // use rollout to get variation for feature if (featureFlag.getRolloutId().isEmpty()) { - logInfo(logger, reasons, "The feature flag \"%s\" is not used in a rollout.", featureFlag.getKey()); + String message = reasons.addInfoF("The feature flag \"%s\" is not used in a rollout.", featureFlag.getKey()); + logger.info(message); return new FeatureDecision(null, null, null); } Rollout rollout = projectConfig.getRolloutIdMapping().get(featureFlag.getRolloutId()); if (rollout == null) { - logError(logger, reasons, "The rollout with id \"%s\" was not found in the datafile for feature flag \"%s\".", + String message = reasons.addInfoF("The rollout with id \"%s\" was not found in the datafile for feature flag \"%s\".", featureFlag.getRolloutId(), featureFlag.getKey()); + logger.error(message); return new FeatureDecision(null, null, null); } @@ -294,7 +304,8 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag return new FeatureDecision(rolloutRule, variation, FeatureDecision.DecisionSource.ROLLOUT); } else { - logDebug(logger, reasons, "User \"%s\" does not meet conditions for targeting rule \"%d\".", userId, i + 1); + String message = reasons.addInfoF("User \"%s\" does not meet conditions for targeting rule \"%d\".", userId, i + 1); + logger.debug(message); } } @@ -303,7 +314,8 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag if (ExperimentUtils.doesUserMeetAudienceConditions(projectConfig, finalRule, filteredAttributes, RULE, "Everyone Else", options, reasons)) { variation = bucketer.bucket(finalRule, bucketingId, projectConfig, options, reasons); if (variation != null) { - logDebug(logger, reasons, "User \"%s\" meets conditions for targeting rule \"Everyone Else\".", userId); + String message = reasons.addInfoF("User \"%s\" meets conditions for targeting rule \"Everyone Else\".", userId); + logger.debug(message); return new FeatureDecision(finalRule, variation, FeatureDecision.DecisionSource.ROLLOUT); } @@ -316,7 +328,7 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag @Nonnull String userId, @Nonnull Map filteredAttributes, @Nonnull ProjectConfig projectConfig) { - return getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig, null, null); + return getVariationForFeatureInRollout(featureFlag, userId, filteredAttributes, projectConfig, Collections.emptyList(), new DecisionReasons()); } /** @@ -332,18 +344,20 @@ FeatureDecision getVariationForFeatureInRollout(@Nonnull FeatureFlag featureFlag @Nullable Variation getWhitelistedVariation(@Nonnull Experiment experiment, @Nonnull String userId, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // if a user has a forced variation mapping, return the respective variation Map userIdToVariationKeyMap = experiment.getUserIdToVariationKeyMap(); if (userIdToVariationKeyMap.containsKey(userId)) { String forcedVariationKey = userIdToVariationKeyMap.get(userId); Variation forcedVariation = experiment.getVariationKeyToVariationMap().get(forcedVariationKey); if (forcedVariation != null) { - logInfo(logger, reasons, "User \"%s\" is forced in variation \"%s\".", userId, forcedVariationKey); + String message = reasons.addInfoF("User \"%s\" is forced in variation \"%s\".", userId, forcedVariationKey); + logger.info(message); } else { - logError(logger, reasons, "Variation \"%s\" is not in the datafile. Not activating user \"%s\".", + String message = reasons.addInfoF("Variation \"%s\" is not in the datafile. Not activating user \"%s\".", forcedVariationKey, userId); + logger.error(message); } return forcedVariation; } @@ -354,7 +368,7 @@ Variation getWhitelistedVariation(@Nonnull Experiment experiment, Variation getWhitelistedVariation(@Nonnull Experiment experiment, @Nonnull String userId) { - return getWhitelistedVariation(experiment, userId, null, null); + return getWhitelistedVariation(experiment, userId, Collections.emptyList(), new DecisionReasons()); } /** @@ -372,10 +386,10 @@ Variation getWhitelistedVariation(@Nonnull Experiment experiment, Variation getStoredVariation(@Nonnull Experiment experiment, @Nonnull UserProfile userProfile, @Nonnull ProjectConfig projectConfig, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { - if (options != null && options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return null; + if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return null; // ---------- Check User Profile for Sticky Bucketing ---------- // If a user profile instance is present then check it for a saved variation @@ -390,18 +404,21 @@ Variation getStoredVariation(@Nonnull Experiment experiment, .getVariationIdToVariationMap() .get(variationId); if (savedVariation != null) { - logInfo(logger, reasons,"Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", + String message = reasons.addInfoF("Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", savedVariation.getKey(), experimentKey, userProfile.userId); + logger.info(message); // A variation is stored for this combined bucket id return savedVariation; } else { - logInfo(logger, reasons,"User \"%s\" was previously bucketed into variation with ID \"%s\" for experiment \"%s\", but no matching variation was found for that user. We will re-bucket the user.", + String message = reasons.addInfoF("User \"%s\" was previously bucketed into variation with ID \"%s\" for experiment \"%s\", but no matching variation was found for that user. We will re-bucket the user.", userProfile.userId, variationId, experimentKey); + logger.info(message); return null; } } else { - logInfo(logger, reasons, "No previously activated variation of experiment \"%s\" for user \"%s\" found in user profile.", + String message = reasons.addInfoF("No previously activated variation of experiment \"%s\" for user \"%s\" found in user profile.", experimentKey, userProfile.userId); + logger.info(message); return null; } } @@ -410,7 +427,7 @@ Variation getStoredVariation(@Nonnull Experiment experiment, Variation getStoredVariation(@Nonnull Experiment experiment, @Nonnull UserProfile userProfile, @Nonnull ProjectConfig projectConfig) { - return getStoredVariation(experiment, userProfile, projectConfig, null, null); + return getStoredVariation(experiment, userProfile, projectConfig, Collections.emptyList(), new DecisionReasons()); } /** @@ -425,10 +442,10 @@ Variation getStoredVariation(@Nonnull Experiment experiment, void saveVariation(@Nonnull Experiment experiment, @Nonnull Variation variation, @Nonnull UserProfile userProfile, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { - if (options != null && options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return; + if (options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE)) return; // only save if the user has implemented a user profile service if (userProfileService != null) { @@ -458,7 +475,7 @@ void saveVariation(@Nonnull Experiment experiment, void saveVariation(@Nonnull Experiment experiment, @Nonnull Variation variation, @Nonnull UserProfile userProfile) { - saveVariation(experiment, variation, userProfile, null, null); + saveVariation(experiment, variation, userProfile, Collections.emptyList(), new DecisionReasons()); } /** @@ -473,15 +490,16 @@ void saveVariation(@Nonnull Experiment experiment, */ String getBucketingId(@Nonnull String userId, @Nonnull Map filteredAttributes, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { String bucketingId = userId; if (filteredAttributes != null && 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 { - logWarn(logger, reasons, "BucketingID attribute is not a string. Defaulted to userId"); + String message = reasons.addInfoF("BucketingID attribute is not a string. Defaulted to userId"); + logger.warn(message); } } return bucketingId; @@ -489,7 +507,7 @@ String getBucketingId(@Nonnull String userId, String getBucketingId(@Nonnull String userId, @Nonnull Map filteredAttributes) { - return getBucketingId(userId, filteredAttributes, null, null); + return getBucketingId(userId, filteredAttributes, Collections.emptyList(), new DecisionReasons()); } public ConcurrentHashMap> getForcedVariationMapping() { @@ -578,8 +596,8 @@ public boolean setForcedVariation(@Nonnull Experiment experiment, @Nullable public Variation getForcedVariation(@Nonnull Experiment experiment, @Nonnull String userId, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // support existing custom DecisionServices if (isMethodOverriden("getForcedVariation", Experiment.class, String.class)) { @@ -592,14 +610,14 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, @Nullable public Variation getForcedVariation(@Nonnull Experiment experiment, @Nonnull String userId) { - return getForcedVariationCore(experiment, userId, null, null); + return getForcedVariationCore(experiment, userId, Collections.emptyList(), new DecisionReasons()); } @Nullable public Variation getForcedVariationCore(@Nonnull Experiment experiment, @Nonnull String userId, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { // if the user id is invalid, return false. if (!validateUserId(userId)) { @@ -612,8 +630,9 @@ public Variation getForcedVariationCore(@Nonnull Experiment experiment, if (variationId != null) { Variation variation = experiment.getVariationIdToVariationMap().get(variationId); if (variation != null) { - logDebug(logger, reasons, "Variation \"%s\" is mapped to experiment \"%s\" and user \"%s\" in the forced variation map", + String message = reasons.addInfoF("Variation \"%s\" is mapped to experiment \"%s\" and user \"%s\" in the forced variation map", variation.getKey(), experiment.getKey(), userId); + logger.debug(message); return variation; } } else { @@ -638,30 +657,6 @@ private boolean validateUserId(String userId) { return true; } - public static void logError(Logger logger, DecisionReasons reasons, String format, Object... args) { - String message = String.format(format, args); - logger.error(message); - if (reasons != null) reasons.addInfo(message); - } - - public static void logWarn(Logger logger, DecisionReasons reasons, String format, Object... args) { - String message = String.format(format, args); - logger.warn(message); - if (reasons != null) reasons.addInfo(message); - } - - public static void logInfo(Logger logger, DecisionReasons reasons, String format, Object... args) { - String message = String.format(format, args); - logger.info(message); - if (reasons != null) reasons.addInfo(message); - } - - public static void logDebug(Logger logger, DecisionReasons reasons, String format, Object... args) { - String message = String.format(format, args); - logger.debug(message); - if (reasons != null) reasons.addInfo(message); - } - Boolean isMethodOverriden(String methodName, Class... params) { try { return getClass() != DecisionService.class && getClass().getDeclaredMethod(methodName, params) != null; diff --git a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java index 68628516d..e703cc527 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java @@ -16,20 +16,20 @@ */ package com.optimizely.ab.internal; -import com.optimizely.ab.bucketing.DecisionService; import com.optimizely.ab.config.Experiment; import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.audience.AudienceIdCondition; import com.optimizely.ab.config.audience.Condition; import com.optimizely.ab.config.audience.OrCondition; -import com.optimizely.ab.optimizelyusercontext.DecisionReasons; -import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -49,11 +49,12 @@ private ExperimentUtils() { * @return whether the pre-conditions are satisfied */ public static boolean isExperimentActive(@Nonnull Experiment experiment, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { if (!experiment.isActive()) { - DecisionService.logInfo(logger, reasons, "Experiment \"%s\" is not running.", experiment.getKey()); + String message = reasons.addInfoF("Experiment \"%s\" is not running.", experiment.getKey()); + logger.info(message); return false; } @@ -61,7 +62,7 @@ public static boolean isExperimentActive(@Nonnull Experiment experiment, } public static boolean isExperimentActive(@Nonnull Experiment experiment) { - return isExperimentActive(experiment, null, null); + return isExperimentActive(experiment, Collections.emptyList(), new DecisionReasons()); } /** @@ -81,8 +82,8 @@ public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig proj @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { if (experiment.getAudienceConditions() != null) { logger.debug("Evaluating audiences for {} \"{}\": {}.", loggingEntityType, loggingKey, experiment.getAudienceConditions()); Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey); @@ -108,7 +109,7 @@ public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig proj @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey) { - return doesUserMeetAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + return doesUserMeetAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, Collections.emptyList(), new DecisionReasons()); } @Nullable @@ -117,8 +118,8 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { List experimentAudienceIds = experiment.getAudienceIds(); // if there are no audiences, ALL users should be part of the experiment @@ -138,7 +139,8 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, Boolean result = implicitOr.evaluate(projectConfig, attributes); - DecisionService.logInfo(logger, reasons, "Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); + String message = reasons.addInfoF("Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); + logger.info(message); return result; } @@ -149,7 +151,7 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey) { - return evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + return evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey, Collections.emptyList(), new DecisionReasons()); } @Nullable @@ -158,18 +160,20 @@ public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectC @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey, - @Nullable List options, - @Nullable DecisionReasons reasons) { + @Nonnull List options, + @Nonnull DecisionReasons reasons) { Condition conditions = experiment.getAudienceConditions(); if (conditions == null) return null; try { Boolean result = conditions.evaluate(projectConfig, attributes); - DecisionService.logInfo(logger, reasons,"Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); + String message = reasons.addInfoF("Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); + logger.info(message); return result; } catch (Exception e) { - DecisionService.logError(logger, reasons,"Condition invalid: %s", e.getMessage()); + String message = reasons.addInfoF("Condition invalid: %s", e.getMessage()); + logger.error(message); return null; } } @@ -180,7 +184,7 @@ public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectC @Nonnull Map attributes, @Nonnull String loggingEntityType, @Nonnull String loggingKey) { - return evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, null, null); + return evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, Collections.emptyList(), new DecisionReasons()); } } \ No newline at end of file diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java similarity index 72% rename from core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java rename to core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java index 7230b7291..c7c7d91b6 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/DecisionReasons.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java @@ -14,21 +14,27 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab.optimizelydecision; import java.util.ArrayList; import java.util.List; public class DecisionReasons { + boolean includeReasons; List errors; List logs; - public DecisionReasons() { + public DecisionReasons(boolean includeReasons) { + this.includeReasons = includeReasons; this.errors = new ArrayList(); this.logs = new ArrayList(); } + public DecisionReasons() { + this(false); + } + public void addError(String message) { errors.add(message); } @@ -37,10 +43,15 @@ public void addInfo(String message) { logs.add(message); } + public String addInfoF(String format, Object... args) { + String message = String.format(format, args); + if(includeReasons) addInfo(message); + return message; + } - public List toReport(List options) { + public List toReport() { List reasons = new ArrayList<>(errors); - if(options.contains(OptimizelyDecideOption.INCLUDE_REASONS)) { + if(includeReasons) { reasons.addAll(logs); } return reasons; diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecideOption.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecideOption.java similarity index 94% rename from core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecideOption.java rename to core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecideOption.java index e7f6d6874..ccd08bb63 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecideOption.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecideOption.java @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab.optimizelydecision; public enum OptimizelyDecideOption { DISABLE_DECISION_EVENT, diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecision.java similarity index 97% rename from core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java rename to core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecision.java index 2eafd4001..77c05642f 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecision.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/OptimizelyDecision.java @@ -14,8 +14,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab.optimizelydecision; +import com.optimizely.ab.OptimizelyUserContext; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import javax.annotation.Nonnull; diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java index 7e6579658..932150337 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java @@ -21,7 +21,7 @@ import com.optimizely.ab.error.ErrorHandler; import com.optimizely.ab.error.NoOpErrorHandler; import com.optimizely.ab.event.EventHandler; -import com.optimizely.ab.optimizelyusercontext.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.junit.Rule; import org.junit.Test; diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index d77d54875..2e3cedc88 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -35,7 +35,6 @@ import com.optimizely.ab.internal.LogbackVerifier; import com.optimizely.ab.notification.*; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; -import com.optimizely.ab.optimizelyusercontext.OptimizelyUserContext; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.junit.Before; import org.junit.Rule; @@ -378,7 +377,7 @@ public void activateForNullVariation() throws Exception { Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); Map testUserAttributes = Collections.singletonMap("browser_type", "chromey"); - when(mockBucketer.bucket(activatedExperiment, testBucketingId, validProjectConfig, null, null)).thenReturn(null); + when(mockBucketer.bucket(eq(activatedExperiment), eq(testBucketingId), eq(validProjectConfig), anyObject(), anyObject())).thenReturn(null); logbackVerifier.expectMessage(Level.INFO, "Not activating user \"userId\" for experiment \"" + activatedExperiment.getKey() + "\"."); @@ -1255,7 +1254,7 @@ public void getVariation() throws Exception { Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); - when(mockBucketer.bucket(activatedExperiment, testUserId, validProjectConfig, null, null)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(eq(activatedExperiment), eq(testUserId), eq(validProjectConfig), anyObject(), anyObject())).thenReturn(bucketedVariation); Map testUserAttributes = new HashMap<>(); testUserAttributes.put("browser_type", "chrome"); @@ -1265,7 +1264,7 @@ public void getVariation() throws Exception { testUserAttributes); // verify that the bucketing algorithm was called correctly - verify(mockBucketer).bucket(activatedExperiment, testUserId, validProjectConfig, null, null); + verify(mockBucketer).bucket(eq(activatedExperiment), eq(testUserId), eq(validProjectConfig), anyObject(), anyObject()); assertThat(actualVariation, is(bucketedVariation)); // verify that we didn't attempt to dispatch an event @@ -1286,13 +1285,13 @@ public void getVariationWithExperimentKey() throws Exception { .withConfig(noAudienceProjectConfig) .build(); - when(mockBucketer.bucket(activatedExperiment, testUserId, noAudienceProjectConfig, null, null)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(eq(activatedExperiment), eq(testUserId), eq(noAudienceProjectConfig), anyObject(), anyObject())).thenReturn(bucketedVariation); // activate the experiment Variation actualVariation = optimizely.getVariation(activatedExperiment.getKey(), testUserId); // verify that the bucketing algorithm was called correctly - verify(mockBucketer).bucket(activatedExperiment, testUserId, noAudienceProjectConfig, null, null); + verify(mockBucketer).bucket(eq(activatedExperiment), eq(testUserId), eq(noAudienceProjectConfig), anyObject(), anyObject()); assertThat(actualVariation, is(bucketedVariation)); // verify that we didn't attempt to dispatch an event @@ -1347,7 +1346,7 @@ public void getVariationWithAudiences() throws Exception { Experiment experiment = validProjectConfig.getExperiments().get(0); Variation bucketedVariation = experiment.getVariations().get(0); - when(mockBucketer.bucket(experiment, testUserId, validProjectConfig, null, null)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(eq(experiment), eq(testUserId), eq(validProjectConfig), anyObject(), anyObject())).thenReturn(bucketedVariation); Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); @@ -1356,7 +1355,7 @@ public void getVariationWithAudiences() throws Exception { Variation actualVariation = optimizely.getVariation(experiment.getKey(), testUserId, testUserAttributes); - verify(mockBucketer).bucket(experiment, testUserId, validProjectConfig, null, null); + verify(mockBucketer).bucket(eq(experiment), eq(testUserId), eq(validProjectConfig), anyObject(), anyObject()); assertThat(actualVariation, is(bucketedVariation)); } @@ -1397,7 +1396,7 @@ public void getVariationNoAudiences() throws Exception { Experiment experiment = noAudienceProjectConfig.getExperiments().get(0); Variation bucketedVariation = experiment.getVariations().get(0); - when(mockBucketer.bucket(experiment, testUserId, noAudienceProjectConfig, null, null)).thenReturn(bucketedVariation); + when(mockBucketer.bucket(eq(experiment), eq(testUserId), eq(noAudienceProjectConfig), anyObject(), anyObject())).thenReturn(bucketedVariation); Optimizely optimizely = optimizelyBuilder .withConfig(noAudienceProjectConfig) @@ -1406,7 +1405,7 @@ public void getVariationNoAudiences() throws Exception { Variation actualVariation = optimizely.getVariation(experiment.getKey(), testUserId); - verify(mockBucketer).bucket(experiment, testUserId, noAudienceProjectConfig, null, null); + verify(mockBucketer).bucket(eq(experiment), eq(testUserId), eq(noAudienceProjectConfig), anyObject(), anyObject()); assertThat(actualVariation, is(bucketedVariation)); } @@ -1464,7 +1463,7 @@ public void getVariationForGroupExperimentWithMatchingAttributes() throws Except attributes.put("browser_type", "chrome"); } - when(mockBucketer.bucket(experiment,"user", validProjectConfig, null, null)).thenReturn(variation); + when(mockBucketer.bucket(eq(experiment), eq("user"), eq(validProjectConfig), anyObject(), anyObject())).thenReturn(variation); Optimizely optimizely = optimizelyBuilder.withBucketing(mockBucketer).build(); diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index c6d2b4a1c..9c7eae98a 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -95,7 +95,7 @@ public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { // no attributes provided for a experiment that has an audience assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap(), validProjectConfig), is(expectedVariation)); - verify(decisionService).getWhitelistedVariation(experiment, whitelistedUserId, null, null); + verify(decisionService).getWhitelistedVariation(eq(experiment), eq(whitelistedUserId), anyObject(), anyObject()); verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class), any(ProjectConfig.class), anyObject(), anyObject()); } @@ -901,7 +901,7 @@ public void getVariationSavesBucketedVariationIntoUserProfile() throws Exception Collections.singletonMap(experiment.getId(), decision)); Bucketer mockBucketer = mock(Bucketer.class); - when(mockBucketer.bucket(experiment, userProfileId, noAudienceProjectConfig, null, null)).thenReturn(variation); + when(mockBucketer.bucket(eq(experiment), eq(userProfileId), eq(noAudienceProjectConfig), anyObject(), anyObject())).thenReturn(variation); DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, userProfileService); @@ -963,7 +963,7 @@ public void getVariationSavesANewUserProfile() throws Exception { UserProfileService userProfileService = mock(UserProfileService.class); DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, userProfileService); - when(bucketer.bucket(experiment, userProfileId, noAudienceProjectConfig, null, null)).thenReturn(variation); + when(bucketer.bucket(eq(experiment), eq(userProfileId), eq(noAudienceProjectConfig), anyObject(), anyObject())).thenReturn(variation); when(userProfileService.lookup(userProfileId)).thenReturn(null); assertEquals(variation, decisionService.getVariation(experiment, userProfileId, Collections.emptyMap(), noAudienceProjectConfig)); @@ -977,7 +977,7 @@ public void getVariationBucketingId() throws Exception { Experiment experiment = validProjectConfig.getExperiments().get(0); Variation expectedVariation = experiment.getVariations().get(0); - when(bucketer.bucket(experiment, "bucketId", validProjectConfig, null, null)).thenReturn(expectedVariation); + when(bucketer.bucket(eq(experiment), eq("bucketId"), eq(validProjectConfig), anyObject(), anyObject())).thenReturn(expectedVariation); Map attr = new HashMap(); attr.put(ControlAttribute.BUCKETING_ATTRIBUTE.toString(), "bucketId"); @@ -1001,8 +1001,8 @@ public void getVariationForRolloutWithBucketingId() { attributes.put(ControlAttribute.BUCKETING_ATTRIBUTE.toString(), bucketingId); Bucketer bucketer = mock(Bucketer.class); - when(bucketer.bucket(rolloutRuleExperiment, userId, v4ProjectConfig, null, null)).thenReturn(null); - when(bucketer.bucket(rolloutRuleExperiment, bucketingId, v4ProjectConfig, null, null)).thenReturn(rolloutVariation); + when(bucketer.bucket(eq(rolloutRuleExperiment), eq(userId), eq(v4ProjectConfig), anyObject(), anyObject())).thenReturn(null); + when(bucketer.bucket(eq(rolloutRuleExperiment), eq(bucketingId), eq(v4ProjectConfig), anyObject(), anyObject())).thenReturn(rolloutVariation); DecisionService decisionService = spy(new DecisionService( bucketer, diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java index dd010731b..32cb17050 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java @@ -17,6 +17,8 @@ package com.optimizely.ab.optimizelyusercontext; import com.optimizely.ab.Optimizely; +import com.optimizely.ab.OptimizelyUserContext; +import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.junit.Test; diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index c207c5fe9..a3ddd6255 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -20,11 +20,14 @@ import com.google.common.io.Resources; import com.optimizely.ab.EventHandlerRule; import com.optimizely.ab.Optimizely; +import com.optimizely.ab.OptimizelyUserContext; import com.optimizely.ab.bucketing.UserProfileService; import com.optimizely.ab.config.Experiment; import com.optimizely.ab.config.parser.ConfigParseException; import com.optimizely.ab.event.ForwardingEventProcessor; import com.optimizely.ab.notification.NotificationCenter; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.junit.Assert; import org.junit.Before; From c0b6c85b30dc5d9caa68de2b97e0d5f06e4dbe81 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 15 Oct 2020 10:44:46 -0700 Subject: [PATCH 12/17] remove support for custom decisionservice --- .../optimizely/ab/OptimizelyUserContext.java | 27 ++--- .../com/optimizely/ab/bucketing/Bucketer.java | 45 ++------ .../ab/bucketing/DecisionService.java | 100 ++++-------------- .../optimizelydecision/DecisionMessage.java | 34 ++++++ .../optimizelydecision/DecisionReasons.java | 8 +- .../OptimizelyUserContextTest.java | 15 +-- 6 files changed, 81 insertions(+), 148 deletions(-) create mode 100644 core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionMessage.java diff --git a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java index 36c87512f..d372cfc9b 100644 --- a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java @@ -19,10 +19,11 @@ import com.optimizely.ab.bucketing.FeatureDecision; import com.optimizely.ab.config.*; import com.optimizely.ab.notification.DecisionNotification; -import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import com.optimizely.ab.optimizelydecision.DecisionMessage; import com.optimizely.ab.optimizelydecision.DecisionReasons; import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import com.optimizely.ab.optimizelydecision.OptimizelyDecision; +import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -41,10 +42,6 @@ public class OptimizelyUserContext { private static final Logger logger = LoggerFactory.getLogger(OptimizelyUserContext.class); - public final static String SDK_NOT_READY = "Optimizely SDK not configured properly yet."; - public final static String FLAG_KEY_INVALID = "No flag was found for key \"%s\"."; - public final static String VARIABLE_VALUE_INVALID = "Variable value for key \"%s\" is invalid or wrong type."; - public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull String userId, @Nonnull Map attributes) { @@ -93,12 +90,12 @@ public OptimizelyDecision decide(@Nonnull String key, ProjectConfig projectConfig = optimizely.getProjectConfig(); if (projectConfig == null) { - return OptimizelyDecision.createErrorDecision(key, this, SDK_NOT_READY); + return OptimizelyDecision.createErrorDecision(key, this, DecisionMessage.SDK_NOT_READY.reason()); } FeatureFlag flag = projectConfig.getFeatureKeyMapping().get(key); if (flag == null) { - return OptimizelyDecision.createErrorDecision(key, this, getFlagKeyInvalidMessage(key)); + return OptimizelyDecision.createErrorDecision(key, this, DecisionMessage.FLAG_KEY_INVALID.reason(key)); } Boolean sentEvent = false; @@ -107,7 +104,7 @@ public OptimizelyDecision decide(@Nonnull String key, Boolean includeReasons = allOptions.contains(OptimizelyDecideOption.INCLUDE_REASONS); DecisionReasons decisionReasons = new DecisionReasons(includeReasons); - Map copiedAttributes = copyAttributes(); + Map copiedAttributes = new HashMap<>(attributes); FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( flag, userId, @@ -287,24 +284,12 @@ public void trackEvent(@Nonnull String eventName) throws UnknownEventTypeExcepti // Utils - private Map copyAttributes() { - return new HashMap<>(attributes); - } - private List getAllOptions(List options) { List copiedOptions = new ArrayList(optimizely.defaultDecideOptions); copiedOptions.addAll(options); return copiedOptions; } - public static String getFlagKeyInvalidMessage(String flagKey) { - return String.format(FLAG_KEY_INVALID, flagKey); - } - - public static String getVariableValueInvalidMessage(String variableKey) { - return String.format(VARIABLE_VALUE_INVALID, variableKey); - } - private Map getDecisionVariableMap(@Nonnull FeatureFlag flag, @Nonnull Variation variation, @Nonnull Boolean featureEnabled, @@ -321,7 +306,7 @@ private Map getDecisionVariableMap(@Nonnull FeatureFlag flag, Object convertedValue = optimizely.convertStringToType(value, variable.getType()); if (convertedValue == null) { - decisionReasons.addError(getVariableValueInvalidMessage(variable.getKey())); + decisionReasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey())); } else if (convertedValue instanceof OptimizelyJSON) { convertedValue = ((OptimizelyJSON) convertedValue).toMap(); } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java index 7b0aa66eb..576227408 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java @@ -139,36 +139,6 @@ public Variation bucket(@Nonnull Experiment experiment, @Nonnull ProjectConfig projectConfig, @Nonnull List options, @Nonnull DecisionReasons reasons) { - - // support existing custom Bucketers - if (isMethodOverriden("bucket", Experiment.class, String.class, ProjectConfig.class)) { - return bucket(experiment, bucketingId, projectConfig); - } - - return bucketCore(experiment, bucketingId, projectConfig, options, reasons); - } - - /** - * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3. - * - * @param experiment The Experiment in which the user is to be bucketed. - * @param bucketingId string A customer-assigned value used to create the key for the murmur hash. - * @param projectConfig The current projectConfig - * @return Variation the user is bucketed into or null. - */ - @Nullable - public Variation bucket(@Nonnull Experiment experiment, - @Nonnull String bucketingId, - @Nonnull ProjectConfig projectConfig) { - return bucketCore(experiment, bucketingId, projectConfig, Collections.emptyList(), new DecisionReasons()); - } - - @Nullable - public Variation bucketCore(@Nonnull Experiment experiment, - @Nonnull String bucketingId, - @Nonnull ProjectConfig projectConfig, - @Nonnull List options, - @Nonnull DecisionReasons reasons) { // ---------- Bucket User ---------- String groupId = experiment.getGroupId(); // check whether the experiment belongs to a group @@ -202,6 +172,13 @@ public Variation bucketCore(@Nonnull Experiment experiment, return bucketToVariation(experiment, bucketingId, options, reasons); } + @Nullable + public Variation bucket(@Nonnull Experiment experiment, + @Nonnull String bucketingId, + @Nonnull ProjectConfig projectConfig) { + return bucket(experiment, bucketingId, projectConfig, Collections.emptyList(), new DecisionReasons()); + } + //======== Helper methods ========// /** @@ -217,12 +194,4 @@ int generateBucketValue(int hashCode) { return (int) Math.floor(MAX_TRAFFIC_VALUE * ratio); } - Boolean isMethodOverriden(String methodName, Class... params) { - try { - return getClass() != Bucketer.class && getClass().getDeclaredMethod(methodName, params) != null; - } catch (NoSuchMethodException e) { - return false; - } - } - } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index bc701326b..f80075c7d 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -95,30 +95,6 @@ public Variation getVariation(@Nonnull Experiment experiment, @Nonnull ProjectConfig projectConfig, @Nonnull List options, @Nonnull DecisionReasons reasons) { - - // support existing custom DecisionServices - if (isMethodOverriden("getVariation", Experiment.class, String.class, Map.class, ProjectConfig.class)) { - return getVariation(experiment, userId, filteredAttributes, projectConfig); - } - - return getVariationCore(experiment, userId, filteredAttributes, projectConfig, options, reasons); - } - - @Nullable - public Variation getVariation(@Nonnull Experiment experiment, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { - return getVariationCore(experiment, userId, filteredAttributes, projectConfig, Collections.emptyList(), new DecisionReasons()); - } - - @Nullable - public Variation getVariationCore(@Nonnull Experiment experiment, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig, - @Nonnull List options, - @Nonnull DecisionReasons reasons) { if (!ExperimentUtils.isExperimentActive(experiment, options, reasons)) { return null; } @@ -188,6 +164,14 @@ public Variation getVariationCore(@Nonnull Experiment experiment, return null; } + @Nullable + public Variation getVariation(@Nonnull Experiment experiment, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + return getVariation(experiment, userId, filteredAttributes, projectConfig, Collections.emptyList(), new DecisionReasons()); + } + /** * Get the variation the user is bucketed into for the FeatureFlag * @@ -206,30 +190,6 @@ public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, @Nonnull ProjectConfig projectConfig, @Nonnull List options, @Nonnull DecisionReasons reasons) { - // support existing custom DecisionServices - if (isMethodOverriden("getVariationForFeature", FeatureFlag.class, String.class, Map.class, ProjectConfig.class)) { - return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig); - } - - return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig, options, reasons); - } - - @Nonnull - public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig) { - - return getVariationForFeatureCore(featureFlag, userId, filteredAttributes, projectConfig,Collections.emptyList(), new DecisionReasons()); - } - - @Nonnull - public FeatureDecision getVariationForFeatureCore(@Nonnull FeatureFlag featureFlag, - @Nonnull String userId, - @Nonnull Map filteredAttributes, - @Nonnull ProjectConfig projectConfig, - @Nonnull List options, - @Nonnull DecisionReasons reasons) { if (!featureFlag.getExperimentIds().isEmpty()) { for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); @@ -256,6 +216,15 @@ public FeatureDecision getVariationForFeatureCore(@Nonnull FeatureFlag featureFl return featureDecision; } + @Nonnull + public FeatureDecision getVariationForFeature(@Nonnull FeatureFlag featureFlag, + @Nonnull String userId, + @Nonnull Map filteredAttributes, + @Nonnull ProjectConfig projectConfig) { + + return getVariationForFeature(featureFlag, userId, filteredAttributes, projectConfig,Collections.emptyList(), new DecisionReasons()); + } + /** * Try to bucket the user into a rollout rule. * Evaluate the user for rules in priority order by seeing if the user satisfies the audience. @@ -598,27 +567,6 @@ public Variation getForcedVariation(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull List options, @Nonnull DecisionReasons reasons) { - - // support existing custom DecisionServices - if (isMethodOverriden("getForcedVariation", Experiment.class, String.class)) { - return getForcedVariation(experiment, userId); - } - - return getForcedVariationCore(experiment, userId, options, reasons); - } - - @Nullable - public Variation getForcedVariation(@Nonnull Experiment experiment, - @Nonnull String userId) { - return getForcedVariationCore(experiment, userId, Collections.emptyList(), new DecisionReasons()); - } - - @Nullable - public Variation getForcedVariationCore(@Nonnull Experiment experiment, - @Nonnull String userId, - @Nonnull List options, - @Nonnull DecisionReasons reasons) { - // if the user id is invalid, return false. if (!validateUserId(userId)) { return null; @@ -642,6 +590,12 @@ public Variation getForcedVariationCore(@Nonnull Experiment experiment, return null; } + @Nullable + public Variation getForcedVariation(@Nonnull Experiment experiment, + @Nonnull String userId) { + return getForcedVariation(experiment, userId, Collections.emptyList(), new DecisionReasons()); + } + /** * Helper function to check that the provided userId is valid * @@ -657,12 +611,4 @@ private boolean validateUserId(String userId) { return true; } - Boolean isMethodOverriden(String methodName, Class... params) { - try { - return getClass() != DecisionService.class && getClass().getDeclaredMethod(methodName, params) != null; - } catch (NoSuchMethodException e) { - return false; - } - } - } diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionMessage.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionMessage.java new file mode 100644 index 000000000..c66be6bee --- /dev/null +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionMessage.java @@ -0,0 +1,34 @@ +/** + * + * Copyright 2020, 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.optimizelydecision; + +public enum DecisionMessage { + SDK_NOT_READY("Optimizely SDK not configured properly yet."), + FLAG_KEY_INVALID("No flag was found for key \"%s\"."), + VARIABLE_VALUE_INVALID("Variable value for key \"%s\" is invalid or wrong type."); + + private String format; + + DecisionMessage(String format) { + this.format = format; + } + + public String reason(Object... args){ + return String.format(format, args); + } +} diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java index c7c7d91b6..ca8bf7c12 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java @@ -21,14 +21,12 @@ public class DecisionReasons { - boolean includeReasons; - List errors; - List logs; + private boolean includeReasons; + private final List errors = new ArrayList<>(); + private final List logs = new ArrayList<>(); public DecisionReasons(boolean includeReasons) { this.includeReasons = includeReasons; - this.errors = new ArrayList(); - this.logs = new ArrayList(); } public DecisionReasons() { diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java index a3ddd6255..9cd978852 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java @@ -26,6 +26,7 @@ import com.optimizely.ab.config.parser.ConfigParseException; import com.optimizely.ab.event.ForwardingEventProcessor; import com.optimizely.ab.notification.NotificationCenter; +import com.optimizely.ab.optimizelydecision.DecisionMessage; import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; @@ -441,11 +442,11 @@ public void decideOptions_includeReasons() { String flagKey = "invalid_key"; OptimizelyDecision decision = user.decide(flagKey); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); flagKey = "feature_1"; decision = user.decide(flagKey); @@ -505,7 +506,7 @@ public void decide_sdkNotReady() { assertEquals(decision.getUserContext(), user); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.SDK_NOT_READY); + assertEquals(decision.getReasons().get(0), DecisionMessage.SDK_NOT_READY.reason()); } @Test @@ -518,7 +519,7 @@ public void decide_invalidFeatureKey() { assertNull(decision.getVariationKey()); assertFalse(decision.getEnabled()); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); } @Test @@ -560,7 +561,7 @@ public void decideAll_errorDecisionIncluded() { OptimizelyDecision.createErrorDecision( flagKey2, user, - OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey2))); + DecisionMessage.FLAG_KEY_INVALID.reason(flagKey2))); } // reasons (errors) @@ -574,7 +575,7 @@ public void decideReasons_sdkNotReady() { OptimizelyDecision decision = user.decide(flagKey); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.SDK_NOT_READY); + assertEquals(decision.getReasons().get(0), DecisionMessage.SDK_NOT_READY.reason()); } @Test @@ -585,7 +586,7 @@ public void decideReasons_featureKeyInvalid() { OptimizelyDecision decision = user.decide(flagKey); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), OptimizelyUserContext.getFlagKeyInvalidMessage(flagKey)); + assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); } @Test From a93fc23f2f82fb06e367f342a156253a0fe1a3cd Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 15 Oct 2020 11:24:03 -0700 Subject: [PATCH 13/17] thread-safe setAttribute --- .../main/java/com/optimizely/ab/OptimizelyUserContext.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java index d372cfc9b..ac2028da4 100644 --- a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java @@ -29,6 +29,7 @@ import javax.annotation.Nonnull; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; public class OptimizelyUserContext { @Nonnull @@ -47,7 +48,7 @@ public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull Map attributes) { this.optimizely = optimizely; this.userId = userId; - this.attributes = new HashMap<>(attributes); + this.attributes = new ConcurrentHashMap<>(attributes); } public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull String userId) { @@ -59,7 +60,7 @@ public String getUserId() { } public Map getAttributes() { - return attributes; + return new HashMap(attributes); } public Optimizely getOptimizely() { From f803cd7d40e4f19c0995173271ad39207a5b553f Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 15 Oct 2020 14:29:51 -0700 Subject: [PATCH 14/17] add option and reasons to condition evaluation --- .../ab/config/audience/AndCondition.java | 16 ++- .../config/audience/AudienceIdCondition.java | 22 +++-- .../ab/config/audience/Condition.java | 10 ++ .../ab/config/audience/EmptyCondition.java | 14 ++- .../ab/config/audience/NotCondition.java | 16 ++- .../ab/config/audience/NullCondition.java | 14 ++- .../ab/config/audience/OrCondition.java | 15 ++- .../ab/config/audience/UserAttribute.java | 13 ++- .../ab/internal/ExperimentUtils.java | 26 +---- .../OptimizelyUserContextTest.java | 66 ++++++++++--- .../AudienceConditionEvaluationTest.java | 99 +++++++++---------- .../OptimizelyDecisionTest.java | 3 +- 12 files changed, 204 insertions(+), 110 deletions(-) rename core-api/src/test/java/com/optimizely/ab/{optimizelyusercontext => }/OptimizelyUserContextTest.java (91%) rename core-api/src/test/java/com/optimizely/ab/{optimizelyusercontext => optimizelydecision}/OptimizelyDecisionTest.java (96%) diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/AndCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/AndCondition.java index 8b458d059..a8a5f3e65 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/AndCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/AndCondition.java @@ -17,10 +17,12 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nonnull; import javax.annotation.Nullable; -import javax.annotation.concurrent.Immutable; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -40,7 +42,10 @@ public List getConditions() { } @Nullable - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { if (conditions == null) return null; boolean foundNull = false; // According to the matrix where: @@ -51,7 +56,7 @@ public Boolean evaluate(ProjectConfig config, Map attributes) { // true and true is true // null and null is null for (Condition condition : conditions) { - Boolean conditionEval = condition.evaluate(config, attributes); + Boolean conditionEval = condition.evaluate(config, attributes, options, reasons); if (conditionEval == null) { foundNull = true; } else if (!conditionEval) { // false with nulls or trues is false. @@ -67,6 +72,11 @@ public Boolean evaluate(ProjectConfig config, Map attributes) { return true; // otherwise, return true } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + @Override public String toString() { StringBuilder s = new StringBuilder(); diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java index 57a4e5bec..ee58fe4d9 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java @@ -18,14 +18,15 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; import com.optimizely.ab.config.ProjectConfig; -import com.optimizely.ab.internal.InvalidAudienceCondition; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nullable; -import javax.annotation.concurrent.Immutable; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Objects; @@ -66,20 +67,29 @@ public String getAudienceId() { @Nullable @Override - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { if (config != null) { audience = config.getAudienceIdMapping().get(audienceId); } if (audience == null) { - logger.error("Audience {} could not be found.", audienceId); + String message = reasons.addInfoF("Audience %s could not be found.", audienceId); + logger.error(message); return null; } logger.debug("Starting to evaluate audience \"{}\" with conditions: {}.", audience.getId(), audience.getConditions()); - Boolean result = audience.getConditions().evaluate(config, attributes); + Boolean result = audience.getConditions().evaluate(config, attributes, options, reasons); logger.debug("Audience \"{}\" evaluated to {}.", audience.getId(), result); return result; } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java index 772d2b03e..dff29a0dc 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java @@ -17,8 +17,11 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nullable; +import java.util.List; import java.util.Map; /** @@ -26,6 +29,13 @@ */ public interface Condition { + @Nullable + Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons); + @Nullable Boolean evaluate(ProjectConfig config, Map attributes); + } diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/EmptyCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/EmptyCondition.java index 8f8aedeae..ad9d79b41 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/EmptyCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/EmptyCondition.java @@ -16,15 +16,27 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; import java.util.Map; public class EmptyCondition implements Condition { @Nullable @Override - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { return true; } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + } diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java index b7f45f2ac..96a192d98 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java @@ -17,11 +17,15 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import javax.annotation.Nonnull; +import java.util.Collections; +import java.util.List; import java.util.Map; /** @@ -41,12 +45,20 @@ public Condition getCondition() { } @Nullable - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { - Boolean conditionEval = condition == null ? null : condition.evaluate(config, attributes); + Boolean conditionEval = condition == null ? null : condition.evaluate(config, attributes, options, reasons); return (conditionEval == null ? null : !conditionEval); } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + @Override public String toString() { StringBuilder s = new StringBuilder(); diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/NullCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/NullCondition.java index fcf5100db..bd49229ae 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/NullCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/NullCondition.java @@ -16,14 +16,26 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; import java.util.Map; public class NullCondition implements Condition { @Nullable @Override - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { return null; } + + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } } diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java b/core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java index 70572a9a9..c3f890b98 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java @@ -17,10 +17,13 @@ package com.optimizely.ab.config.audience; import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -45,11 +48,14 @@ public List getConditions() { // false or false is false // null or null is null @Nullable - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { if (conditions == null) return null; boolean foundNull = false; for (Condition condition : conditions) { - Boolean conditionEval = condition.evaluate(config, attributes); + Boolean conditionEval = condition.evaluate(config, attributes, options, reasons); if (conditionEval == null) { // true with falses and nulls is still true foundNull = true; } else if (conditionEval) { @@ -65,6 +71,11 @@ public Boolean evaluate(ProjectConfig config, Map attributes) { return false; } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + @Override public String toString() { StringBuilder s = new StringBuilder(); diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java index 277f2f184..9141300aa 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.audience.match.*; +import com.optimizely.ab.optimizelydecision.DecisionReasons; +import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,6 +30,7 @@ import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import java.util.Collections; +import java.util.List; import java.util.Map; /** @@ -71,7 +74,10 @@ public Object getValue() { } @Nullable - public Boolean evaluate(ProjectConfig config, Map attributes) { + public Boolean evaluate(ProjectConfig config, + Map attributes, + List options, + DecisionReasons reasons) { if (attributes == null) { attributes = Collections.emptyMap(); } @@ -118,6 +124,11 @@ public Boolean evaluate(ProjectConfig config, Map attributes) { return null; } + @Nullable + public Boolean evaluate(ProjectConfig config, Map attributes) { + return evaluate(config, attributes, Collections.emptyList(), new DecisionReasons()); + } + @Override public String toString() { final String valueStr; diff --git a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java index e703cc527..479e1c237 100644 --- a/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java +++ b/core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java @@ -86,10 +86,10 @@ public static boolean doesUserMeetAudienceConditions(@Nonnull ProjectConfig proj @Nonnull DecisionReasons reasons) { if (experiment.getAudienceConditions() != null) { logger.debug("Evaluating audiences for {} \"{}\": {}.", loggingEntityType, loggingKey, experiment.getAudienceConditions()); - Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey); + Boolean resolveReturn = evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, options, reasons); return resolveReturn == null ? false : resolveReturn; } else { - Boolean resolveReturn = evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey); + Boolean resolveReturn = evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey, options, reasons); return Boolean.TRUE.equals(resolveReturn); } } @@ -137,7 +137,7 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, logger.debug("Evaluating audiences for {} \"{}\": {}.", loggingEntityType, loggingKey, conditions); - Boolean result = implicitOr.evaluate(projectConfig, attributes); + Boolean result = implicitOr.evaluate(projectConfig, attributes, options, reasons); String message = reasons.addInfoF("Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); logger.info(message); @@ -145,15 +145,6 @@ public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, return result; } - @Nullable - public static Boolean evaluateAudience(@Nonnull ProjectConfig projectConfig, - @Nonnull Experiment experiment, - @Nonnull Map attributes, - @Nonnull String loggingEntityType, - @Nonnull String loggingKey) { - return evaluateAudience(projectConfig, experiment, attributes, loggingEntityType, loggingKey, Collections.emptyList(), new DecisionReasons()); - } - @Nullable public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @@ -167,7 +158,7 @@ public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectC if (conditions == null) return null; try { - Boolean result = conditions.evaluate(projectConfig, attributes); + Boolean result = conditions.evaluate(projectConfig, attributes, options, reasons); String message = reasons.addInfoF("Audiences for %s \"%s\" collectively evaluated to %s.", loggingEntityType, loggingKey, result); logger.info(message); return result; @@ -178,13 +169,4 @@ public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectC } } - @Nullable - public static Boolean evaluateAudienceConditions(@Nonnull ProjectConfig projectConfig, - @Nonnull Experiment experiment, - @Nonnull Map attributes, - @Nonnull String loggingEntityType, - @Nonnull String loggingKey) { - return evaluateAudienceConditions(projectConfig, experiment, attributes, loggingEntityType, loggingKey, Collections.emptyList(), new DecisionReasons()); - } - } \ No newline at end of file diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java similarity index 91% rename from core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java rename to core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java index 9cd978852..5be10890a 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java @@ -14,15 +14,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab; import com.google.common.base.Charsets; import com.google.common.io.Resources; -import com.optimizely.ab.EventHandlerRule; -import com.optimizely.ab.Optimizely; -import com.optimizely.ab.OptimizelyUserContext; import com.optimizely.ab.bucketing.UserProfileService; +import com.optimizely.ab.config.DatafileProjectConfig; import com.optimizely.ab.config.Experiment; +import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.config.Rollout; import com.optimizely.ab.config.parser.ConfigParseException; import com.optimizely.ab.event.ForwardingEventProcessor; import com.optimizely.ab.notification.NotificationCenter; @@ -30,6 +30,7 @@ import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import junit.framework.TestCase; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; @@ -104,6 +105,20 @@ public void setAttribute() { assertEquals(newAttributes.get("k4"), 3.5); } + @Test + public void setAttribute_noAttribute() { + OptimizelyUserContext user = new OptimizelyUserContext(optimizely, userId); + + user.setAttribute("k1", "v1"); + user.setAttribute("k2", true); + + assertEquals(user.getOptimizely(), optimizely); + assertEquals(user.getUserId(), userId); + Map newAttributes = user.getAttributes(); + assertEquals(newAttributes.get("k1"), "v1"); + assertEquals(newAttributes.get("k2"), true); + } + @Test public void setAttribute_override() { Map attributes = Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); @@ -112,8 +127,6 @@ public void setAttribute_override() { user.setAttribute("k1", "v1"); user.setAttribute(ATTRIBUTE_HOUSE_KEY, "v2"); - assertEquals(user.getOptimizely(), optimizely); - assertEquals(user.getUserId(), userId); Map newAttributes = user.getAttributes(); assertEquals(newAttributes.get("k1"), "v1"); assertEquals(newAttributes.get(ATTRIBUTE_HOUSE_KEY), "v2"); @@ -442,7 +455,7 @@ public void decideOptions_includeReasons() { String flagKey = "invalid_key"; OptimizelyDecision decision = user.decide(flagKey); assertEquals(decision.getReasons().size(), 1); - assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); + TestCase.assertEquals(decision.getReasons().get(0), DecisionMessage.FLAG_KEY_INVALID.reason(flagKey)); decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertEquals(decision.getReasons().size(), 1); @@ -597,6 +610,16 @@ public void decideReasons_variableValueInvalid() { @Test public void decideReasons_conditionNoMatchingAudience() throws ConfigParseException { + String flagKey = "feature_1"; + String audienceId = "invalid_id"; + setAudienceForFeatureTest(flagKey, audienceId); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); + + assertTrue(decision.getReasons().contains( + String.format("Audience %s could not be found.", audienceId) + )); } @Test @@ -641,8 +664,7 @@ public void decideReasons_gotVariationFromUserProfile() throws Exception { OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( - String.format( - "Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", variationKey2, experimentKey, userId) + String.format("Returning previously activated variation \"%s\" of experiment \"%s\" for user \"%s\" from user profile.", variationKey2, experimentKey, userId) )); } @@ -665,9 +687,8 @@ public void decideReasons_userMeetsConditionsForTargetingRule() { OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); assertTrue(decision.getReasons().contains( - String.format( - "The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, flagKey) - )); + String.format("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".", userId, flagKey) + )); } @Test @@ -790,12 +811,27 @@ Map createUserProfileMap(String experimentId, String variationId return userProfileMap; } - void setAudienceForFeatureTest(String featureKey, String audienceId) throws ConfigParseException { - String experimentId = optimizely.getProjectConfig().getFeatureKeyMapping().get(featureKey).getExperimentIds().get(0); - Experiment experimentReal = optimizely.getProjectConfig().getExperimentIdMapping().get(experimentId); + void setAudienceForFeatureTest(String flagKey, String audienceId) throws ConfigParseException { + ProjectConfig configReal = new DatafileProjectConfig.Builder().withDatafile(datafile).build(); + ProjectConfig config = spy(configReal); + optimizely = Optimizely.builder().withConfig(config).build(); + + String experimentId = config.getFeatureKeyMapping().get(flagKey).getExperimentIds().get(0); + String rolloutId = config.getFeatureKeyMapping().get(flagKey).getRolloutId(); + Map experimentIdMapping = new HashMap<>(config.getExperimentIdMapping()); + Map rolloutIdMapping = new HashMap<>(config.getRolloutIdMapping()); + Experiment experimentReal = experimentIdMapping.get(experimentId); + Rollout rolloutReal = rolloutIdMapping.get(rolloutId); Experiment experiment = spy(experimentReal); + Rollout rollout = spy(rolloutReal); when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + + experimentIdMapping.put(experimentId, experiment); + rolloutIdMapping.put(rolloutId, rollout); + + when(config.getExperimentIdMapping()).thenReturn(experimentIdMapping); + when(config.getRolloutIdMapping()).thenReturn(rolloutIdMapping); } } diff --git a/core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java b/core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java index 772d22ef7..dbcdda88e 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/audience/AudienceConditionEvaluationTest.java @@ -18,27 +18,16 @@ import ch.qos.logback.classic.Level; import com.optimizely.ab.internal.LogbackVerifier; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import java.math.BigInteger; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import java.util.*; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertNull; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; /** * Tests for the evaluation of different audience condition types (And, Or, Not, and UserAttribute) @@ -1183,11 +1172,11 @@ public void notConditionEvaluateNull() { @Test public void notConditionEvaluateTrue() { UserAttribute userAttribute = mock(UserAttribute.class); - when(userAttribute.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); NotCondition notCondition = new NotCondition(userAttribute); assertTrue(notCondition.evaluate(null, testUserAttributes)); - verify(userAttribute, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1196,11 +1185,11 @@ public void notConditionEvaluateTrue() { @Test public void notConditionEvaluateFalse() { UserAttribute userAttribute = mock(UserAttribute.class); - when(userAttribute.evaluate(null, testUserAttributes)).thenReturn(true); + when(userAttribute.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); NotCondition notCondition = new NotCondition(userAttribute); assertFalse(notCondition.evaluate(null, testUserAttributes)); - verify(userAttribute, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1209,10 +1198,10 @@ public void notConditionEvaluateFalse() { @Test public void orConditionEvaluateTrue() { UserAttribute userAttribute1 = mock(UserAttribute.class); - when(userAttribute1.evaluate(null, testUserAttributes)).thenReturn(true); + when(userAttribute1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); UserAttribute userAttribute2 = mock(UserAttribute.class); - when(userAttribute2.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); List conditions = new ArrayList(); conditions.add(userAttribute1); @@ -1220,9 +1209,9 @@ public void orConditionEvaluateTrue() { OrCondition orCondition = new OrCondition(conditions); assertTrue(orCondition.evaluate(null, testUserAttributes)); - verify(userAttribute1, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); // shouldn't be called due to short-circuiting in 'Or' evaluation - verify(userAttribute2, times(0)).evaluate(null, testUserAttributes); + verify(userAttribute2, times(0)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1231,10 +1220,10 @@ public void orConditionEvaluateTrue() { @Test public void orConditionEvaluateTrueWithNullAndTrue() { UserAttribute userAttribute1 = mock(UserAttribute.class); - when(userAttribute1.evaluate(null, testUserAttributes)).thenReturn(null); + when(userAttribute1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(null); UserAttribute userAttribute2 = mock(UserAttribute.class); - when(userAttribute2.evaluate(null, testUserAttributes)).thenReturn(true); + when(userAttribute2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); List conditions = new ArrayList(); conditions.add(userAttribute1); @@ -1242,9 +1231,9 @@ public void orConditionEvaluateTrueWithNullAndTrue() { OrCondition orCondition = new OrCondition(conditions); assertTrue(orCondition.evaluate(null, testUserAttributes)); - verify(userAttribute1, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); // shouldn't be called due to short-circuiting in 'Or' evaluation - verify(userAttribute2, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1253,10 +1242,10 @@ public void orConditionEvaluateTrueWithNullAndTrue() { @Test public void orConditionEvaluateNullWithNullAndFalse() { UserAttribute userAttribute1 = mock(UserAttribute.class); - when(userAttribute1.evaluate(null, testUserAttributes)).thenReturn(null); + when(userAttribute1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(null); UserAttribute userAttribute2 = mock(UserAttribute.class); - when(userAttribute2.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); List conditions = new ArrayList(); conditions.add(userAttribute1); @@ -1264,9 +1253,9 @@ public void orConditionEvaluateNullWithNullAndFalse() { OrCondition orCondition = new OrCondition(conditions); assertNull(orCondition.evaluate(null, testUserAttributes)); - verify(userAttribute1, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); // shouldn't be called due to short-circuiting in 'Or' evaluation - verify(userAttribute2, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1275,10 +1264,10 @@ public void orConditionEvaluateNullWithNullAndFalse() { @Test public void orConditionEvaluateFalseWithFalseAndFalse() { UserAttribute userAttribute1 = mock(UserAttribute.class); - when(userAttribute1.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); UserAttribute userAttribute2 = mock(UserAttribute.class); - when(userAttribute2.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); List conditions = new ArrayList(); conditions.add(userAttribute1); @@ -1286,9 +1275,9 @@ public void orConditionEvaluateFalseWithFalseAndFalse() { OrCondition orCondition = new OrCondition(conditions); assertFalse(orCondition.evaluate(null, testUserAttributes)); - verify(userAttribute1, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); // shouldn't be called due to short-circuiting in 'Or' evaluation - verify(userAttribute2, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1297,10 +1286,10 @@ public void orConditionEvaluateFalseWithFalseAndFalse() { @Test public void orConditionEvaluateFalse() { UserAttribute userAttribute1 = mock(UserAttribute.class); - when(userAttribute1.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); UserAttribute userAttribute2 = mock(UserAttribute.class); - when(userAttribute2.evaluate(null, testUserAttributes)).thenReturn(false); + when(userAttribute2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); List conditions = new ArrayList(); conditions.add(userAttribute1); @@ -1308,8 +1297,8 @@ public void orConditionEvaluateFalse() { OrCondition orCondition = new OrCondition(conditions); assertFalse(orCondition.evaluate(null, testUserAttributes)); - verify(userAttribute1, times(1)).evaluate(null, testUserAttributes); - verify(userAttribute2, times(1)).evaluate(null, testUserAttributes); + verify(userAttribute1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); + verify(userAttribute2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1318,10 +1307,10 @@ public void orConditionEvaluateFalse() { @Test public void andConditionEvaluateTrue() { OrCondition orCondition1 = mock(OrCondition.class); - when(orCondition1.evaluate(null, testUserAttributes)).thenReturn(true); + when(orCondition1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); OrCondition orCondition2 = mock(OrCondition.class); - when(orCondition2.evaluate(null, testUserAttributes)).thenReturn(true); + when(orCondition2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); List conditions = new ArrayList(); conditions.add(orCondition1); @@ -1329,8 +1318,8 @@ public void andConditionEvaluateTrue() { AndCondition andCondition = new AndCondition(conditions); assertTrue(andCondition.evaluate(null, testUserAttributes)); - verify(orCondition1, times(1)).evaluate(null, testUserAttributes); - verify(orCondition2, times(1)).evaluate(null, testUserAttributes); + verify(orCondition1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); + verify(orCondition2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1339,10 +1328,10 @@ public void andConditionEvaluateTrue() { @Test public void andConditionEvaluateFalseWithNullAndFalse() { OrCondition orCondition1 = mock(OrCondition.class); - when(orCondition1.evaluate(null, testUserAttributes)).thenReturn(null); + when(orCondition1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(null); OrCondition orCondition2 = mock(OrCondition.class); - when(orCondition2.evaluate(null, testUserAttributes)).thenReturn(false); + when(orCondition2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); List conditions = new ArrayList(); conditions.add(orCondition1); @@ -1350,8 +1339,8 @@ public void andConditionEvaluateFalseWithNullAndFalse() { AndCondition andCondition = new AndCondition(conditions); assertFalse(andCondition.evaluate(null, testUserAttributes)); - verify(orCondition1, times(1)).evaluate(null, testUserAttributes); - verify(orCondition2, times(1)).evaluate(null, testUserAttributes); + verify(orCondition1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); + verify(orCondition2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1360,10 +1349,10 @@ public void andConditionEvaluateFalseWithNullAndFalse() { @Test public void andConditionEvaluateNullWithNullAndTrue() { OrCondition orCondition1 = mock(OrCondition.class); - when(orCondition1.evaluate(null, testUserAttributes)).thenReturn(null); + when(orCondition1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(null); OrCondition orCondition2 = mock(OrCondition.class); - when(orCondition2.evaluate(null, testUserAttributes)).thenReturn(true); + when(orCondition2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); List conditions = new ArrayList(); conditions.add(orCondition1); @@ -1371,8 +1360,8 @@ public void andConditionEvaluateNullWithNullAndTrue() { AndCondition andCondition = new AndCondition(conditions); assertNull(andCondition.evaluate(null, testUserAttributes)); - verify(orCondition1, times(1)).evaluate(null, testUserAttributes); - verify(orCondition2, times(1)).evaluate(null, testUserAttributes); + verify(orCondition1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); + verify(orCondition2, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); } /** @@ -1381,10 +1370,10 @@ public void andConditionEvaluateNullWithNullAndTrue() { @Test public void andConditionEvaluateFalse() { OrCondition orCondition1 = mock(OrCondition.class); - when(orCondition1.evaluate(null, testUserAttributes)).thenReturn(false); + when(orCondition1.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(false); OrCondition orCondition2 = mock(OrCondition.class); - when(orCondition2.evaluate(null, testUserAttributes)).thenReturn(true); + when(orCondition2.evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject())).thenReturn(true); // and[false, true] List conditions = new ArrayList(); @@ -1393,9 +1382,9 @@ public void andConditionEvaluateFalse() { AndCondition andCondition = new AndCondition(conditions); assertFalse(andCondition.evaluate(null, testUserAttributes)); - verify(orCondition1, times(1)).evaluate(null, testUserAttributes); + verify(orCondition1, times(1)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); // shouldn't be called due to short-circuiting in 'And' evaluation - verify(orCondition2, times(0)).evaluate(null, testUserAttributes); + verify(orCondition2, times(0)).evaluate(eq(null), eq(testUserAttributes), anyObject(), anyObject()); OrCondition orCondition3 = mock(OrCondition.class); when(orCondition3.evaluate(null, testUserAttributes)).thenReturn(null); diff --git a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java b/core-api/src/test/java/com/optimizely/ab/optimizelydecision/OptimizelyDecisionTest.java similarity index 96% rename from core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java rename to core-api/src/test/java/com/optimizely/ab/optimizelydecision/OptimizelyDecisionTest.java index 32cb17050..92fdbde4a 100644 --- a/core-api/src/test/java/com/optimizely/ab/optimizelyusercontext/OptimizelyDecisionTest.java +++ b/core-api/src/test/java/com/optimizely/ab/optimizelydecision/OptimizelyDecisionTest.java @@ -14,11 +14,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.optimizely.ab.optimizelyusercontext; +package com.optimizely.ab.optimizelydecision; import com.optimizely.ab.Optimizely; import com.optimizely.ab.OptimizelyUserContext; -import com.optimizely.ab.optimizelydecision.OptimizelyDecision; import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.junit.Test; From 43f7b017f8b03477553812e86d6e0714121c7d4e Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Thu, 15 Oct 2020 18:03:56 -0700 Subject: [PATCH 15/17] add more tests for reasons --- .../ab/config/audience/UserAttribute.java | 22 +- .../ab/OptimizelyUserContextTest.java | 286 +++++++++++++++--- 2 files changed, 252 insertions(+), 56 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java index 9141300aa..5044e3dac 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java @@ -85,7 +85,8 @@ public Boolean evaluate(ProjectConfig config, Object userAttributeValue = attributes.get(name); if (!"custom_attribute".equals(type)) { - logger.warn("Audience condition \"{}\" uses an unknown condition type. You may need to upgrade to a newer release of the Optimizely SDK.", this); + String message = reasons.addInfoF("Audience condition \"%s\" uses an unknown condition type. You may need to upgrade to a newer release of the Optimizely SDK.", this); + logger.warn(message); return null; // unknown type } // check user attribute value is equal @@ -100,26 +101,27 @@ public Boolean evaluate(ProjectConfig config, } catch(UnknownValueTypeException e) { if (!attributes.containsKey(name)) { //Missing attribute value - logger.debug("Audience condition \"{}\" evaluated to UNKNOWN because no value was passed for user attribute \"{}\"", this, name); + String message = reasons.addInfoF("Audience condition \"%s\" evaluated to UNKNOWN because no value was passed for user attribute \"%s\"", this, name); + logger.debug(message); } else { //if attribute value is not valid if (userAttributeValue != null) { - logger.warn( - "Audience condition \"{}\" evaluated to UNKNOWN because a value of type \"{}\" was passed for user attribute \"{}\"", + String message = reasons.addInfoF("Audience condition \"%s\" evaluated to UNKNOWN because a value of type \"%s\" was passed for user attribute \"%s\"", this, userAttributeValue.getClass().getCanonicalName(), name); + logger.warn(message); } else { - logger.debug( - "Audience condition \"{}\" evaluated to UNKNOWN because a null value was passed for user attribute \"{}\"", - this, - name); + String message = reasons.addInfoF("Audience condition \"%s\" evaluated to UNKNOWN because a null value was passed for user attribute \"%s\"", this, name); + logger.debug(message); } } } catch (UnknownMatchTypeException | UnexpectedValueTypeException e) { - logger.warn("Audience condition \"{}\" " + e.getMessage(), this); + String message = reasons.addInfoF("Audience condition \"%s\" " + e.getMessage(), this); + logger.warn(message); } catch (NullPointerException e) { - logger.error("attribute or value null for match {}", match != null ? match : "legacy condition", e); + String message = reasons.addInfoF("attribute or value null for match %s", match != null ? match : "legacy condition"); + logger.error(message, e); } return null; } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java index 5be10890a..ce8131049 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java @@ -19,10 +19,7 @@ import com.google.common.base.Charsets; import com.google.common.io.Resources; import com.optimizely.ab.bucketing.UserProfileService; -import com.optimizely.ab.config.DatafileProjectConfig; -import com.optimizely.ab.config.Experiment; -import com.optimizely.ab.config.ProjectConfig; -import com.optimizely.ab.config.Rollout; +import com.optimizely.ab.config.*; import com.optimizely.ab.config.parser.ConfigParseException; import com.optimizely.ab.event.ForwardingEventProcessor; import com.optimizely.ab.notification.NotificationCenter; @@ -44,19 +41,23 @@ import static com.optimizely.ab.notification.DecisionNotification.FlagDecisionNotificationBuilder.*; import static junit.framework.TestCase.assertEquals; import static junit.framework.TestCase.assertTrue; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.*; import static org.mockito.Mockito.*; public class OptimizelyUserContextTest { @Rule public EventHandlerRule eventHandler = new EventHandlerRule(); - public Optimizely optimizely; - public String datafile; - public String userId = "tester"; + String userId = "tester"; boolean isListenerCalled = false; + Optimizely optimizely; + String datafile; + ProjectConfig config; + Map experimentIdMapping; + Map featureKeyMapping; + Map groupIdMapping; + @Before public void setUp() throws Exception { datafile = Resources.toString(Resources.getResource("config/decide-project-config.json"), Charsets.UTF_8); @@ -604,6 +605,17 @@ public void decideReasons_featureKeyInvalid() { @Test public void decideReasons_variableValueInvalid() { + String flagKey = "feature_1"; + + FeatureFlag flag = getSpyFeatureFlag(flagKey); + List variables = Arrays.asList(new FeatureVariable("any-id", "any-key", "invalid", null, "integer", null)); + when(flag.getVariables()).thenReturn(variables); + addSpyFeatureFlag(flag); + + OptimizelyUserContext user = optimizely.createUserContext(userId); + OptimizelyDecision decision = user.decide(flagKey); + + assertEquals(decision.getReasons().get(0), DecisionMessage.VARIABLE_VALUE_INVALID.reason("any-key")); } // reasons (logs with includeReasons) @@ -612,10 +624,11 @@ public void decideReasons_variableValueInvalid() { public void decideReasons_conditionNoMatchingAudience() throws ConfigParseException { String flagKey = "feature_1"; String audienceId = "invalid_id"; - setAudienceForFeatureTest(flagKey, audienceId); - OptimizelyUserContext user = optimizely.createUserContext(userId); - OptimizelyDecision decision = user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); assertTrue(decision.getReasons().contains( String.format("Audience %s could not be found.", audienceId) @@ -623,26 +636,108 @@ public void decideReasons_conditionNoMatchingAudience() throws ConfigParseExcept } @Test - public void decideReasons_conditionInvalidFormat() {} - @Test - public void decideReasons_evaluateAttributeInvalidCondition() {} - @Test - public void decideReasons_evaluateAttributeInvalidType() {} - @Test - public void decideReasons_evaluateAttributeValueOutOfRange() {} + public void decideReasons_evaluateAttributeInvalidType() { + String flagKey = "feature_1"; + String audienceId = "13389130056"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("country", 25)); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='country', type='custom_attribute', match='exact', value='US'}\" evaluated to UNKNOWN because a value of type \"java.lang.Integer\" was passed for user attribute \"country\"") + )); + } + @Test - public void decideReasons_userAttributeInvalidType() {} + public void decideReasons_evaluateAttributeValueOutOfRange() { + String flagKey = "feature_1"; + String audienceId = "age_18"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("age", (float)Math.pow(2, 54))); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='age', type='custom_attribute', match='gt', value=18.0}\" evaluated to UNKNOWN because a value of type \"java.lang.Float\" was passed for user attribute \"age\"") + )); + } + @Test - public void decideReasons_userAttributeInvalidMatch() {} + public void decideReasons_userAttributeInvalidType() { + String flagKey = "feature_1"; + String audienceId = "invalid_type"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("age", 25)); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='age', type='invalid', match='gt', value=18.0}\" uses an unknown condition type. You may need to upgrade to a newer release of the Optimizely SDK.") + )); + } + @Test - public void decideReasons_userAttributeNilValue() {} + public void decideReasons_userAttributeInvalidMatch() { + String flagKey = "feature_1"; + String audienceId = "invalid_match"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("age", 25)); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='age', type='custom_attribute', match='invalid', value=18.0}\" uses an unknown match type. You may need to upgrade to a newer release of the Optimizely SDK.") + )); + } + @Test - public void decideReasons_userAttributeInvalidName() {} + public void decideReasons_userAttributeNilValue() { + String flagKey = "feature_1"; + String audienceId = "nil_value"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("age", 25)); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='age', type='custom_attribute', match='gt', value=null}\" evaluated to UNKNOWN because a value of type \"java.lang.Integer\" was passed for user attribute \"age\"") + )); + } + @Test - public void decideReasons_missingAttributeValue() {} + public void decideReasons_missingAttributeValue() { + String flagKey = "feature_1"; + String audienceId = "age_18"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + + assertTrue(decision.getReasons().contains( + String.format("Audience condition \"{name='age', type='custom_attribute', match='gt', value=18.0}\" evaluated to UNKNOWN because no value was passed for user attribute \"age\"") + )); + } @Test - public void decideReasons_experimentNotRunning() {} + public void decideReasons_experimentNotRunning() { + String flagKey = "feature_1"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.isActive()).thenReturn(false); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + + assertTrue(decision.getReasons().contains( + String.format("Experiment \"exp_with_audience\" is not running.") + )); + } @Test public void decideReasons_gotVariationFromUserProfile() throws Exception { @@ -670,12 +765,32 @@ public void decideReasons_gotVariationFromUserProfile() throws Exception { @Test public void decideReasons_forcedVariationFound() { + String flagKey = "feature_1"; + String variationKey = "b"; + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getUserIdToVariationKeyMap()).thenReturn(Collections.singletonMap(userId, variationKey)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + + assertTrue(decision.getReasons().contains( + String.format("User \"%s\" is forced in variation \"%s\".", userId, variationKey) + )); } @Test public void decideReasons_forcedVariationFoundButInvalid() { + String flagKey = "feature_1"; + String variationKey = "invalid-key"; + + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getUserIdToVariationKeyMap()).thenReturn(Collections.singletonMap(userId, variationKey)); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + assertTrue(decision.getReasons().contains( + String.format("Variation \"%s\" is not in the datafile. Not activating user \"%s\".", variationKey, userId) + )); } @Test @@ -760,28 +875,68 @@ public void decideReasons_userBucketedIntoVariationInExperiment() { @Test public void decideReasons_userNotBucketedIntoVariation() { - } + String flagKey = "feature_2"; - @Test - public void decideReasons_userBucketedIntoInvalidVariation() { + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getTrafficAllocation()).thenReturn(Arrays.asList(new TrafficAllocation("any-id", 0))); + addSpyExperiment(experiment); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey, Collections.singletonMap("age", 25)); + + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"%s\" is not in any variation of experiment \"exp_no_audience\".", userId) + )); } @Test public void decideReasons_userBucketedIntoExperimentInGroup() { + String flagKey = "feature_3"; + String experimentId = "10390965532"; // "group_exp_1" + + FeatureFlag flag = getSpyFeatureFlag(flagKey); + when(flag.getExperimentIds()).thenReturn(Arrays.asList(experimentId)); + addSpyFeatureFlag(flag); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"tester\" is in experiment \"group_exp_1\" of group 13142870430.") + )); } + @Test public void decideReasons_userNotBucketedIntoExperimentInGroup() { + String flagKey = "feature_3"; + String experimentId = "10420843432"; // "group_exp_2" + FeatureFlag flag = getSpyFeatureFlag(flagKey); + when(flag.getExperimentIds()).thenReturn(Arrays.asList(experimentId)); + addSpyFeatureFlag(flag); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"tester\" is not in experiment \"group_exp_2\" of group 13142870430.") + )); } + @Test public void decideReasons_userNotBucketedIntoAnyExperimentInGroup() { + String flagKey = "feature_3"; + String experimentId = "10390965532"; // "group_exp_1" + String groupId = "13142870430"; - } - @Test - public void decideReasons_userBucketedIntoInvalidExperiment() { + FeatureFlag flag = getSpyFeatureFlag(flagKey); + when(flag.getExperimentIds()).thenReturn(Arrays.asList(experimentId)); + addSpyFeatureFlag(flag); + + Group group = getSpyGroup(groupId); + when(group.getTrafficAllocation()).thenReturn(Collections.emptyList()); + addSpyGroup(group); + OptimizelyDecision decision = callDecideWithIncludeReasons(flagKey); + assertTrue(decision.getReasons().contains( + String.format("User with bucketingId \"tester\" is not in any experiment of group 13142870430.") + )); } + @Test public void decideReasons_userNotInExperiment() { String flagKey = "feature_1"; @@ -812,26 +967,65 @@ Map createUserProfileMap(String experimentId, String variationId } void setAudienceForFeatureTest(String flagKey, String audienceId) throws ConfigParseException { - ProjectConfig configReal = new DatafileProjectConfig.Builder().withDatafile(datafile).build(); - ProjectConfig config = spy(configReal); - optimizely = Optimizely.builder().withConfig(config).build(); + Experiment experiment = getSpyExperiment(flagKey); + when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + addSpyExperiment(experiment); + } + Experiment getSpyExperiment(String flagKey) { + setMockConfig(); String experimentId = config.getFeatureKeyMapping().get(flagKey).getExperimentIds().get(0); - String rolloutId = config.getFeatureKeyMapping().get(flagKey).getRolloutId(); - Map experimentIdMapping = new HashMap<>(config.getExperimentIdMapping()); - Map rolloutIdMapping = new HashMap<>(config.getRolloutIdMapping()); - Experiment experimentReal = experimentIdMapping.get(experimentId); - Rollout rolloutReal = rolloutIdMapping.get(rolloutId); - - Experiment experiment = spy(experimentReal); - Rollout rollout = spy(rolloutReal); - when(experiment.getAudienceIds()).thenReturn(Arrays.asList(audienceId)); + return spy(experimentIdMapping.get(experimentId)); + } - experimentIdMapping.put(experimentId, experiment); - rolloutIdMapping.put(rolloutId, rollout); + FeatureFlag getSpyFeatureFlag(String flagKey) { + setMockConfig(); + return spy(config.getFeatureKeyMapping().get(flagKey)); + } + + Group getSpyGroup(String groupId) { + setMockConfig(); + return spy(groupIdMapping.get(groupId)); + } + void addSpyExperiment(Experiment experiment) { + experimentIdMapping.put(experiment.getId(), experiment); when(config.getExperimentIdMapping()).thenReturn(experimentIdMapping); - when(config.getRolloutIdMapping()).thenReturn(rolloutIdMapping); + } + + void addSpyFeatureFlag(FeatureFlag flag) { + featureKeyMapping.put(flag.getKey(), flag); + when(config.getFeatureKeyMapping()).thenReturn(featureKeyMapping); + } + + void addSpyGroup(Group group) { + groupIdMapping.put(group.getId(), group); + when(config.getGroupIdMapping()).thenReturn(groupIdMapping); + } + + void setMockConfig() { + if (config != null) return; + + ProjectConfig configReal = null; + try { + configReal = new DatafileProjectConfig.Builder().withDatafile(datafile).build(); + config = spy(configReal); + optimizely = Optimizely.builder().withConfig(config).build(); + experimentIdMapping = new HashMap<>(config.getExperimentIdMapping()); + groupIdMapping = new HashMap<>(config.getGroupIdMapping()); + featureKeyMapping = new HashMap<>(config.getFeatureKeyMapping()); + } catch (ConfigParseException e) { + fail("ProjectConfig build failed"); + } + } + + OptimizelyDecision callDecideWithIncludeReasons(String flagKey, Map attributes) { + OptimizelyUserContext user = optimizely.createUserContext(userId, attributes); + return user.decide(flagKey, Arrays.asList(OptimizelyDecideOption.INCLUDE_REASONS)); + } + + OptimizelyDecision callDecideWithIncludeReasons(String flagKey) { + return callDecideWithIncludeReasons(flagKey, Collections.emptyMap()); } } From 7ce768d61befa0ce35d370126cc498e8b6b5856a Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 16 Oct 2020 09:46:42 -0700 Subject: [PATCH 16/17] pass options to DecisonReasons --- .../java/com/optimizely/ab/OptimizelyUserContext.java | 3 +-- .../ab/optimizelydecision/DecisionReasons.java | 10 ++++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java index ac2028da4..e760a0c25 100644 --- a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java @@ -102,8 +102,7 @@ public OptimizelyDecision decide(@Nonnull String key, Boolean sentEvent = false; Boolean flagEnabled = false; List allOptions = getAllOptions(options); - Boolean includeReasons = allOptions.contains(OptimizelyDecideOption.INCLUDE_REASONS); - DecisionReasons decisionReasons = new DecisionReasons(includeReasons); + DecisionReasons decisionReasons = new DecisionReasons(allOptions); Map copiedAttributes = new HashMap<>(attributes); FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( diff --git a/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java index ca8bf7c12..0a25b29eb 100644 --- a/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java +++ b/core-api/src/main/java/com/optimizely/ab/optimizelydecision/DecisionReasons.java @@ -16,21 +16,23 @@ */ package com.optimizely.ab.optimizelydecision; +import javax.annotation.Nonnull; import java.util.ArrayList; +import java.util.Collections; import java.util.List; public class DecisionReasons { - private boolean includeReasons; private final List errors = new ArrayList<>(); private final List logs = new ArrayList<>(); + private boolean includeReasons; - public DecisionReasons(boolean includeReasons) { - this.includeReasons = includeReasons; + public DecisionReasons(@Nonnull List options) { + this.includeReasons = options.contains(OptimizelyDecideOption.INCLUDE_REASONS); } public DecisionReasons() { - this(false); + this(Collections.emptyList()); } public void addError(String message) { From 5b6e79869e5a32110cbf1b28f3315a44581072ab Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 16 Oct 2020 10:31:31 -0700 Subject: [PATCH 17/17] move decide core to optimizely --- .../java/com/optimizely/ab/Optimizely.java | 174 +++++++++++++++++- .../optimizely/ab/OptimizelyUserContext.java | 171 +---------------- 2 files changed, 183 insertions(+), 162 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 26fe4a6b9..aa3214dd6 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -34,8 +34,11 @@ import com.optimizely.ab.optimizelyconfig.OptimizelyConfig; import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager; import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService; -import com.optimizely.ab.optimizelyjson.OptimizelyJSON; +import com.optimizely.ab.optimizelydecision.DecisionMessage; +import com.optimizely.ab.optimizelydecision.DecisionReasons; import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; +import com.optimizely.ab.optimizelydecision.OptimizelyDecision; +import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -1089,6 +1092,8 @@ public OptimizelyConfig getOptimizelyConfig() { return new OptimizelyConfigService(projectConfig).getConfig(); } + //============ decide ============// + /** * Create a context of the user for which decision APIs will be called. * @@ -1107,6 +1112,173 @@ public OptimizelyUserContext createUserContext(@Nonnull String userId) { return new OptimizelyUserContext(this, userId); } + OptimizelyDecision decide(@Nonnull OptimizelyUserContext user, + @Nonnull String key, + @Nonnull List options) { + + ProjectConfig projectConfig = getProjectConfig(); + if (projectConfig == null) { + return OptimizelyDecision.createErrorDecision(key, user, DecisionMessage.SDK_NOT_READY.reason()); + } + + FeatureFlag flag = projectConfig.getFeatureKeyMapping().get(key); + if (flag == null) { + return OptimizelyDecision.createErrorDecision(key, user, DecisionMessage.FLAG_KEY_INVALID.reason(key)); + } + + String userId = user.getUserId(); + Map attributes = user.getAttributes(); + Boolean sentEvent = false; + Boolean flagEnabled = false; + List allOptions = getAllOptions(options); + DecisionReasons decisionReasons = new DecisionReasons(allOptions); + + Map copiedAttributes = new HashMap<>(attributes); + FeatureDecision flagDecision = decisionService.getVariationForFeature( + flag, + userId, + copiedAttributes, + projectConfig, + allOptions, + decisionReasons); + + if (flagDecision.variation != null) { + if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { + if (!allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) { + sendImpression( + projectConfig, + flagDecision.experiment, + userId, + copiedAttributes, + flagDecision.variation); + sentEvent = true; + } + } else { + String message = String.format("The user \"%s\" is not included in an experiment for flag \"%s\".", userId, key); + logger.info(message); + decisionReasons.addInfo(message); + } + if (flagDecision.variation.getFeatureEnabled()) { + flagEnabled = true; + } + } + + Map variableMap = new HashMap<>(); + if (!allOptions.contains(OptimizelyDecideOption.EXCLUDE_VARIABLES)) { + variableMap = getDecisionVariableMap( + flag, + flagDecision.variation, + flagEnabled, + decisionReasons); + } + + OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); + + List reasonsToReport = decisionReasons.toReport(); + String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; + // TODO: add ruleKey values when available later. use a copy of experimentKey until then. + String ruleKey = flagDecision.experiment != null ? flagDecision.experiment.getKey() : null; + + DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder() + .withUserId(userId) + .withAttributes(copiedAttributes) + .withFlagKey(key) + .withEnabled(flagEnabled) + .withVariables(variableMap) + .withVariationKey(variationKey) + .withRuleKey(ruleKey) + .withReasons(reasonsToReport) + .withDecisionEventDispatched(sentEvent) + .build(); + notificationCenter.send(decisionNotification); + + logger.info("Feature \"{}\" is enabled for user \"{}\"? {}", key, userId, flagEnabled); + + return new OptimizelyDecision( + variationKey, + flagEnabled, + optimizelyJSON, + ruleKey, + key, + user, + reasonsToReport); + } + + Map decideForKeys(@Nonnull OptimizelyUserContext user, + @Nonnull List keys, + @Nonnull List options) { + Map decisionMap = new HashMap<>(); + + ProjectConfig projectConfig = getProjectConfig(); + if (projectConfig == null) { + logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); + return decisionMap; + } + + if (keys.isEmpty()) return decisionMap; + + List allOptions = getAllOptions(options); + + for (String key : keys) { + OptimizelyDecision decision = decide(user, key, options); + if (!allOptions.contains(OptimizelyDecideOption.ENABLED_FLAGS_ONLY) || decision.getEnabled()) { + decisionMap.put(key, decision); + } + } + + return decisionMap; + } + + Map decideAll(@Nonnull OptimizelyUserContext user, + @Nonnull List options) { + Map decisionMap = new HashMap<>(); + + ProjectConfig projectConfig = getProjectConfig(); + if (projectConfig == null) { + logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); + return decisionMap; + } + + List allFlags = projectConfig.getFeatureFlags(); + List allFlagKeys = new ArrayList<>(); + for (int i = 0; i < allFlags.size(); i++) allFlagKeys.add(allFlags.get(i).getKey()); + + return decideForKeys(user, allFlagKeys, options); + } + + private List getAllOptions(List options) { + List copiedOptions = new ArrayList(defaultDecideOptions); + copiedOptions.addAll(options); + return copiedOptions; + } + + private Map getDecisionVariableMap(@Nonnull FeatureFlag flag, + @Nonnull Variation variation, + @Nonnull Boolean featureEnabled, + @Nonnull DecisionReasons decisionReasons) { + Map valuesMap = new HashMap(); + for (FeatureVariable variable : flag.getVariables()) { + String value = variable.getDefaultValue(); + if (featureEnabled) { + FeatureVariableUsageInstance instance = variation.getVariableIdToFeatureVariableUsageInstanceMap().get(variable.getId()); + if (instance != null) { + value = instance.getValue(); + } + } + + Object convertedValue = convertStringToType(value, variable.getType()); + if (convertedValue == null) { + decisionReasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey())); + } else if (convertedValue instanceof OptimizelyJSON) { + convertedValue = ((OptimizelyJSON) convertedValue).toMap(); + } + + valuesMap.put(variable.getKey(), convertedValue); + } + + return valuesMap; + } + /** * Helper method which makes separate copy of attributesMap variable and returns it * diff --git a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java index e760a0c25..5f65bca36 100644 --- a/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java +++ b/core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java @@ -16,19 +16,16 @@ */ package com.optimizely.ab; -import com.optimizely.ab.bucketing.FeatureDecision; -import com.optimizely.ab.config.*; -import com.optimizely.ab.notification.DecisionNotification; -import com.optimizely.ab.optimizelydecision.DecisionMessage; -import com.optimizely.ab.optimizelydecision.DecisionReasons; import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption; import com.optimizely.ab.optimizelydecision.OptimizelyDecision; -import com.optimizely.ab.optimizelyjson.OptimizelyJSON; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; -import java.util.*; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.concurrent.ConcurrentHashMap; public class OptimizelyUserContext { @@ -88,91 +85,7 @@ public void setAttribute(@Nonnull String key, @Nonnull Object value) { */ public OptimizelyDecision decide(@Nonnull String key, @Nonnull List options) { - - ProjectConfig projectConfig = optimizely.getProjectConfig(); - if (projectConfig == null) { - return OptimizelyDecision.createErrorDecision(key, this, DecisionMessage.SDK_NOT_READY.reason()); - } - - FeatureFlag flag = projectConfig.getFeatureKeyMapping().get(key); - if (flag == null) { - return OptimizelyDecision.createErrorDecision(key, this, DecisionMessage.FLAG_KEY_INVALID.reason(key)); - } - - Boolean sentEvent = false; - Boolean flagEnabled = false; - List allOptions = getAllOptions(options); - DecisionReasons decisionReasons = new DecisionReasons(allOptions); - - Map copiedAttributes = new HashMap<>(attributes); - FeatureDecision flagDecision = optimizely.decisionService.getVariationForFeature( - flag, - userId, - copiedAttributes, - projectConfig, - allOptions, - decisionReasons); - - if (flagDecision.variation != null) { - if (flagDecision.decisionSource.equals(FeatureDecision.DecisionSource.FEATURE_TEST)) { - if (!allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) { - optimizely.sendImpression( - projectConfig, - flagDecision.experiment, - userId, - copiedAttributes, - flagDecision.variation); - sentEvent = true; - } - } else { - String message = String.format("The user \"%s\" is not included in an experiment for flag \"%s\".", userId, key); - logger.info(message); - decisionReasons.addInfo(message); - } - if (flagDecision.variation.getFeatureEnabled()) { - flagEnabled = true; - } - } - - Map variableMap = new HashMap<>(); - if (!allOptions.contains(OptimizelyDecideOption.EXCLUDE_VARIABLES)) { - variableMap = getDecisionVariableMap( - flag, - flagDecision.variation, - flagEnabled, - decisionReasons); - } - - OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap); - - List reasonsToReport = decisionReasons.toReport(); - String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null; - // TODO: add ruleKey values when available later. use a copy of experimentKey until then. - String ruleKey = flagDecision.experiment != null ? flagDecision.experiment.getKey() : null; - - DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder() - .withUserId(userId) - .withAttributes(copiedAttributes) - .withFlagKey(key) - .withEnabled(flagEnabled) - .withVariables(variableMap) - .withVariationKey(variationKey) - .withRuleKey(ruleKey) - .withReasons(reasonsToReport) - .withDecisionEventDispatched(sentEvent) - .build(); - optimizely.notificationCenter.send(decisionNotification); - - logger.info("Feature \"{}\" is enabled for user \"{}\"? {}", key, userId, flagEnabled); - - return new OptimizelyDecision( - variationKey, - flagEnabled, - optimizelyJSON, - ruleKey, - key, - this, - reasonsToReport); + return optimizely.decide(this, key, options); } /** @@ -182,7 +95,7 @@ public OptimizelyDecision decide(@Nonnull String key, * @return A decision result. */ public OptimizelyDecision decide(String key) { - return decide(key, Collections.emptyList()); + return optimizely.decide(this, key, Collections.emptyList()); } /** @@ -197,26 +110,7 @@ public OptimizelyDecision decide(String key) { */ public Map decideForKeys(@Nonnull List keys, @Nonnull List options) { - Map decisionMap = new HashMap<>(); - - ProjectConfig projectConfig = optimizely.getProjectConfig(); - if (projectConfig == null) { - logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); - return decisionMap; - } - - if (keys.isEmpty()) return decisionMap; - - List allOptions = getAllOptions(options); - - for (String key : keys) { - OptimizelyDecision decision = decide(key, options); - if (!allOptions.contains(OptimizelyDecideOption.ENABLED_FLAGS_ONLY) || decision.getEnabled()) { - decisionMap.put(key, decision); - } - } - - return decisionMap; + return optimizely.decideForKeys(this, keys, options); } /** @@ -226,7 +120,7 @@ public Map decideForKeys(@Nonnull List keys, * @return All decision results mapped by flag keys. */ public Map decideForKeys(@Nonnull List keys) { - return decideForKeys(keys, Collections.emptyList()); + return optimizely.decideForKeys(this, keys, Collections.emptyList()); } /** @@ -236,19 +130,7 @@ public Map decideForKeys(@Nonnull List keys) * @return All decision results mapped by flag keys. */ public Map decideAll(@Nonnull List options) { - Map decisionMap = new HashMap<>(); - - ProjectConfig projectConfig = optimizely.getProjectConfig(); - if (projectConfig == null) { - logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); - return decisionMap; - } - - List allFlags = projectConfig.getFeatureFlags(); - List allFlagKeys = new ArrayList<>(); - for (int i = 0; i < allFlags.size(); i++) allFlagKeys.add(allFlags.get(i).getKey()); - - return decideForKeys(allFlagKeys, options); + return optimizely.decideAll(this, options); } /** @@ -257,7 +139,7 @@ public Map decideAll(@Nonnull List decideAll() { - return decideAll(Collections.emptyList()); + return optimizely.decideAll(this, Collections.emptyList()); } /** @@ -284,39 +166,6 @@ public void trackEvent(@Nonnull String eventName) throws UnknownEventTypeExcepti // Utils - private List getAllOptions(List options) { - List copiedOptions = new ArrayList(optimizely.defaultDecideOptions); - copiedOptions.addAll(options); - return copiedOptions; - } - - private Map getDecisionVariableMap(@Nonnull FeatureFlag flag, - @Nonnull Variation variation, - @Nonnull Boolean featureEnabled, - @Nonnull DecisionReasons decisionReasons) { - Map valuesMap = new HashMap(); - for (FeatureVariable variable : flag.getVariables()) { - String value = variable.getDefaultValue(); - if (featureEnabled) { - FeatureVariableUsageInstance instance = variation.getVariableIdToFeatureVariableUsageInstanceMap().get(variable.getId()); - if (instance != null) { - value = instance.getValue(); - } - } - - Object convertedValue = optimizely.convertStringToType(value, variable.getType()); - if (convertedValue == null) { - decisionReasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey())); - } else if (convertedValue instanceof OptimizelyJSON) { - convertedValue = ((OptimizelyJSON) convertedValue).toMap(); - } - - valuesMap.put(variable.getKey(), convertedValue); - } - - return valuesMap; - } - @Override public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) return false;