Skip to content

Commit de1ffe0

Browse files
eli-darklyeli-darkly
authored
(U2C #16) implement segment recursion and segment cycle detection (#117)
* remove LDUser and LDUserBuilder * implement prerequisite cycle detection * lint * rm unused * rm debugging * implement segment recursion and segment cycle detection
1 parent e821ab8 commit de1ffe0

File tree

5 files changed

+122
-21
lines changed

5 files changed

+122
-21
lines changed

‎Makefile‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,9 @@ TEMP_TEST_OUTPUT=/tmp/sse-contract-test-service.log
1515
# - "evaluation/parameterized/attribute references/array index is not supported": Due to how PHP
1616
# arrays work, there's no way to disallow an array index lookup without breaking object property
1717
# lookups for properties that are numeric strings.
18-
# - "evaluation/parameterized/segment recursion": Haven't yet implemented segment recursion.
1918
TEST_HARNESS_PARAMS := $(TEST_HARNESS_PARAMS) \
2019
-skip 'evaluation/bucketing/secondary' \
21-
-skip 'evaluation/parameterized/attribute references/array index is not supported' \
22-
-skip 'evaluation/parameterized/segment recursion'
20+
-skip 'evaluation/parameterized/attribute references/array index is not supported'
2321

2422
build-contract-tests:
2523
@cd test-service && composer install --no-progress

‎src/LaunchDarkly/Impl/Evaluation/Evaluator.php‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
class EvaluatorState
2222
{
2323
public ?array $prerequisiteStack = null;
24+
public ?array $segmentStack = null;
2425

2526
public function __construct(public FeatureFlag $originalFlag)
2627
{
@@ -94,7 +95,7 @@ private function evaluateInternal(
9495

9596
// Now walk through the rules and see if any match
9697
foreach ($flag->getRules() as $i => $rule) {
97-
if ($this->ruleMatchesContext($rule, $context)) {
98+
if ($this->ruleMatchesContext($rule, $context, $state)) {
9899
return EvaluatorHelpers::getResultForVariationOrRollout(
99100
$flag,
100101
$rule,
@@ -214,23 +215,29 @@ private function checkTargets(FeatureFlag $flag, LDContext $context): ?EvalResul
214215
return null;
215216
}
216217

217-
private function ruleMatchesContext(Rule $rule, LDContext $context): bool
218+
private function ruleMatchesContext(Rule $rule, LDContext $context, EvaluatorState $state): bool
218219
{
219220
foreach ($rule->getClauses() as $clause) {
220-
if (!$this->clauseMatchesContext($clause, $context)) {
221+
if (!$this->clauseMatchesContext($clause, $context, $state)) {
221222
return false;
222223
}
223224
}
224225
return true;
225226
}
226227

227-
private function clauseMatchesContext(Clause $clause, LDContext $context): bool
228+
private function clauseMatchesContext(Clause $clause, LDContext $context, EvaluatorState $state): bool
228229
{
229230
if ($clause->getOp() === 'segmentMatch') {
230-
foreach ($clause->getValues() as $value) {
231-
$segment = $this->_featureRequester->getSegment($value);
231+
foreach ($clause->getValues() as $segmentKey) {
232+
if ($state->segmentStack !== null && in_array($segmentKey, $state->segmentStack)) {
233+
throw new EvaluationException(
234+
"segment rule referencing segment \"$segmentKey\" caused a circular reference; this is probably a temporary condition due to an incomplete update",
235+
EvaluationReason::MALFORMED_FLAG_ERROR
236+
);
237+
}
238+
$segment = $this->_featureRequester->getSegment($segmentKey);
232239
if ($segment) {
233-
if ($this->segmentMatchesContext($segment, $context)) {
240+
if ($this->segmentMatchesContext($segment, $context, $state)) {
234241
return EvaluatorHelpers::maybeNegate($clause, true);
235242
}
236243
}
@@ -240,7 +247,7 @@ private function clauseMatchesContext(Clause $clause, LDContext $context): bool
240247
return EvaluatorHelpers::matchClauseWithoutSegments($clause, $context);
241248
}
242249

243-
private function segmentMatchesContext(Segment $segment, LDContext $context): bool
250+
private function segmentMatchesContext(Segment $segment, LDContext $context, EvaluatorState $state): bool
244251
{
245252
if (EvaluatorHelpers::contextKeyIsInTargetList($context, null, $segment->getIncluded())) {
246253
return true;
@@ -258,9 +265,22 @@ private function segmentMatchesContext(Segment $segment, LDContext $context): bo
258265
return false;
259266
}
260267
}
261-
foreach ($segment->getRules() as $rule) {
262-
if ($this->segmentRuleMatchesContext($rule, $context, $segment->getKey(), $segment->getSalt())) {
263-
return true;
268+
$rules = $segment->getRules();
269+
if (count($rules) !== 0) {
270+
// Evaluating rules means we might be doing recursive segment matches, so we'll push the current
271+
// segment key onto the stack for cycle detection.
272+
if ($state->segmentStack === null) {
273+
$state->segmentStack = [];
274+
}
275+
$state->segmentStack[] = $segment->getKey();
276+
try {
277+
foreach ($rules as $rule) {
278+
if ($this->segmentRuleMatchesContext($rule, $context, $segment->getKey(), $segment->getSalt(), $state)) {
279+
return true;
280+
}
281+
}
282+
} finally {
283+
array_pop($state->segmentStack);
264284
}
265285
}
266286
return false;
@@ -270,11 +290,12 @@ private function segmentRuleMatchesContext(
270290
SegmentRule $rule,
271291
LDContext $context,
272292
string $segmentKey,
273-
string $segmentSalt
293+
string $segmentSalt,
294+
EvaluatorState $state
274295
): bool {
275296
$rulej = print_r($rule, true);
276297
foreach ($rule->getClauses() as $clause) {
277-
if (!EvaluatorHelpers::matchClauseWithoutSegments($clause, $context)) {
298+
if (!$this->clauseMatchesContext($clause, $context, $state)) {
278299
return false;
279300
}
280301
}

‎tests/Impl/Evaluation/EvaluatorPrerequisiteTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,12 +116,12 @@ public function testFlagReturnsFallthroughVariationAndEventIfPrerequisiteIsMetAn
116116
self::assertEquals($flag0, $eval->getPrereqOfFlag());
117117
}
118118

119-
public function prerequisiteCycleDepths()
119+
public function recursionDepth()
120120
{
121121
return [[1], [2], [3], [4]];
122122
}
123123

124-
/** @dataProvider prerequisiteCycleDepths */
124+
/** @dataProvider recursionDepth */
125125
public function testPrerequisiteCycleDetection($depth)
126126
{
127127
$flagKeys = [];

‎tests/Impl/Evaluation/EvaluatorSegmentTest.php‎

Lines changed: 77 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace LaunchDarkly\Tests\Impl\Evaluation;
44

5+
use LaunchDarkly\EvaluationReason;
56
use LaunchDarkly\Impl\Evaluation\Evaluator;
67
use LaunchDarkly\Impl\Evaluation\EvaluatorBucketing;
78
use LaunchDarkly\Impl\Model\Segment;
@@ -53,7 +54,7 @@ public function testIncludedKeyForContextKind()
5354
$this->assertTrue(self::segmentMatchesContext($segment, $multi));
5455
}
5556

56-
public function excludedKeyForContextKind()
57+
public function testExcludedKeyForContextKind()
5758
{
5859
$c1 = LDContext::create('key1', 'kind1');
5960
$c2 = LDContext::create('key2', 'kind2');
@@ -181,14 +182,87 @@ public function testNonMatchingRuleWithMultipleClauses()
181182
$this->assertFalse(self::segmentMatchesContext($segment, $context));
182183
}
183184

185+
public function recursionDepth()
186+
{
187+
return [[1], [2], [3], [4]];
188+
}
189+
190+
/** @dataProvider recursionDepth */
191+
public function testSegmentReferencingSegment($depth)
192+
{
193+
$context = LDContext::create('foo');
194+
195+
$segmentKeys = [];
196+
for ($i = 0; $i < $depth; $i++) {
197+
$segmentKeys[] = "segmentkey$i";
198+
}
199+
$flags = [];
200+
$requester = new MockFeatureRequester();
201+
for ($i = 0; $i < $depth; $i++) {
202+
$builder = ModelBuilders::segmentBuilder($segmentKeys[$i]);
203+
if ($i == $depth - 1) {
204+
$builder->included($context->getKey());
205+
} else {
206+
$builder->rule(
207+
ModelBuilders::segmentRuleBuilder()
208+
->clause(ModelBuilders::clause(null, '', 'segmentMatch', $segmentKeys[$i + 1]))
209+
->build()
210+
);
211+
}
212+
$segment = $builder->build();
213+
$segments[] = $segment;
214+
$requester->addSegment($segment);
215+
}
216+
$evaluator = new Evaluator($requester);
217+
218+
$flag = ModelBuilders::booleanFlagWithClauses(ModelBuilders::clauseMatchingSegment($segments[0]));
219+
220+
$result = $evaluator->evaluate($flag, $context, EvaluatorTestUtil::expectNoPrerequisiteEvals());
221+
self::assertTrue($result->getDetail()->getValue());
222+
}
223+
224+
/** @dataProvider recursionDepth */
225+
public function testSegmentCycleDetection($depth)
226+
{
227+
$context = LDContext::create('foo');
228+
229+
$segmentKeys = [];
230+
for ($i = 0; $i < $depth; $i++) {
231+
$segmentKeys[] = "segmentkey$i";
232+
}
233+
$flags = [];
234+
$requester = new MockFeatureRequester();
235+
for ($i = 0; $i < $depth; $i++) {
236+
$builder = ModelBuilders::segmentBuilder($segmentKeys[$i]);
237+
$builder->rule(
238+
ModelBuilders::segmentRuleBuilder()
239+
->clause(ModelBuilders::clause(null, '', 'segmentMatch', $segmentKeys[($i + 1) % $depth]))
240+
->build()
241+
);
242+
$segment = $builder->build();
243+
$segments[] = $segment;
244+
$requester->addSegment($segment);
245+
}
246+
$evaluator = new Evaluator($requester);
247+
248+
$flag = ModelBuilders::booleanFlagWithClauses(ModelBuilders::clauseMatchingSegment($segments[0]));
249+
250+
$result = $evaluator->evaluate($flag, $context, EvaluatorTestUtil::expectNoPrerequisiteEvals());
251+
self::assertEquals(EvaluationReason::error(EvaluationReason::MALFORMED_FLAG_ERROR), $result->getDetail()->getReason());
252+
}
253+
184254
private static function segmentMatchesContext(Segment $segment, LDContext $context): bool
185255
{
186256
$flag = ModelBuilders::booleanFlagWithClauses(ModelBuilders::clauseMatchingSegment($segment));
187257

188258
$requester = new MockFeatureRequester();
189259
$requester->addSegment($segment);
190-
$evaluator = new Evaluator($requester);
260+
$evaluator = new Evaluator($requester, EvaluatorTestUtil::testLogger());
191261

192-
return $evaluator->evaluate($flag, $context, EvaluatorTestUtil::expectNoPrerequisiteEvals())->getDetail()->getValue();
262+
$detail = $evaluator->evaluate($flag, $context, EvaluatorTestUtil::expectNoPrerequisiteEvals())->getDetail();
263+
if ($detail->getValue() === null) {
264+
self::assertTrue(false, "Evaluation failed with reason: " . json_encode($detail->getReason()));
265+
}
266+
return $detail->getValue();
193267
}
194268
}

‎tests/Impl/Evaluation/EvaluatorTestUtil.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
use LaunchDarkly\Impl\Evaluation\Evaluator;
66
use LaunchDarkly\Impl\Evaluation\PrerequisiteEvaluationRecord;
77
use LaunchDarkly\Tests\MockFeatureRequester;
8+
use Monolog\Handler\ErrorLogHandler;
9+
use Monolog\Logger;
10+
use Psr\Log\LoggerInterface;
811

912
class EvaluatorTestUtil
1013
{
@@ -25,6 +28,11 @@ public static function prerequisiteRecorder(): PrerequisiteRecorder
2528
{
2629
return new PrerequisiteRecorder();
2730
}
31+
32+
public static function testLogger(): LoggerInterface
33+
{
34+
return new Logger("EvaluatorTest", [new ErrorLogHandler()]);
35+
}
2836
}
2937

3038
class PrerequisiteRecorder

0 commit comments

Comments
 (0)