Merge pull request #47 from launchdarkly/eb/ch43307/bucketing-fix

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

Author: eli-darkly

src/LaunchDarkly/VariationOrRollout.php

@@ -52,16 +52,23 @@ public function variationIndexForUser($user, $_key, $_salt)
     {
         if ($this->_variation !== null) {
             return $this->_variation;
-        } elseif ($this->_rollout !== null) {
+        }
+        $rollout = $this->_rollout;
+        if ($rollout === null) {
+            return null;
+        }
+        $variations = $rollout->getVariations();
+        if ($variations) {
             $bucketBy = $this->_rollout->getBucketBy() === null ? "key" : $this->_rollout->getBucketBy();
             $bucket = self::bucketUser($user, $_key, $bucketBy, $_salt);
             $sum = 0.0;
-            foreach ($this->_rollout->getVariations() as $wv) {
+            foreach ($variations as $wv) {
                 $sum += $wv->getWeight() / 100000.0;
                 if ($bucket < $sum) {
                     return $wv->getVariation();
                 }
             }
+            return $variations[count($variations) - 1]->getVariation();
         }
         return null;
     }

tests/FeatureFlagTest.php

@@ -8,6 +8,7 @@
 use LaunchDarkly\LDUser;
 use LaunchDarkly\LDUserBuilder;
 use LaunchDarkly\Segment;
+use LaunchDarkly\VariationOrRollout;
 
 const RULE_ID = 'ruleid';
 
@@ -610,6 +611,80 @@ public function testFlagReturnsErrorIfRuleHasRolloutWithNoVariations()
         self::assertEquals(array(), $result->getPrerequisiteEvents());
     }
 
+    public function testRolloutSelectsBucket()
+    {
+        $ub = new LDUserBuilder('userkey');
+        $user = $ub->build();
+        $flagKey = 'flagkey';
+        $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
+        $bucketValue = floor(VariationOrRollout::bucketUser($user, $flagKey, "key", $salt) * 100000);
+        self::assertGreaterThan(0, $bucketValue);
+        self::assertLessThan(100000, $bucketValue);
+
+        $badVariationA = 0;
+        $matchedVariation = 1;
+        $badVariationB = 2;
+        $rollout = array(
+            'variations' => array(
+                array('variation' => $badVariationA, 'weight' => $bucketValue), // end of bucket range is not inclusive, so it will *not* match the target value
+                array('variation' => $matchedVariation, 'weight' => 1), // size of this bucket is 1, so it only matches that specific value
+                array('variation' => $badVariationB, 'weight' => 100000 - ($bucketValue + 1))
+            )
+        );
+        $flag = FeatureFlag::decode(array(
+            'key' => $flagKey,
+            'version' => 1,
+            'deleted' => false,
+            'on' => true,
+            'offVariation' => null,
+            'targets' => array(),
+            'prerequisites' => array(),
+            'rules' => array(),
+            'fallthrough' => array('rollout' => $rollout),
+            'variations' => array('', '', ''),
+            'salt' => $salt
+        ));
+
+        $result = $flag->evaluate($user, null, static::$eventFactory);
+        self::assertSame($matchedVariation, $result->getDetail()->getVariationIndex());
+    }
+
+    public function testRolloutSelectsLastBucketIfBucketValueEqualsTotalWeight()
+    {
+        $ub = new LDUserBuilder('userkey');
+        $user = $ub->build();
+        $flagKey = 'flagkey';
+        $salt = 'salt';
+        
+        $bucketValue = floor(VariationOrRollout::bucketUser($user, $flagKey, "key", $salt) * 100000);
+
+        // We'll construct a list of variations that stops right at the target bucket value
+        $rollout = array(
+            'variations' => array(
+                array('variation' => 0, 'weight' => $bucketValue)
+            )
+        );
+        $flag = FeatureFlag::decode(array(
+            'key' => $flagKey,
+            'version' => 1,
+            'deleted' => false,
+            'on' => true,
+            'offVariation' => null,
+            'targets' => array(),
+            'prerequisites' => array(),
+            'rules' => array(),
+            'fallthrough' => array('rollout' => $rollout),
+            'variations' => array(''),
+            'salt' => $salt
+        ));
+
+        $result = $flag->evaluate($user, null, static::$eventFactory);
+        self::assertSame(0, $result->getDetail()->getVariationIndex());
+    }
+
     public function testRolloutCalculationBucketsByUserKeyByDefault()
     {
         $ub = new LDUserBuilder('userkey');