fix(decide): change to return reasons as a part of tuple in decision hierarchy (#415)

Author: jaeopt

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -1,5 +1,5 @@
 /****************************************************************************
- * Copyright 2016-2020, Optimizely, Inc. and contributors                   *
+ * Copyright 2016-2021, Optimizely, Inc. and contributors                   *
  *                                                                          *
  * Licensed under the Apache License, Version 2.0 (the "License");          *
  * you may not use this file except in compliance with the License.         *
@@ -34,11 +34,7 @@
 import com.optimizely.ab.optimizelyconfig.OptimizelyConfig;
 import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager;
 import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService;
-import com.optimizely.ab.optimizelydecision.DecisionMessage;
-import com.optimizely.ab.optimizelydecision.DecisionReasons;
-import com.optimizely.ab.optimizelydecision.DefaultDecisionReasons;
-import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption;
-import com.optimizely.ab.optimizelydecision.OptimizelyDecision;
+import com.optimizely.ab.optimizelydecision.*;
 import com.optimizely.ab.optimizelyjson.OptimizelyJSON;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -427,7 +423,7 @@ private Boolean isFeatureEnabled(@Nonnull ProjectConfig projectConfig,
 
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
         FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT;
-        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
         Boolean featureEnabled = false;
         SourceInfo sourceInfo = new RolloutSourceInfo();
         if (featureDecision.decisionSource != null) {
@@ -736,7 +732,7 @@ <T> T getFeatureVariableValueForType(@Nonnull String featureKey,
 
         String variableValue = variable.getDefaultValue();
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
-        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
         Boolean featureEnabled = false;
         if (featureDecision.variation != null) {
             if (featureDecision.variation.getFeatureEnabled()) {
@@ -869,7 +865,7 @@ public OptimizelyJSON getAllFeatureVariables(@Nonnull String featureKey,
         }
 
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
-        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
         Boolean featureEnabled = false;
         Variation variation = featureDecision.variation;
 
@@ -970,7 +966,7 @@ private Variation getVariation(@Nonnull ProjectConfig projectConfig,
                                    @Nonnull String userId,
                                    @Nonnull Map<String, ?> attributes) throws UnknownExperimentException {
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
-        Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig);
+        Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig).getResult();
 
         String notificationType = NotificationCenter.DecisionNotificationType.AB_TEST.toString();
 
@@ -1084,7 +1080,7 @@ public Variation getForcedVariation(@Nonnull String experimentKey,
             return null;
         }
 
-        return decisionService.getForcedVariation(experiment, userId);
+        return decisionService.getForcedVariation(experiment, userId).getResult();
     }
 
     /**
@@ -1182,13 +1178,14 @@ OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,
         DecisionReasons decisionReasons = DefaultDecisionReasons.newInstance(allOptions);
 
         Map<String, ?> copiedAttributes = new HashMap<>(attributes);
-        FeatureDecision flagDecision = decisionService.getVariationForFeature(
+        DecisionResponse<FeatureDecision> decisionVariation = decisionService.getVariationForFeature(
             flag,
             userId,
             copiedAttributes,
             projectConfig,
-            allOptions,
-            decisionReasons);
+            allOptions);
+        FeatureDecision flagDecision = decisionVariation.getResult();
+        decisionReasons.merge(decisionVariation.getReasons());
 
         Boolean flagEnabled = false;
         if (flagDecision.variation != null) {
@@ -1200,11 +1197,12 @@ OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,
 
         Map<String, Object> variableMap = new HashMap<>();
         if (!allOptions.contains(OptimizelyDecideOption.EXCLUDE_VARIABLES)) {
-            variableMap = getDecisionVariableMap(
+            DecisionResponse<Map<String, Object>> decisionVariables = getDecisionVariableMap(
                 flag,
                 flagDecision.variation,
-                flagEnabled,
-                decisionReasons);
+                flagEnabled);
+            variableMap = decisionVariables.getResult();
+            decisionReasons.merge(decisionVariables.getReasons());
         }
         OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap);
 
@@ -1305,10 +1303,12 @@ private List<OptimizelyDecideOption> getAllOptions(List<OptimizelyDecideOption>
         return copiedOptions;
     }
 
-    private Map<String, Object> getDecisionVariableMap(@Nonnull FeatureFlag flag,
-                                                       @Nonnull Variation variation,
-                                                       @Nonnull Boolean featureEnabled,
-                                                       @Nonnull DecisionReasons decisionReasons) {
+    @Nonnull
+    private DecisionResponse<Map<String, Object>> getDecisionVariableMap(@Nonnull FeatureFlag flag,
+                                                                         @Nonnull Variation variation,
+                                                                         @Nonnull Boolean featureEnabled) {
+        DecisionReasons reasons = new DecisionReasons();
+
         Map<String, Object> valuesMap = new HashMap<String, Object>();
         for (FeatureVariable variable : flag.getVariables()) {
             String value = variable.getDefaultValue();
@@ -1321,15 +1321,15 @@ private Map<String, Object> getDecisionVariableMap(@Nonnull FeatureFlag flag,
 
             Object convertedValue = convertStringToType(value, variable.getType());
             if (convertedValue == null) {
-                decisionReasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey()));
+                reasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey()));
             } else if (convertedValue instanceof OptimizelyJSON) {
                 convertedValue = ((OptimizelyJSON) convertedValue).toMap();
             }
 
             valuesMap.put(variable.getKey(), convertedValue);
         }
 
-        return valuesMap;
+        return new DecisionResponse(valuesMap, reasons);
     }
 
     /**

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

@@ -1,6 +1,6 @@
 /**
  *
- *    Copyright 2016-2017, 2019, Optimizely and contributors
+ *    Copyright 2016-2017, 2019-2021 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.
@@ -20,12 +20,12 @@
 import com.optimizely.ab.bucketing.internal.MurmurHash3;
 import com.optimizely.ab.config.*;
 import com.optimizely.ab.optimizelydecision.DecisionReasons;
+import com.optimizely.ab.optimizelydecision.DecisionResponse;
 import com.optimizely.ab.optimizelydecision.DefaultDecisionReasons;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import java.util.List;
 
@@ -69,8 +69,7 @@ private String bucketToEntity(int bucketValue, List<TrafficAllocation> trafficAl
 
     private Experiment bucketToExperiment(@Nonnull Group group,
                                           @Nonnull String bucketingId,
-                                          @Nonnull ProjectConfig projectConfig,
-                                          @Nonnull DecisionReasons reasons) {
+                                          @Nonnull ProjectConfig projectConfig) {
         // "salt" the bucket id using the group id
         String bucketKey = bucketingId + group.getId();
 
@@ -89,9 +88,11 @@ private Experiment bucketToExperiment(@Nonnull Group group,
         return null;
     }
 
-    private Variation bucketToVariation(@Nonnull Experiment experiment,
-                                        @Nonnull String bucketingId,
-                                        @Nonnull DecisionReasons reasons) {
+    @Nonnull
+    private DecisionResponse<Variation> bucketToVariation(@Nonnull Experiment experiment,
+                                                          @Nonnull String bucketingId) {
+        DecisionReasons reasons = DefaultDecisionReasons.newInstance();
+
         // "salt" the bucket id using the experiment id
         String experimentId = experiment.getId();
         String experimentKey = experiment.getKey();
@@ -111,13 +112,13 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
                 experimentKey);
             logger.info(message);
 
-            return bucketedVariation;
+            return new DecisionResponse(bucketedVariation, reasons);
         }
 
         // user was not bucketed to a variation
         String message = reasons.addInfo("User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", bucketingId, experimentKey);
         logger.info(message);
-        return null;
+        return new DecisionResponse(null, reasons);
     }
 
     /**
@@ -126,26 +127,26 @@ 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 reasons            Decision log messages
-     * @return Variation the user is bucketed into or null.
+     * @return A {@link DecisionResponse} including the {@link Variation} that user is bucketed into (or null) and the decision reasons
      */
-    @Nullable
-    public Variation bucket(@Nonnull Experiment experiment,
-                            @Nonnull String bucketingId,
-                            @Nonnull ProjectConfig projectConfig,
-                            @Nonnull DecisionReasons reasons) {
+    @Nonnull
+    public DecisionResponse<Variation> bucket(@Nonnull Experiment experiment,
+                                              @Nonnull String bucketingId,
+                                              @Nonnull ProjectConfig projectConfig) {
+        DecisionReasons reasons = DefaultDecisionReasons.newInstance();
+
         // ---------- Bucket User ----------
         String groupId = experiment.getGroupId();
         // check whether the experiment belongs to a group
         if (!groupId.isEmpty()) {
             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, reasons);
+                Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig);
                 if (bucketedExperiment == null) {
                     String message = reasons.addInfo("User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId());
                     logger.info(message);
-                    return null;
+                    return new DecisionResponse(null, reasons);
                 } else {
 
                 }
@@ -155,7 +156,7 @@ public Variation bucket(@Nonnull Experiment experiment,
                     String message = reasons.addInfo("User with bucketingId \"%s\" is not in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(),
                         experimentGroup.getId());
                     logger.info(message);
-                    return null;
+                    return new DecisionResponse(null, reasons);
                 }
 
                 String message = reasons.addInfo("User with bucketingId \"%s\" is in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(),
@@ -164,14 +165,9 @@ public Variation bucket(@Nonnull Experiment experiment,
             }
         }
 
-        return bucketToVariation(experiment, bucketingId, reasons);
-    }
-
-    @Nullable
-    public Variation bucket(@Nonnull Experiment experiment,
-                            @Nonnull String bucketingId,
-                            @Nonnull ProjectConfig projectConfig) {
-        return bucket(experiment, bucketingId, projectConfig, DefaultDecisionReasons.newInstance());
+        DecisionResponse<Variation> decisionResponse = bucketToVariation(experiment, bucketingId);
+        reasons.merge(decisionResponse.getReasons());
+        return new DecisionResponse<>(decisionResponse.getResult(), reasons);
     }
 
     //======== Helper methods ========//