Merge pull request #165 from launchdarkly/bw/ch43307/bucket-issue

don't let user fall outside of last bucket in rollout

evaluate_flag.js

@@ -295,18 +295,30 @@ function variationForUser(r, user, flag) {
   if (r.variation != null) {
     // This represets a fixed variation; return it
     return r.variation;
-  } else if (r.rollout != null) {
-    // This represents a percentage rollout. Assume
-    // we're rolling out by key
-    const bucketBy = r.rollout.bucketBy != null ? r.rollout.bucketBy : 'key';
-    const bucket = bucketUser(user, flag.key, bucketBy, flag.salt);
-    let sum = 0;
-    for (let i = 0; i < r.rollout.variations.length; i++) {
-      const variate = r.rollout.variations[i];
-      sum += variate.weight / 100000.0;
-      if (bucket < sum) {
-        return variate.variation;
+  }
+  const rollout = r.rollout;
+  if (rollout) {
+    const variations = rollout.variations;
+    if (variations && variations.length > 0) {
+      // This represents a percentage rollout. Assume
+      // we're rolling out by key
+      const bucketBy = rollout.bucketBy || 'key';
+      const bucket = bucketUser(user, flag.key, bucketBy, flag.salt);
+      let sum = 0;
+      for (let i = 0; i < variations.length; i++) {
+        const variate = variations[i];
+        sum += variate.weight / 100000.0;
+        if (bucket < sum) {
+          return variate.variation;
+        }
       }
+
+      // 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 variations[variations.length - 1].variation;
     }
   }
 

test/evaluate_flag-test.js

@@ -696,6 +696,66 @@ describe('evaluate', () => {
   });
 });
 
+describe('rollout', () => {
+  it('selects bucket', done => {
+    const user = { key: 'userkey' };
+    const flagKey = 'flagkey';
+    const 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
+    const bucketValue = Math.floor(evaluate.bucketUser(user, flagKey, 'key', salt) * 100000);
+    expect(bucketValue).toBeGreaterThan(0);
+    expect(bucketValue).toBeLessThan(100000);
+
+    const badVariationA = 0, matchedVariation = 1, badVariationB = 2;
+    const rollout = {
+      variations: [
+        { variation: badVariationA, weight: bucketValue }, // end of bucket range is not inclusive, so it will *not* match the target value
+        { variation: matchedVariation, weight: 1 }, // size of this bucket is 1, so it only matches that specific value
+        { variation: badVariationB, weight: 100000 - (bucketValue + 1) }
+      ]
+    };
+    const flag = {
+      key: flagKey,
+      salt: salt,
+      on: true,
+      fallthrough: { rollout: rollout },
+      variations: [ null, null, null ]
+    };
+    evaluate.evaluate(flag, user, featureStore, eventFactory, (err, detail) => {
+      expect(err).toEqual(null);
+      expect(detail.variationIndex).toEqual(matchedVariation);
+      done();
+    });
+  });
+
+  it('uses last bucket if bucket value is equal to total weight', done => {
+    const user = { key: 'userkey' };
+    const flagKey = 'flagkey';
+    const salt = 'salt';
+
+    // We'll construct a list of variations that stops right at the target bucket value
+    const bucketValue = Math.floor(evaluate.bucketUser(user, flagKey, 'key', salt) * 100000);
+
+    const rollout = {
+      variations: [ { variation: 0, weight: bucketValue }]
+    };
+    const flag = {
+      key: flagKey,
+      salt: salt,
+      on: true,
+      fallthrough: { rollout: rollout },
+      variations: [ null, null, null ]
+    };
+    evaluate.evaluate(flag, user, featureStore, eventFactory, (err, detail) => {
+      expect(err).toEqual(null);
+      expect(detail.variationIndex).toEqual(0);
+      done();
+    });
+  });
+});
+
 describe('bucketUser', () => {
   it('gets expected bucket values for specific keys', () => {
     var user = { key: 'userKeyA' };