revise rollout bucketing to check audience in last rollout rule (#174)

* revise rollout bucketing to check audience in last rollout rule

* remove unused methods.

* remove hash on decision payload object

* add test to make sure we are using bucketing ID in rollouts bucketing as well

* bring back some log messages

* add hashcode back

* remove comment

Author: wangjoshuah

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

@@ -219,32 +219,29 @@ public DecisionService(@Nonnull Bucketer bucketer,
             Experiment rolloutRule = rollout.getExperiments().get(i);
             Audience audience = projectConfig.getAudienceIdMapping().get(rolloutRule.getAudienceIds().get(0));
             if (ExperimentUtils.isUserInExperiment(projectConfig, rolloutRule, filteredAttributes)) {
-                logger.debug("Attempting to bucket user \"" + userId +
-                        "\" into rollout rule for audience \"" + audience.getName() +
-                        "\".");
                 variation = bucketer.bucket(rolloutRule, bucketingId);
                 if (variation == null) {
-                    logger.debug("User \"{}\" was excluded due to traffic allocation.", userId);
                     break;
                 }
                 return new FeatureDecision(rolloutRule, variation,
                         FeatureDecision.DecisionSource.ROLLOUT);
-            } else {
+            }
+            else {
                 logger.debug("User \"{}\" did not meet the conditions to be in rollout rule for audience \"{}\".",
                         userId, audience.getName());
             }
         }
 
-        // get last rule which is the everyone else rule
-        Experiment everyoneElseRule = rollout.getExperiments().get(rolloutRulesLength - 1);
-        variation = bucketer.bucket(everyoneElseRule, bucketingId); // ignore audience
-        if (variation == null) {
-            logger.debug("User \"{}\" was excluded from the \"Everyone Else\" rule for feature flag \"{}\".",
-                    userId, featureFlag.getKey());
-            return new FeatureDecision(null, null, null);
+        // get last rule which is the fall back rule
+        Experiment finalRule = rollout.getExperiments().get(rolloutRulesLength - 1);
+        if (ExperimentUtils.isUserInExperiment(projectConfig, finalRule, filteredAttributes)) {
+            variation = bucketer.bucket(finalRule, bucketingId);
+            if (variation != null) {
+                return new FeatureDecision(finalRule, variation,
+                        FeatureDecision.DecisionSource.ROLLOUT);
+            }
         }
-        return new FeatureDecision(everyoneElseRule, variation,
-                FeatureDecision.DecisionSource.ROLLOUT);
+        return new FeatureDecision(null, null, null);
     }
 
     /**

core-api/src/main/java/com/optimizely/ab/config/audience/Audience.java

@@ -19,7 +19,6 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
-
 import com.optimizely.ab.config.IdKeyMapped;
 
 import javax.annotation.concurrent.Immutable;
@@ -50,11 +49,11 @@ public String getId() {
         return id;
     }
 
-    public String getName() {
+    public String getKey() {
         return name;
     }
 
-    public String getKey() {
+    public String getName() {
         return name;
     }
 

core-api/src/main/java/com/optimizely/ab/event/internal/payload/Decision.java

@@ -18,6 +18,8 @@
 
 import com.fasterxml.jackson.annotation.JsonProperty;
 
+import java.util.Objects;
+
 public class Decision {
     @JsonProperty("campaign_id")
     String campaignId;
@@ -43,34 +45,18 @@ public String getCampaignId() {
         return campaignId;
     }
 
-    public void setCampaignId(String campaignId) {
-        this.campaignId = campaignId;
-    }
-
     public String getExperimentId() {
         return experimentId;
     }
 
-    public void setExperimentId(String experimentId) {
-        this.experimentId = experimentId;
-    }
-
     public String getVariationId() {
         return variationId;
     }
 
-    public void setVariationId(String variationId) {
-        this.variationId = variationId;
-    }
-
     public boolean getIsCampaignHoldback() {
         return isCampaignHoldback;
     }
 
-    public void setIsCampaignHoldback(boolean campaignHoldback) {
-        isCampaignHoldback = campaignHoldback;
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
@@ -83,7 +69,6 @@ public boolean equals(Object o) {
         if (!experimentId.equals(that.experimentId)) return false;
         return variationId.equals(that.variationId);
     }
-
     @Override
     public int hashCode() {
         int result = campaignId.hashCode();

core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java

@@ -50,8 +50,11 @@
 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 org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
@@ -1053,4 +1056,39 @@ public void getVariationBucketingId() throws Exception {
 
     }
 
+    /**
+    Verify that {@link DecisionService#getVariationForFeatureInRollout(FeatureFlag, String, Map)}
+     uses bucketing ID to bucket the user into rollouts.
+     */
+    @Test
+    public void getVariationForRolloutWithBucketingId() {
+        Experiment rolloutRuleExperiment = ROLLOUT_3_EVERYONE_ELSE_RULE;
+        Variation rolloutVariation = ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION;
+        FeatureFlag featureFlag = FEATURE_FLAG_SINGLE_VARIABLE_INTEGER;
+        String bucketingId = "user_bucketing_id";
+        String userId = "user_id";
+        Map<String, String> attributes = new HashMap<String, String>();
+        attributes.put(DecisionService.BUCKETING_ATTRIBUTE, bucketingId);
+
+        Bucketer bucketer = mock(Bucketer.class);
+        when(bucketer.bucket(rolloutRuleExperiment, userId)).thenReturn(null);
+        when(bucketer.bucket(rolloutRuleExperiment, bucketingId)).thenReturn(rolloutVariation);
+
+        DecisionService decisionService = spy(new DecisionService(
+                bucketer,
+                mockErrorHandler,
+                v4ProjectConfig,
+                null
+        ));
+
+        FeatureDecision expectedFeatureDecision = new FeatureDecision(
+                rolloutRuleExperiment,
+                rolloutVariation,
+                FeatureDecision.DecisionSource.ROLLOUT);
+
+        FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, attributes);
+
+        assertEquals(expectedFeatureDecision, featureDecision);
+    }
+
 }

core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java

@@ -227,14 +227,14 @@ public class ValidProjectConfigV4 {
     private static final String     ROLLOUT_3_ID = "2048875663";
     private static final String     ROLLOUT_3_EVERYONE_ELSE_EXPERIMENT_ID = "3794675122";
     private static final String     ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION_ID = "589640735";
-    private  static final Boolean   ROLLOUT_3_FEATURE_ENABLED_VALUE = true;
-    private static final Variation  ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION = new Variation(
+    private static final Boolean   ROLLOUT_3_FEATURE_ENABLED_VALUE = true;
+    public  static final Variation  ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION = new Variation(
             ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION_ID,
             ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION_ID,
             ROLLOUT_3_FEATURE_ENABLED_VALUE,
             Collections.<LiveVariableUsageInstance>emptyList()
     );
-    private static final Experiment ROLLOUT_3_EVERYONE_ELSE_RULE = new Experiment(
+    public  static final Experiment ROLLOUT_3_EVERYONE_ELSE_RULE = new Experiment(
             ROLLOUT_3_EVERYONE_ELSE_EXPERIMENT_ID,
             ROLLOUT_3_EVERYONE_ELSE_EXPERIMENT_ID,
             Experiment.ExperimentStatus.RUNNING.toString(),