Fixed EvaluatorBucketing Index Selection Bug

src/main/java/com/launchdarkly/sdk/server/EvaluatorBucketing.java

@@ -11,11 +11,11 @@
  */
 abstract class EvaluatorBucketing {
   private EvaluatorBucketing() {}
-  
+
   private static final float LONG_SCALE = (float) 0xFFFFFFFFFFFFFFFL;
 
   // Attempt to determine the variation index for a given user. Returns null if no index can be computed
-  // due to internal inconsistency of the data (i.e. a malformed flag). 
+  // due to internal inconsistency of the data (i.e. a malformed flag).
   static Integer variationIndexForUser(DataModel.VariationOrRollout vr, LDUser user, String key, String salt) {
     Integer variation = vr.getVariation();
     if (variation != null) {
@@ -25,18 +25,25 @@ static Integer variationIndexForUser(DataModel.VariationOrRollout vr, LDUser use
       if (rollout != null && !rollout.getVariations().isEmpty()) {
         float bucket = bucketUser(user, key, rollout.getBucketBy(), salt);
         float sum = 0F;
+        int lastNonZeroWeightedVariationIndex = rollout.getVariations().size() - 1; // Default to last.
+        int currentIndex = 0;
         for (DataModel.WeightedVariation wv : rollout.getVariations()) {
           sum += (float) wv.getWeight() / 100000F;
+          if (wv.getWeight() > 0) {
+            lastNonZeroWeightedVariationIndex = currentIndex;
+          }
           if (bucket < sum) {
             return wv.getVariation();
           }
+          currentIndex++;
         }
         // The user's bucket value was greater than or equal to the end of the last bucket. This could happen due
         // to a rounding error, or due to the fact that we are scaling to 100000 rather than 99999, or the flag
         // data could contain buckets that don't actually add up to 100000. Rather than returning an error in
         // this case (or changing the scaling, which would potentially change the results for *all* users), we
-        // will simply put the user in the last bucket.
-        return rollout.getVariations().get(rollout.getVariations().size() - 1).getVariation();
+        // will simply put the user in the last non-zero weighted bucket (or the last bucket, if all buckets
+        // were zero-weighted).
+        return rollout.getVariations().get(lastNonZeroWeightedVariationIndex).getVariation();
       }
     }
     return null;
@@ -57,7 +64,7 @@ static float bucketUser(LDUser user, String key, UserAttribute attr, String salt
   }
 
   private static String getBucketableStringValue(LDValue userValue) {
-    switch (userValue.getType()) { 
+    switch (userValue.getType()) {
     case STRING:
       return userValue.stringValue();
     case NUMBER:

src/test/java/com/launchdarkly/sdk/server/EvaluatorBucketingTest.java

@@ -24,20 +24,46 @@ public void variationIndexIsReturnedForBucket() {
     LDUser user = new LDUser.Builder("userkey").build();
     String flagKey = "flagkey";
     String salt = "salt";
-    
+
     // First verify that with our test inputs, the bucket value will be greater than zero and less than 100000,
     // so we can construct a rollout whose second bucket just barely contains that value
     int bucketValue = (int)(EvaluatorBucketing.bucketUser(user, flagKey, UserAttribute.KEY, salt) * 100000);
     assertThat(bucketValue, greaterThanOrEqualTo(1));
     assertThat(bucketValue, Matchers.lessThan(100000));
-    
+
     int badVariationA = 0, matchedVariation = 1, badVariationB = 2;
     List<WeightedVariation> variations = Arrays.asList(
         new WeightedVariation(badVariationA, bucketValue), // end of bucket range is not inclusive, so it will *not* match the target value
         new WeightedVariation(matchedVariation, 1), // size of this bucket is 1, so it only matches that specific value
         new WeightedVariation(badVariationB, 100000 - (bucketValue + 1)));
     VariationOrRollout vr = new VariationOrRollout(null, new Rollout(variations, null));
-
+
+    Integer resultVariation = EvaluatorBucketing.variationIndexForUser(vr, user, flagKey, salt);
+    assertEquals(Integer.valueOf(matchedVariation), resultVariation);
+  }
+
+  @Test
+  public void variationIndexIgnoresTrailingZeroWeightedBuckets() {
+    LDUser user = new LDUser.Builder("userkey").build();
+    String flagKey = "flagkey";
+    String salt = "salt";
+
+    // Generate the bucket value so we can artificially construct a set of buckets that will exclude this value.
+    // In theory these could fail, but SHA1 is static so we'll just pick good constants.
+    int bucketValue = (int)(EvaluatorBucketing.bucketUser(user, flagKey, UserAttribute.KEY, salt) * 100000);
+    assertThat(bucketValue, greaterThanOrEqualTo(2));
+    assertThat(bucketValue, Matchers.lessThan(999999));
+
+    int badVariation = 0, matchedVariation = 1, ignoredVariationA = 2, ignoredVariationB = 3;
+    List<WeightedVariation> variations = Arrays.asList(
+        new WeightedVariation(badVariation, bucketValue - 1), // Two before valid bucket range.
+        new WeightedVariation(matchedVariation, bucketValue), // One before valid bucket range.
+        new WeightedVariation(ignoredVariationA, 0),
+        new WeightedVariation(ignoredVariationB, 0));
+    VariationOrRollout vr = new VariationOrRollout(null, new Rollout(variations, null));
+
+    // No bucket contained the value, but we should still see the last non-zero weighted bucket as
+    // the index we choose.
     Integer resultVariation = EvaluatorBucketing.variationIndexForUser(vr, user, flagKey, salt);
     assertEquals(Integer.valueOf(matchedVariation), resultVariation);
   }
@@ -50,10 +76,10 @@ public void lastBucketIsUsedIfBucketValueEqualsTotalWeight() {
 
     // We'll construct a list of variations that stops right at the target bucket value
     int bucketValue = (int)(EvaluatorBucketing.bucketUser(user, flagKey, UserAttribute.KEY, salt) * 100000);
-    
+
     List<WeightedVariation> variations = Arrays.asList(new WeightedVariation(0, bucketValue));
     VariationOrRollout vr = new VariationOrRollout(null, new Rollout(variations, null));
-    
+
     Integer resultVariation = EvaluatorBucketing.variationIndexForUser(vr, user, flagKey, salt);
     assertEquals(Integer.valueOf(0), resultVariation);
   }