(U2C #9) support contextKind in rollouts/experiments (#110)

eli-darkly · web-flow · commit 5d88c20dbf1f · 2022-10-14T09:13:04.000-07:00
diff --git a/Makefile b/Makefile
@@ -11,13 +11,16 @@ TEMP_TEST_OUTPUT=/tmp/sse-contract-test-service.log
 
 # TEST_HARNESS_PARAMS can be set to add -skip parameters for any contract tests that cannot yet pass
 # Explanation of current skips:
+# - "evaluation" subtests involving attribute references: Haven't yet implemented attribute references.
+# - "evaluation/bucketing/secondary": The "secondary" behavior needs to be removed from contract tests.
 # - "evaluation/parameterized/prerequisites": Can't pass yet because prerequisite cycle detection is not implemented.
+# - "evaluation/parameterized/segment match": Haven't yet implemented context kinds in segments.
+# - "evaluation/parameterized/segment recursion": Haven't yet implemented segment recursion.
 # - various other "evaluation" subtests: These tests require context kind support.
 # - "events": These test suites will be unavailable until more of the U2C implementation is done.
 TEST_HARNESS_PARAMS := $(TEST_HARNESS_PARAMS) \
-	-skip 'evaluation/bucketing/bucket by non-key attribute' \
+	-skip 'evaluation/bucketing/bucket by non-key attribute/in rollouts/string value/complex attribute reference' \
 	-skip 'evaluation/bucketing/secondary' \
-	-skip 'evaluation/bucketing/selection of context' \
 	-skip 'evaluation/parameterized/attribute references' \
 	-skip 'evaluation/parameterized/bad attribute reference errors' \
 	-skip 'evaluation/parameterized/prerequisites' \
diff --git a/src/LaunchDarkly/Impl/Evaluation/Evaluator.php b/src/LaunchDarkly/Impl/Evaluation/Evaluator.php
@@ -190,9 +190,6 @@ private function clauseMatchesContext(Clause $clause, LDContext $context): bool
     private function segmentMatchesContext(Segment $segment, LDContext $context): bool
     {
         $key = $context->getKey();
-        if (!$key) {
-            return false;
-        }
         if (in_array($key, $segment->getIncluded(), true)) {
             return true;
         }
@@ -224,7 +221,14 @@ private function segmentRuleMatchesContext(
         }
         // All of the clauses are met. See if the user buckets in
         $bucketBy = $rule->getBucketBy() ?: 'key';
-        $bucket = EvaluatorBucketing::getBucketValueForContext($context, $segmentKey, $bucketBy, $segmentSalt, null);
+        $bucket = EvaluatorBucketing::getBucketValueForContext(
+            $context,
+            $rule->getRolloutContextKind(),
+            $segmentKey,
+            $bucketBy,
+            $segmentSalt,
+            null
+        );
         $weight = $rule->getWeight() / 100000.0;
         return $bucket < $weight;
     }
diff --git a/src/LaunchDarkly/Impl/Evaluation/EvaluatorBucketing.php b/src/LaunchDarkly/Impl/Evaluation/EvaluatorBucketing.php
@@ -29,30 +29,47 @@ public static function variationIndexForContext(
             return [null, false];
         }
         $variations = $rollout->getVariations();
-        if ($variations) {
-            $bucketBy = $rollout->getBucketBy() ?: "key";
-            $bucket = self::getBucketValueForContext($context, $_key, $bucketBy, $_salt, $rollout->getSeed());
-            $sum = 0.0;
-            foreach ($variations as $wv) {
-                $sum += $wv->getWeight() / 100000.0;
-                if ($bucket < $sum) {
-                    return [$wv->getVariation(), $rollout->isExperiment() && !$wv->isUntracked()];
-                }
+        if (count($variations) === 0) {
+            return [null, false];
+        }
+
+        $bucketBy = ($rollout->isExperiment() ? null : $rollout->getBucketBy()) ?: 'key';
+        $bucket = self::getBucketValueForContext(
+            $context,
+            $rollout->getContextKind(),
+            $_key,
+            $bucketBy,
+            $_salt,
+            $rollout->getSeed()
+        );
+        $experiment = $rollout->isExperiment() && $bucket >= 0;
+        // getBucketValueForContext returns a negative value if the context didn't exist, in which case we
+        // still end up returning the first bucket, but we will force the "in experiment" state to be false.
+
+        $sum = 0.0;
+        foreach ($variations as $wv) {
+            $sum += $wv->getWeight() / 100000.0;
+            if ($bucket < $sum) {
+                return [$wv->getVariation(), $experiment && !$wv->isUntracked()];
             }
-            $lastVariation = $variations[count($variations) - 1];
-            return [$lastVariation->getVariation(), $rollout->isExperiment() && !$lastVariation->isUntracked()];
         }
-        return [null, false];
+        $lastVariation = $variations[count($variations) - 1];
+        return [$lastVariation->getVariation(), $experiment && !$lastVariation->isUntracked()];
     }
 
     public static function getBucketValueForContext(
         LDContext $context,
-        string $_key,
+        ?string $contextKind,
+        string $key,
         string $attr,
-        ?string $_salt,
+        ?string $salt,
         ?int $seed
     ): float {
-        $contextValue = $context->get($attr);
+        $matchContext = $context->getIndividualContext($contextKind ?? LDContext::DEFAULT_KIND);
+        if ($matchContext === null) {
+            return -1;
+        }
+        $contextValue = $matchContext->get($attr);
         if ($contextValue === null) {
             return 0.0;
         }
@@ -65,7 +82,7 @@ public static function getBucketValueForContext(
         if (isset($seed)) {
             $prefix = (string) $seed;
         } else {
-            $prefix = $_key . "." . ($_salt ?: '');
+            $prefix = $key . "." . ($salt ?: '');
         }
         $hash = substr(sha1($prefix . "." . $idHash), 0, 15);
         $longVal = (int)base_convert($hash, 16, 10);
diff --git a/src/LaunchDarkly/Impl/Model/Rollout.php b/src/LaunchDarkly/Impl/Model/Rollout.php
@@ -21,17 +21,20 @@ class Rollout
     private ?string $_bucketBy = null;
     private string $_kind;
     private ?int $_seed = null;
+    private ?string $_contextKind = null;
 
     public function __construct(
         array $variations,
         ?string $bucketBy,
         ?string $kind = null,
-        ?int $seed = null
+        ?int $seed = null,
+        ?string $contextKind = null
     ) {
         $this->_variations = $variations;
         $this->_bucketBy = $bucketBy;
         $this->_kind = $kind ?: 'rollout';
         $this->_seed = $seed;
+        $this->_contextKind = $contextKind;
     }
 
     /**
@@ -44,7 +47,7 @@ public static function getDecoder(): \Closure
             $vars = array_map($decoder, $v['variations']);
             $bucket = $v['bucketBy'] ?? null;
             
-            return new Rollout($vars, $bucket, $v['kind'] ?? null, $v['seed'] ?? null);
+            return new Rollout($vars, $bucket, $v['kind'] ?? null, $v['seed'] ?? null, $v['contextKind'] ?? null);
         };
     }
 
@@ -70,4 +73,9 @@ public function isExperiment(): bool
     {
         return $this->_kind === self::KIND_EXPERIMENT;
     }
+
+    public function getContextKind(): ?string
+    {
+        return $this->_contextKind;
+    }
 }
diff --git a/src/LaunchDarkly/Impl/Model/SegmentRule.php b/src/LaunchDarkly/Impl/Model/SegmentRule.php
@@ -18,20 +18,23 @@ class SegmentRule
     private array $_clauses = [];
     private ?int $_weight = null;
     private ?string $_bucketBy = null;
+    private ?string $_rolloutContextKind = null;
 
-    public function __construct(array $clauses, ?int $weight, ?string $bucketBy)
+    public function __construct(array $clauses, ?int $weight, ?string $bucketBy, ?string $rolloutContextKind)
     {
         $this->_clauses = $clauses;
         $this->_weight = $weight;
         $this->_bucketBy = $bucketBy;
+        $this->_rolloutContextKind = $rolloutContextKind;
     }
 
     public static function getDecoder(): \Closure
     {
         return fn (array $v) => new SegmentRule(
             array_map(Clause::getDecoder(), $v['clauses'] ?: []),
             $v['weight'] ?? null,
-            $v['bucketBy'] ?? null
+            $v['bucketBy'] ?? null,
+            $v['rolloutContextKind'] ?? null
         );
     }
 
@@ -48,6 +51,11 @@ public function getBucketBy(): ?string
         return $this->_bucketBy;
     }
 
+    public function getRolloutContextKind(): ?string
+    {
+        return $this->_rolloutContextKind;
+    }
+
     public function getWeight(): ?int
     {
         return $this->_weight;
diff --git a/tests/Impl/Evaluation/EvaluatorBucketingTest.php b/tests/Impl/Evaluation/EvaluatorBucketingTest.php
@@ -15,8 +15,8 @@ public function testUsingSeedIsDifferentThanSalt()
         $key = 'flag-key';
         $attr = 'key';
         $salt = 'testing123';
-        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, null);
-        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, $seed);
+        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, null);
+        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, $seed);
 
         $this->assertNotEquals($contextPoint1, $contextPoint2);
     }
@@ -29,8 +29,8 @@ public function testDifferentSaltsProduceDifferentAssignment()
         $key = 'flag-key';
         $attr = 'key';
         $salt = 'testing123';
-        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, $seed1);
-        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, $seed2);
+        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, $seed1);
+        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, $seed2);
 
         $this->assertNotEquals($contextPoint1, $contextPoint2);
     }
@@ -42,9 +42,38 @@ public function testSameSeedIsDeterministic()
         $key = 'flag-key';
         $attr = 'key';
         $salt = 'testing123';
-        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, $seed);
-        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, $key, $attr, $salt, $seed);
+        $contextPoint1 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, $seed);
+        $contextPoint2 = EvaluatorBucketing::getBucketValueForContext($context, null, $key, $attr, $salt, $seed);
 
         $this->assertEquals($contextPoint1, $contextPoint2);
     }
+
+    public function testContextKindSelectsContext()
+    {
+        $seed = 357;
+        $context1 = LDContext::create('key1');
+        $context2 = LDContext::create('key2', 'kind2');
+        $multi = LDContext::createMulti($context1, $context2);
+
+        $key = 'flag-key';
+        $attr = 'key';
+        $salt = 'testing123';
+
+        $this->assertEquals(
+            EvaluatorBucketing::getBucketValueForContext($context1, null, $key, $attr, $salt, $seed),
+            EvaluatorBucketing::getBucketValueForContext($context1, 'user', $key, $attr, $salt, $seed)
+        );
+        $this->assertEquals(
+            EvaluatorBucketing::getBucketValueForContext($context1, null, $key, $attr, $salt, $seed),
+            EvaluatorBucketing::getBucketValueForContext($multi, 'user', $key, $attr, $salt, $seed)
+        );
+        $this->assertEquals(
+            EvaluatorBucketing::getBucketValueForContext($context2, 'kind2', $key, $attr, $salt, $seed),
+            EvaluatorBucketing::getBucketValueForContext($multi, 'kind2', $key, $attr, $salt, $seed)
+        );
+        $this->assertNotEquals(
+            EvaluatorBucketing::getBucketValueForContext($multi, 'user', $key, $attr, $salt, $seed),
+            EvaluatorBucketing::getBucketValueForContext($multi, 'kind2', $key, $attr, $salt, $seed)
+        );
+    }
 }
diff --git a/tests/Impl/Evaluation/EvaluatorFlagTest.php b/tests/Impl/Evaluation/EvaluatorFlagTest.php
@@ -242,7 +242,7 @@ public function testRolloutSelectsBucket()
         
         // 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(EvaluatorBucketing::getBucketValueForContext($context, $flagKey, "key", $salt, null) * 100000);
+        $bucketValue = floor(EvaluatorBucketing::getBucketValueForContext($context, null, $flagKey, "key", $salt, null) * 100000);
         self::assertGreaterThan(0, $bucketValue);
         self::assertLessThan(100000, $bucketValue);
 
@@ -272,7 +272,7 @@ public function testRolloutSelectsLastBucketIfBucketValueEqualsTotalWeight()
         $flagKey = 'flagkey';
         $salt = 'salt';
         
-        $bucketValue = floor(EvaluatorBucketing::getBucketValueForContext($context, $flagKey, "key", $salt, null) * 100000);
+        $bucketValue = floor(EvaluatorBucketing::getBucketValueForContext($context, null, $flagKey, "key", $salt, null) * 100000);
 
         // We'll construct a list of variations that stops right at the target bucket value
         $rollout = ModelBuilders::rolloutWithVariations(
diff --git a/tests/Impl/Evaluation/EvaluatorSegmentTest.php b/tests/Impl/Evaluation/EvaluatorSegmentTest.php
@@ -3,6 +3,7 @@
 namespace LaunchDarkly\Tests\Impl\Evaluation;
 
 use LaunchDarkly\Impl\Evaluation\Evaluator;
+use LaunchDarkly\Impl\Evaluation\EvaluatorBucketing;
 use LaunchDarkly\Impl\Model\Segment;
 use LaunchDarkly\LDContext;
 use LaunchDarkly\Tests\MockFeatureRequester;
@@ -70,39 +71,48 @@ public function testMatchingRuleWithZeroRollout()
     public function testRolloutCalculationCanBucketByKey()
     {
         $context = LDContext::builder('userkey')->name('Bob')->build();
-        $this->verifyRollout($context, 12551, 'test', 'salt', null);
+        $this->verifyRollout($context, $context, 12551, 'test', 'salt', null, null);
     }
 
     public function testRolloutCalculationCanBucketBySpecificAttribute()
     {
         $context = LDContext::builder('userkey')->name('Bob')->build();
-        $this->verifyRollout($context, 61691, 'test', 'salt', 'name');
+        $this->verifyRollout($context, $context, 61691, 'test', 'salt', 'name', null);
     }
 
-    private function verifyRollout(LDContext $context, int $expectedBucketValue, string $segmentKey, string $salt, ?string $bucketBy)
-    {
+    private function verifyRollout(
+        LDContext $evalContext,
+        LDContext $matchContext,
+        int $expectedBucketValue,
+        string $segmentKey,
+        string $salt,
+        ?string $bucketBy,
+        ?string $rolloutContextKind
+    ) {
         $segmentShouldMatch = ModelBuilders::segmentBuilder($segmentKey)
             ->salt($salt)
             ->rule(
                 ModelBuilders::segmentRuleBuilder()
-                    ->clause(ModelBuilders::clauseMatchingContext($context))
+                    ->clause(ModelBuilders::clauseMatchingContext($matchContext))
                     ->weight($expectedBucketValue + 1)
                     ->bucketBy($bucketBy)
+                    ->rolloutContextKind($rolloutContextKind)
                     ->build()
             )
             ->build();
         $segmentShouldNotMatch = ModelBuilders::segmentBuilder($segmentKey)
             ->salt($salt)
             ->rule(
                 ModelBuilders::segmentRuleBuilder()
-                    ->clause(ModelBuilders::clauseMatchingContext($context))
+                    ->clause(ModelBuilders::clauseMatchingContext($matchContext))
                     ->weight($expectedBucketValue)
                     ->bucketBy($bucketBy)
+                    ->rolloutContextKind($rolloutContextKind)
                     ->build()
             )
             ->build();
-        $this->assertTrue($this->segmentMatchesContext($segmentShouldMatch, $context));
-        $this->assertFalse($this->segmentMatchesContext($segmentShouldNotMatch, $context));
+        $this->assertTrue($this->segmentMatchesContext($segmentShouldMatch, $evalContext));
+        $this->assertFalse($this->segmentMatchesContext($segmentShouldNotMatch, $evalContext));
     }
 
     public function testMatchingRuleWithMultipleClauses()
@@ -119,6 +129,16 @@ public function testMatchingRuleWithMultipleClauses()
         $this->assertTrue(self::segmentMatchesContext($segment, $context));
     }
 
+    public function testRolloutUsesContextKind()
+    {
+        $context1 = LDContext::create('key1', 'kind1');
+        $context2 = LDContext::create('key2', 'kind2');
+        $multi = LDContext::createMulti($context1, $context2);
+        $expectedBucketValue = (int)(100000 *
+            EvaluatorBucketing::getBucketValueForContext($context2, 'kind2', 'test', 'key', 'salt', null));
+        $this->verifyRollout($multi, $context2, $expectedBucketValue, 'test', 'salt', null, 'kind2');
+    }
+
     public function testNonMatchingRuleWithMultipleClauses()
     {
         $segment = ModelBuilders::segmentBuilder('test')
diff --git a/tests/Impl/Evaluation/RolloutRandomizationConsistencyTest.php b/tests/Impl/Evaluation/RolloutRandomizationConsistencyTest.php
diff --git a/tests/SegmentRuleBuilder.php b/tests/SegmentRuleBuilder.php

Original file line number	Diff line number	Diff line change
`@@ -18,20 +18,23 @@ class SegmentRule`
`18`	`18`	`private array $_clauses = [];`
`19`	`19`	`private ?int $_weight = null;`
`20`	`20`	`private ?string $_bucketBy = null;`
	`21`	`+ private ?string $_rolloutContextKind = null;`
`21`	`22`
`22`		`- public function __construct(array $clauses, ?int $weight, ?string $bucketBy)`
	`23`	`+ public function __construct(array $clauses, ?int $weight, ?string $bucketBy, ?string $rolloutContextKind)`
`23`	`24`	`{`
`24`	`25`	`$this->_clauses = $clauses;`
`25`	`26`	`$this->_weight = $weight;`
`26`	`27`	`$this->_bucketBy = $bucketBy;`
	`28`	`+ $this->_rolloutContextKind = $rolloutContextKind;`
`27`	`29`	`}`
`28`	`30`
`29`	`31`	`public static function getDecoder(): \Closure`
`30`	`32`	`{`
`31`	`33`	`return fn (array $v) => new SegmentRule(`
`32`	`34`	`array_map(Clause::getDecoder(), $v['clauses'] ?: []),`
`33`	`35`	`$v['weight'] ?? null,`
`34`		`- $v['bucketBy'] ?? null`
	`36`	`+ $v['bucketBy'] ?? null,`
	`37`	`+ $v['rolloutContextKind'] ?? null`
`35`	`38`	`);`
`36`	`39`	`}`
`37`	`40`
`@@ -48,6 +51,11 @@ public function getBucketBy(): ?string`
`48`	`51`	`return $this->_bucketBy;`
`49`	`52`	`}`
`50`	`53`
	`54`	`+ public function getRolloutContextKind(): ?string`
	`55`	`+ {`
	`56`	`+ return $this->_rolloutContextKind;`
	`57`	`+ }`
	`58`	`+`
`51`	`59`	`public function getWeight(): ?int`
`52`	`60`	`{`
`53`	`61`	`return $this->_weight;`