Skip to content

Commit 98f418e

Browse files
jsonbaileyjsonbailey
authored
fix: Invalid circular reference detection for nested segments (#137)
1 parent 96c1918 commit 98f418e

File tree

5 files changed

+196
-33
lines changed

5 files changed

+196
-33
lines changed

‎pkgs/sdk/server/src/Internal/Evaluation/Evaluator.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ internal sealed partial class Evaluator
3737

3838
// EvalState is a container for mutable state information and immutable parameters whose scope is a
3939
// single call to Evaluator.Evaluate(). The flag being evaluated is *not* part of the state-- we pass
40-
// it around as a parameter-- because a single Evaluate may cause multiple flags to be evaluated due
40+
// it around as a parameter-- because a single Evaluate may cause multiple flags to be evaluated due
4141
// to prerequisite relationships. But the Context is part of the state, because it is always the same
4242
// no matter how many nested things are being evaluated.
4343
internal struct EvalState

‎pkgs/sdk/server/src/Internal/Evaluation/EvaluatorTypes.cs‎

Lines changed: 26 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -44,55 +44,50 @@ internal struct LazyStack<T>
4444

4545
internal void Push(T value)
4646
{
47-
if (_hasFirstValue)
48-
{
49-
if (_values is null)
50-
{
51-
_values = new List<T>();
52-
_values.Add(_firstValue);
53-
}
54-
_values.Add(value);
55-
}
56-
else
47+
if (!_hasFirstValue)
5748
{
5849
_firstValue = value;
5950
_hasFirstValue = true;
51+
return;
52+
}
53+
54+
if (_values is null)
55+
{
56+
_values = new List<T>();
6057
}
58+
_values.Add(value);
6159
}
6260

6361
internal T Pop()
6462
{
65-
if (_values is null || _values.Count == 0)
63+
if (!_hasFirstValue)
64+
{
65+
throw new InvalidOperationException();
66+
}
67+
68+
if (_values != null && _values.Count > 0)
6669
{
67-
if (!_hasFirstValue)
68-
{
69-
throw new InvalidOperationException();
70-
}
71-
_hasFirstValue = false;
72-
return _firstValue;
70+
var value = _values[_values.Count - 1];
71+
_values.RemoveAt(_values.Count - 1);
72+
return value;
7373
}
74-
var value = _values[_values.Count - 1];
75-
_values.RemoveAt(_values.Count - 1);
76-
return value;
74+
75+
_hasFirstValue = false;
76+
return _firstValue;
7777
}
7878

7979
internal bool Contains(T value)
8080
{
81-
if (_hasFirstValue && _firstValue.Equals(value))
81+
if (!_hasFirstValue)
8282
{
83-
return true;
83+
return false;
8484
}
85-
if (!(_values is null))
85+
else if (_firstValue.Equals(value))
8686
{
87-
foreach (var v in _values)
88-
{
89-
if (v.Equals(value))
90-
{
91-
return true;
92-
}
93-
}
87+
return true;
9488
}
95-
return false;
89+
90+
return _values != null && _values.Contains(value);
9691
}
9792
}
9893

‎pkgs/sdk/server/test/Internal/Evaluation/EvaluatorSegmentMatchTest.cs‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public void ExplicitIncludeByContextKind()
2323
{
2424
var s = new SegmentBuilder("test").Version(1).
2525
IncludedContext(kind1, "key1").IncludedContext(kind2, "key2").Build();
26-
26+
2727
Assert.True(SegmentMatchesUser(s, Context.New(kind1, "key1")));
2828
Assert.True(SegmentMatchesUser(s, Context.New(kind2, "key2")));
2929
Assert.False(SegmentMatchesUser(s, Context.New(kind1, "key2")));
@@ -171,5 +171,33 @@ private bool SegmentMatchesUser(Segment segment, Context context)
171171
var result = evaluator.Evaluate(flag, context);
172172
return result.Result.Value.AsBool;
173173
}
174+
175+
[Fact]
176+
public void ContextNotInExcludedNestedSegment()
177+
{
178+
var nestedSegment = new SegmentBuilder("nested-segment")
179+
.Rules(new SegmentRuleBuilder().Clauses(ClauseBuilder.ShouldMatchUser(Context.New("user1"))).Build())
180+
.Build();
181+
var parentSegment = new SegmentBuilder("parent-segment")
182+
.Rules(new SegmentRuleBuilder().Clauses(ClauseBuilder.ShouldMatchSegment(nestedSegment.Key)).Build())
183+
.Build();
184+
var excludeOtherSegments = new SegmentBuilder("exclude-other-segments")
185+
.Rules(new SegmentRuleBuilder().Clauses(ClauseBuilder.ShouldNotMatchSegment(parentSegment.Key)).Build())
186+
.Build();
187+
188+
var flag = new FeatureFlagBuilder("key").On(true).OffVariation(0)
189+
.FallthroughVariation(0)
190+
.Variations(false, true)
191+
.Rules(
192+
new RuleBuilder().Id("id1").Variation(0).Clauses(ClauseBuilder.ShouldMatchSegment(parentSegment.Key)).Build(),
193+
new RuleBuilder().Id("id2").Variation(1).Clauses(ClauseBuilder.ShouldMatchSegment(excludeOtherSegments.Key)).Build()
194+
)
195+
.Build();
196+
var logCapture = Logs.Capture();
197+
var evaluator = BasicEvaluator.WithStoredSegments(nestedSegment, parentSegment, excludeOtherSegments).WithLogger(logCapture.Logger(""));
198+
var result = evaluator.Evaluate(flag, Context.New("key"));
199+
Assert.Null(result.Result.Reason.ErrorKind);
200+
Assert.True(result.Result.Value.AsBool);
201+
}
174202
}
175203
}

‎pkgs/sdk/server/test/Internal/Evaluation/EvaluatorTypesLazyStackTest.cs‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
using System;
2+
using Xunit;
3+
using static LaunchDarkly.Sdk.Server.Internal.Evaluation.EvaluatorTypes;
4+
5+
namespace LaunchDarkly.Sdk.Server.Internal.Evaluation
6+
{
7+
public class EvaluatorTypesLazyStackTest
8+
{
9+
[Fact]
10+
public void CanPushAndPopSingleItem()
11+
{
12+
var stack = new LazyStack<string>();
13+
14+
stack.Push("item1");
15+
16+
Assert.Equal("item1", stack.Pop());
17+
}
18+
19+
[Fact]
20+
public void CanPushAndPopMultipleItems()
21+
{
22+
var stack = new LazyStack<string>();
23+
24+
stack.Push("item1");
25+
stack.Push("item2");
26+
stack.Push("item3");
27+
28+
Assert.Equal("item3", stack.Pop());
29+
Assert.Equal("item2", stack.Pop());
30+
Assert.Equal("item1", stack.Pop());
31+
}
32+
33+
[Fact]
34+
public void PopEmptyStackThrowsException()
35+
{
36+
var stack = new LazyStack<string>();
37+
38+
Assert.Throws<InvalidOperationException>(() => stack.Pop());
39+
}
40+
41+
[Fact]
42+
public void ContainsReturnsTrueForExistingItem()
43+
{
44+
var stack = new LazyStack<string>();
45+
46+
stack.Push("item1");
47+
stack.Push("item2");
48+
49+
Assert.True(stack.Contains("item1"));
50+
Assert.True(stack.Contains("item2"));
51+
}
52+
53+
[Fact]
54+
public void ContainsReturnsFalseForNonExistingItem()
55+
{
56+
var stack = new LazyStack<string>();
57+
58+
stack.Push("item1");
59+
60+
Assert.False(stack.Contains("item2"));
61+
}
62+
63+
[Fact]
64+
public void ContainsWorksAfterPop()
65+
{
66+
var stack = new LazyStack<string>();
67+
68+
stack.Push("item1");
69+
stack.Push("item2");
70+
stack.Pop();
71+
72+
Assert.True(stack.Contains("item1"));
73+
Assert.False(stack.Contains("item2"));
74+
}
75+
76+
[Fact]
77+
public void ContainsReturnsFalseForEmptyStack()
78+
{
79+
var stack = new LazyStack<string>();
80+
81+
Assert.False(stack.Contains("item1"));
82+
}
83+
84+
[Fact]
85+
public void CanPushSameItemMultipleTimes()
86+
{
87+
var stack = new LazyStack<string>();
88+
89+
stack.Push("duplicate");
90+
stack.Push("other");
91+
stack.Push("duplicate");
92+
93+
Assert.True(stack.Contains("duplicate"));
94+
Assert.Equal("duplicate", stack.Pop());
95+
Assert.Equal("other", stack.Pop());
96+
Assert.Equal("duplicate", stack.Pop());
97+
}
98+
99+
[Fact]
100+
public void WorksWithDifferentTypes()
101+
{
102+
var intStack = new LazyStack<int>();
103+
104+
intStack.Push(1);
105+
intStack.Push(2);
106+
107+
Assert.Equal(2, intStack.Pop());
108+
Assert.Equal(1, intStack.Pop());
109+
110+
var strStack = new LazyStack<string>();
111+
112+
strStack.Push("one");
113+
strStack.Push("two");
114+
115+
Assert.Equal("two", strStack.Pop());
116+
Assert.Equal("one", strStack.Pop());
117+
}
118+
119+
[Fact]
120+
public void ContainsReturnsFalseAfterLazyTransition()
121+
{
122+
var stack = new LazyStack<string>();
123+
124+
// Push enough items to trigger transition to list
125+
stack.Push("A");
126+
stack.Push("B");
127+
128+
Assert.True(stack.Contains("A"));
129+
130+
stack.Pop(); // Removes "B"
131+
Assert.True(stack.Contains("A"));
132+
133+
stack.Pop(); // Removes "A"
134+
Assert.False(stack.Contains("A"));
135+
}
136+
}
137+
}

‎pkgs/sdk/server/test/Internal/Model/FeatureFlagBuilder.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,9 @@ public static Clause ShouldNotMatchUser(Context user) =>
367367

368368
public static Clause ShouldMatchSegment(params string[] segmentKeys) =>
369369
new ClauseBuilder().Attribute("").Op("segmentMatch").Values(segmentKeys).Build();
370+
371+
public static Clause ShouldNotMatchSegment(params string[] segmentKeys) =>
372+
new ClauseBuilder().Attribute("").Op("segmentMatch").Values(segmentKeys).Negate(true).Build();
370373
}
371374

372375
internal class TargetBuilder

0 commit comments

Comments
 (0)