diff --git a/src/main/java/com/launchdarkly/sdk/server/EvaluatorBucketing.java b/src/main/java/com/launchdarkly/sdk/server/EvaluatorBucketing.java index 6f6891dff..2dea929c2 100644 --- a/src/main/java/com/launchdarkly/sdk/server/EvaluatorBucketing.java +++ b/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: diff --git a/src/test/java/com/launchdarkly/sdk/server/EvaluatorBucketingTest.java b/src/test/java/com/launchdarkly/sdk/server/EvaluatorBucketingTest.java index 2e245874d..33936083a 100644 --- a/src/test/java/com/launchdarkly/sdk/server/EvaluatorBucketingTest.java +++ b/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 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 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 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); }