Skip to content

Commit e821ab8

Browse files
eli-darklyeli-darkly
authored
(U2C #15) implement prerequisite cycle detection (#120)
1 parent dc0ace9 commit e821ab8

File tree

5 files changed

+236
-152
lines changed

5 files changed

+236
-152
lines changed

‎Makefile‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,10 @@ 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/prerequisites": Can't pass yet because prerequisite cycle detection is not implemented.
1918
# - "evaluation/parameterized/segment recursion": Haven't yet implemented segment recursion.
2019
TEST_HARNESS_PARAMS := $(TEST_HARNESS_PARAMS) \
2120
-skip 'evaluation/bucketing/secondary' \
2221
-skip 'evaluation/parameterized/attribute references/array index is not supported' \
23-
-skip 'evaluation/parameterized/prerequisites' \
2422
-skip 'evaluation/parameterized/segment recursion'
2523

2624
build-contract-tests:

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

Lines changed: 83 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,19 @@
1414
use LaunchDarkly\LDContext;
1515
use Psr\Log\LoggerInterface;
1616

17+
/**
18+
* @ignore
19+
* @internal
20+
*/
21+
class EvaluatorState
22+
{
23+
public ?array $prerequisiteStack = null;
24+
25+
public function __construct(public FeatureFlag $originalFlag)
26+
{
27+
}
28+
}
29+
1730
/**
1831
* Encapsulates the feature flag evaluation logic. The Evaluator has no direct access to the
1932
* rest of the SDK environment; if it needs to retrieve flags or segments that are referenced
@@ -46,81 +59,96 @@ public function __construct(FeatureRequester $featureRequester, ?LoggerInterface
4659
*/
4760
public function evaluate(FeatureFlag $flag, LDContext $context, ?callable $prereqEvalSink): EvalResult
4861
{
62+
$stateStack = null;
63+
$state = new EvaluatorState($flag);
4964
try {
50-
return $this->evaluateInternal($flag, $context, $prereqEvalSink);
65+
return $this->evaluateInternal($flag, $context, $prereqEvalSink, $state);
5166
} catch (EvaluationException $e) {
5267
return new EvalResult(new EvaluationDetail(null, null, EvaluationReason::error($e->getErrorKind())));
68+
} catch (\Throwable $e) {
69+
Util::logExceptionAtErrorLevel($this->_logger, $e, 'Unexpected error when evaluating flag ' . $flag->getKey());
70+
return new EvalResult(new EvaluationDetail(null, null, EvaluationReason::error(EvaluationReason::EXCEPTION_ERROR)));
5371
}
5472
}
5573

5674
private function evaluateInternal(
5775
FeatureFlag $flag,
5876
LDContext $context,
59-
?callable $prereqEvalSink
77+
?callable $prereqEvalSink,
78+
EvaluatorState $state
6079
): EvalResult {
61-
try {
62-
// The reason there's an extra try block here is that if something fails during evaluation of the
63-
// prerequisites, we don't want that to short-circuit evaluation of the base flag in the way that
64-
// an error normally would, where the whole evaluation would return an error result with a default
65-
// value. Instead we want the base flag to return its off variation, as it always does if any
66-
// prerequisites have failed.
67-
if (!$flag->isOn()) {
68-
return EvaluatorHelpers::getOffResult($flag, EvaluationReason::off());
69-
}
80+
if (!$flag->isOn()) {
81+
return EvaluatorHelpers::getOffResult($flag, EvaluationReason::off());
82+
}
7083

71-
$prereqFailureReason = $this->checkPrerequisites($flag, $context, $prereqEvalSink);
72-
if ($prereqFailureReason !== null) {
73-
return EvaluatorHelpers::getOffResult($flag, $prereqFailureReason);
74-
}
84+
$prereqFailureReason = $this->checkPrerequisites($flag, $context, $prereqEvalSink, $state);
85+
if ($prereqFailureReason !== null) {
86+
return EvaluatorHelpers::getOffResult($flag, $prereqFailureReason);
87+
}
7588

76-
// Check to see if targets match
77-
$targetResult = $this->checkTargets($flag, $context);
78-
if ($targetResult) {
79-
return $targetResult;
80-
}
89+
// Check to see if targets match
90+
$targetResult = $this->checkTargets($flag, $context);
91+
if ($targetResult) {
92+
return $targetResult;
93+
}
8194

82-
// Now walk through the rules and see if any match
83-
foreach ($flag->getRules() as $i => $rule) {
84-
if ($this->ruleMatchesContext($rule, $context)) {
85-
return EvaluatorHelpers::getResultForVariationOrRollout(
86-
$flag,
87-
$rule,
88-
$rule->isTrackEvents(),
89-
$context,
90-
EvaluationReason::ruleMatch($i, $rule->getId())
91-
);
92-
}
95+
// Now walk through the rules and see if any match
96+
foreach ($flag->getRules() as $i => $rule) {
97+
if ($this->ruleMatchesContext($rule, $context)) {
98+
return EvaluatorHelpers::getResultForVariationOrRollout(
99+
$flag,
100+
$rule,
101+
$rule->isTrackEvents(),
102+
$context,
103+
EvaluationReason::ruleMatch($i, $rule->getId())
104+
);
93105
}
94-
return EvaluatorHelpers::getResultForVariationOrRollout(
95-
$flag,
96-
$flag->getFallthrough(),
97-
$flag->isTrackEventsFallthrough(),
98-
$context,
99-
EvaluationReason::fallthrough()
100-
);
101-
} catch (EvaluationException $e) {
102-
return new EvalResult(new EvaluationDetail(null, null, EvaluationReason::error($e->getErrorKind())));
103-
} catch (\Throwable $e) {
104-
Util::logExceptionAtErrorLevel($this->_logger, $e, 'Unexpected error when evaluating flag ' . $flag->getKey());
105-
return new EvalResult(new EvaluationDetail(null, null, EvaluationReason::error(EvaluationReason::EXCEPTION_ERROR)));
106106
}
107+
return EvaluatorHelpers::getResultForVariationOrRollout(
108+
$flag,
109+
$flag->getFallthrough(),
110+
$flag->isTrackEventsFallthrough(),
111+
$context,
112+
EvaluationReason::fallthrough()
113+
);
107114
}
108115

109116
private function checkPrerequisites(
110117
FeatureFlag $flag,
111118
LDContext $context,
112-
?callable $prereqEvalSink
119+
?callable $prereqEvalSink,
120+
EvaluatorState $state
113121
): ?EvaluationReason {
114-
foreach ($flag->getPrerequisites() as $prereq) {
115-
$prereqOk = true;
116-
try {
117-
$prereqFeatureFlag = $this->_featureRequester->getFeature($prereq->getKey());
122+
// We use the state object to guard against circular references in prerequisites. To avoid
123+
// the overhead of creating the $state->prerequisiteStack array in the most common case where
124+
// there's only a single level of prerequisites, we treat $state->originalFlag as the first
125+
// element in the stack.
126+
$flagKey = $flag->getKey();
127+
if ($flag !== $state->originalFlag) {
128+
if ($state->prerequisiteStack === null) {
129+
$state->prerequisiteStack = [];
130+
}
131+
$state->prerequisiteStack[] = $flagKey;
132+
}
133+
try {
134+
foreach ($flag->getPrerequisites() as $prereq) {
135+
$prereqKey = $prereq->getKey();
136+
137+
if ($prereqKey === $state->originalFlag->getKey() ||
138+
($state->prerequisiteStack !== null && in_array($prereqKey, $state->prerequisiteStack))) {
139+
throw new EvaluationException(
140+
"prerequisite relationship to \"$prereqKey\" caused a circular reference; this is probably a temporary condition due to an incomplete update",
141+
EvaluationReason::MALFORMED_FLAG_ERROR
142+
);
143+
}
144+
$prereqOk = true;
145+
$prereqFeatureFlag = $this->_featureRequester->getFeature($prereqKey);
118146
if ($prereqFeatureFlag === null) {
119147
$prereqOk = false;
120148
} else {
121149
// Note that if the prerequisite flag is off, we don't consider it a match no matter what its
122150
// off variation was. But we still need to evaluate it in order to generate an event.
123-
$prereqEvalResult = $this->evaluateInternal($prereqFeatureFlag, $context, $prereqEvalSink);
151+
$prereqEvalResult = $this->evaluateInternal($prereqFeatureFlag, $context, $prereqEvalSink, $state);
124152
$variation = $prereq->getVariation();
125153
if (!$prereqFeatureFlag->isOn() || $prereqEvalResult->getDetail()->getVariationIndex() !== $variation) {
126154
$prereqOk = false;
@@ -129,16 +157,13 @@ private function checkPrerequisites(
129157
$prereqEvalSink(new PrerequisiteEvaluationRecord($prereqFeatureFlag, $flag, $prereqEvalResult));
130158
}
131159
}
132-
} catch (\Throwable $e) {
133-
Util::logExceptionAtErrorLevel(
134-
$this->_logger,
135-
$e,
136-
'Unexpected error when evaluating prerequisite flag ' . $prereq->getKey()
137-
);
138-
$prereqOk = false;
160+
if (!$prereqOk) {
161+
return EvaluationReason::prerequisiteFailed($prereqKey);
162+
}
139163
}
140-
if (!$prereqOk) {
141-
return EvaluationReason::prerequisiteFailed($prereq->getKey());
164+
} finally {
165+
if ($state->prerequisiteStack !== null && count($state->prerequisiteStack) !== 0) {
166+
array_pop($state->prerequisiteStack);
142167
}
143168
}
144169
return null;

‎tests/FlagBuilder.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ class FlagBuilder
3535
public function __construct(string $key)
3636
{
3737
$this->_key = $key;
38+
$this->_fallthrough = new VariationOrRollout(null, null);
3839
}
3940

4041
public function build(): FeatureFlag

‎tests/Impl/Evaluation/EvaluatorFlagTest.php‎

Lines changed: 0 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
use LaunchDarkly\Impl\Evaluation\EvaluatorBucketing;
99
use LaunchDarkly\Impl\Model\Rollout;
1010
use LaunchDarkly\LDContext;
11-
use LaunchDarkly\Tests\MockFeatureRequester;
1211
use LaunchDarkly\Tests\ModelBuilders;
1312
use PHPUnit\Framework\TestCase;
1413

@@ -81,97 +80,6 @@ public function testFlagReturnsErrorIfOffVariationIsNegative()
8180
self::assertEquals($detail, $result->getDetail());
8281
}
8382

84-
public function testFlagReturnsOffVariationIfPrerequisiteIsNotFound()
85-
{
86-
$flag = ModelBuilders::flagBuilder('feature0')->variations('fall', 'off', 'on')
87-
->on(true)->offVariation(1)->fallthroughVariation(0)
88-
->prerequisite('feature1', 1)
89-
->build();
90-
91-
$result = static::$basicEvaluator->evaluate($flag, LDContext::create('user'), EvaluatorTestUtil::expectNoPrerequisiteEvals());
92-
$detail = new EvaluationDetail('off', 1, EvaluationReason::prerequisiteFailed('feature1'));
93-
self::assertEquals($detail, $result->getDetail());
94-
}
95-
96-
public function testFlagReturnsOffVariationAndEventIfPrerequisiteIsOff()
97-
{
98-
$flag1 = ModelBuilders::flagBuilder('feature1')->variations('nogo', 'go')
99-
->on(false)->offVariation(1)->fallthroughVariation(0)
100-
// note that even though it returns the desired variation, it is still off and therefore not a match
101-
->build();
102-
$flag0 = ModelBuilders::flagBuilder('feature0')->variations('fall', 'off', 'on')
103-
->on(true)->offVariation(1)->fallthroughVariation(0)
104-
->prerequisite($flag1->getKey(), 1)
105-
->build();
106-
107-
$requester = new MockFeatureRequester();
108-
$requester->addFlag($flag1);
109-
$evaluator = new Evaluator($requester);
110-
$recorder = EvaluatorTestUtil::prerequisiteRecorder();
111-
112-
$result = $evaluator->evaluate($flag0, LDContext::create('user'), $recorder->record());
113-
$detail = new EvaluationDetail('off', 1, EvaluationReason::prerequisiteFailed($flag1->getKey()));
114-
self::assertEquals($detail, $result->getDetail());
115-
116-
self::assertEquals(1, count($recorder->evals));
117-
$eval = $recorder->evals[0];
118-
self::assertEquals($flag1, $eval->getFlag());
119-
self::assertEquals('go', $eval->getResult()->getDetail()->getValue());
120-
self::assertEquals($flag0, $eval->getPrereqOfFlag());
121-
}
122-
123-
public function testFlagReturnsOffVariationAndEventIfPrerequisiteIsNotMet()
124-
{
125-
$flag1 = ModelBuilders::flagBuilder('feature1')->variations('nogo', 'go')
126-
->on(true)->offVariation(1)->fallthroughVariation(0)
127-
->build();
128-
$flag0 = ModelBuilders::flagBuilder('feature0')->variations('fall', 'off', 'on')
129-
->on(true)->offVariation(1)->fallthroughVariation(0)
130-
->prerequisite($flag1->getKey(), 1)
131-
->build();
132-
133-
$requester = new MockFeatureRequester();
134-
$requester->addFlag($flag1);
135-
$evaluator = new Evaluator($requester);
136-
$recorder = EvaluatorTestUtil::prerequisiteRecorder();
137-
138-
$result = $evaluator->evaluate($flag0, LDContext::create('user'), $recorder->record());
139-
$detail = new EvaluationDetail('off', 1, EvaluationReason::prerequisiteFailed($flag1->getKey()));
140-
self::assertEquals($detail, $result->getDetail());
141-
142-
self::assertEquals(1, count($recorder->evals));
143-
$eval = $recorder->evals[0];
144-
self::assertEquals($flag1, $eval->getFlag());
145-
self::assertEquals('nogo', $eval->getResult()->getDetail()->getValue());
146-
self::assertEquals($flag0, $eval->getPrereqOfFlag());
147-
}
148-
149-
public function testFlagReturnsFallthroughVariationAndEventIfPrerequisiteIsMetAndThereAreNoRules()
150-
{
151-
$flag1 = ModelBuilders::flagBuilder('feature1')->variations('nogo', 'go')
152-
->on(true)->offVariation(1)->fallthroughVariation(1)
153-
->build();
154-
$flag0 = ModelBuilders::flagBuilder('feature0')->variations('fall', 'off', 'on')
155-
->on(true)->fallthroughVariation(0)
156-
->prerequisite($flag1->getKey(), 1)
157-
->build();
158-
159-
$requester = new MockFeatureRequester();
160-
$requester->addFlag($flag1);
161-
$evaluator = new Evaluator($requester);
162-
$recorder = EvaluatorTestUtil::prerequisiteRecorder();
163-
164-
$result = $evaluator->evaluate($flag0, LDContext::create('user'), $recorder->record());
165-
$detail = new EvaluationDetail('fall', 0, EvaluationReason::fallthrough());
166-
self::assertEquals($detail, $result->getDetail());
167-
168-
self::assertEquals(1, count($recorder->evals));
169-
$eval = $recorder->evals[0];
170-
self::assertEquals($flag1, $eval->getFlag());
171-
self::assertEquals('go', $eval->getResult()->getDetail()->getValue());
172-
self::assertEquals($flag0, $eval->getPrereqOfFlag());
173-
}
174-
17583
public function testFlagMatchesContextFromRules()
17684
{
17785
global $defaultContext;

0 commit comments

Comments
 (0)