Skip to content

Commit ccf2503

Browse files
yasirfolio3Michael Ng
yasirfolio3
authored and
Michael Ng
committed
refact(audience-evaluation): Fixed null-bubbling issues. (#177)
1 parent 4a36a9d commit ccf2503

20 files changed

+117
-68
lines changed

‎pkg/decision/evaluator/audience.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,6 @@ func NewTypedAudienceEvaluator() *TypedAudienceEvaluator {
4040
}
4141

4242
// Evaluate evaluates the typed audience against the given user's attributes
43-
func (a TypedAudienceEvaluator) Evaluate(audience entities.Audience, condTreeParams *entities.TreeParameters) bool {
43+
func (a TypedAudienceEvaluator) Evaluate(audience entities.Audience, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
4444
return a.conditionTreeEvaluator.Evaluate(audience.ConditionTree, condTreeParams)
4545
}

‎pkg/decision/evaluator/condition.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,10 @@ func (c AudienceConditionEvaluator) Evaluate(audienceID string, condTreeParams *
9292
if audience, ok := condTreeParams.AudienceMap[audienceID]; ok {
9393
condTree := audience.ConditionTree
9494
conditionTreeEvaluator := NewMixedTreeEvaluator()
95-
retValue := conditionTreeEvaluator.Evaluate(condTree, condTreeParams)
95+
retValue, isValid := conditionTreeEvaluator.Evaluate(condTree, condTreeParams)
96+
if !isValid {
97+
return false, fmt.Errorf(`an error occurred while evaluating nested tree for audience ID "%s"`, audienceID)
98+
}
9699
return retValue, nil
97100

98101
}

‎pkg/decision/evaluator/condition_tree.go‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ const (
3636

3737
// TreeEvaluator evaluates a tree
3838
type TreeEvaluator interface {
39-
Evaluate(*entities.TreeNode, *entities.TreeParameters) bool
39+
Evaluate(*entities.TreeNode, *entities.TreeParameters) (evalResult, isValid bool)
4040
}
4141

4242
// MixedTreeEvaluator evaluates a tree of mixed node types (condition node or audience nodes)
@@ -48,16 +48,8 @@ func NewMixedTreeEvaluator() *MixedTreeEvaluator {
4848
return &MixedTreeEvaluator{}
4949
}
5050

51-
// Evaluate returns true if the userAttributes satisfy the given condition tree
52-
func (c MixedTreeEvaluator) Evaluate(node *entities.TreeNode, condTreeParams *entities.TreeParameters) bool {
53-
// This wrapper method converts the conditionEvalResult to a boolean
54-
result, _ := c.evaluate(node, condTreeParams)
55-
return result
56-
}
57-
58-
// Helper method to recursively evaluate a condition tree
59-
// Returns the result of the evaluation and whether the evaluation of the condition is valid or not (to handle null bubbling)
60-
func (c MixedTreeEvaluator) evaluate(node *entities.TreeNode, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
51+
// Evaluate returns whether the userAttributes satisfy the given condition tree and the evaluation of the condition is valid or not (to handle null bubbling)
52+
func (c MixedTreeEvaluator) Evaluate(node *entities.TreeNode, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
6153
operator := node.Operator
6254
if operator != "" {
6355
switch operator {
@@ -94,7 +86,7 @@ func (c MixedTreeEvaluator) evaluate(node *entities.TreeNode, condTreeParams *en
9486
func (c MixedTreeEvaluator) evaluateAnd(nodes []*entities.TreeNode, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
9587
sawInvalid := false
9688
for _, node := range nodes {
97-
result, isValid := c.evaluate(node, condTreeParams)
89+
result, isValid := c.Evaluate(node, condTreeParams)
9890
if !isValid {
9991
return false, isValid
10092
} else if !result {
@@ -112,7 +104,7 @@ func (c MixedTreeEvaluator) evaluateAnd(nodes []*entities.TreeNode, condTreePara
112104

113105
func (c MixedTreeEvaluator) evaluateNot(nodes []*entities.TreeNode, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
114106
if len(nodes) > 0 {
115-
result, isValid := c.evaluate(nodes[0], condTreeParams)
107+
result, isValid := c.Evaluate(nodes[0], condTreeParams)
116108
if !isValid {
117109
return false, false
118110
}
@@ -124,7 +116,7 @@ func (c MixedTreeEvaluator) evaluateNot(nodes []*entities.TreeNode, condTreePara
124116
func (c MixedTreeEvaluator) evaluateOr(nodes []*entities.TreeNode, condTreeParams *entities.TreeParameters) (evalResult, isValid bool) {
125117
sawInvalid := false
126118
for _, node := range nodes {
127-
result, isValid := c.evaluate(node, condTreeParams)
119+
result, isValid := c.Evaluate(node, condTreeParams)
128120
if !isValid {
129121
sawInvalid = true
130122
} else if result {

‎pkg/decision/evaluator/condition_tree_test.go‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func TestConditionTreeEvaluateSimpleCondition(t *testing.T) {
4747
},
4848
}
4949
condTreeParams := e.NewTreeParameters(&user, map[string]e.Audience{})
50-
result := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
50+
result, _ := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
5151
assert.True(t, result)
5252

5353
// Test no match
@@ -56,7 +56,7 @@ func TestConditionTreeEvaluateSimpleCondition(t *testing.T) {
5656
"string_foo": "not foo",
5757
},
5858
}
59-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
59+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
6060
assert.False(t, result)
6161
}
6262

@@ -82,7 +82,7 @@ func TestConditionTreeEvaluateMultipleOrConditions(t *testing.T) {
8282
}
8383

8484
condTreeParams := e.NewTreeParameters(&user, map[string]e.Audience{})
85-
result := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
85+
result, _ := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
8686
assert.True(t, result)
8787

8888
// Test match bool
@@ -91,7 +91,7 @@ func TestConditionTreeEvaluateMultipleOrConditions(t *testing.T) {
9191
"bool_true": true,
9292
},
9393
}
94-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
94+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
9595
assert.True(t, result)
9696

9797
// Test match both
@@ -101,7 +101,7 @@ func TestConditionTreeEvaluateMultipleOrConditions(t *testing.T) {
101101
"bool_true": true,
102102
},
103103
}
104-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
104+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
105105
assert.True(t, result)
106106

107107
// Test no match
@@ -111,7 +111,7 @@ func TestConditionTreeEvaluateMultipleOrConditions(t *testing.T) {
111111
"bool_true": false,
112112
},
113113
}
114-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
114+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
115115
assert.False(t, result)
116116
}
117117

@@ -137,7 +137,7 @@ func TestConditionTreeEvaluateMultipleAndConditions(t *testing.T) {
137137
}
138138

139139
condTreeParams := e.NewTreeParameters(&user, map[string]e.Audience{})
140-
result := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
140+
result, _ := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
141141
assert.False(t, result)
142142

143143
// Test only bool match with NULL bubbling
@@ -146,7 +146,7 @@ func TestConditionTreeEvaluateMultipleAndConditions(t *testing.T) {
146146
"bool_true": true,
147147
},
148148
}
149-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
149+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
150150
assert.False(t, result)
151151

152152
// Test match both
@@ -156,7 +156,7 @@ func TestConditionTreeEvaluateMultipleAndConditions(t *testing.T) {
156156
"bool_true": true,
157157
},
158158
}
159-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
159+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
160160
assert.True(t, result)
161161

162162
// Test no match
@@ -166,7 +166,7 @@ func TestConditionTreeEvaluateMultipleAndConditions(t *testing.T) {
166166
"bool_true": false,
167167
},
168168
}
169-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
169+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
170170
assert.False(t, result)
171171
}
172172

@@ -203,7 +203,7 @@ func TestConditionTreeEvaluateNotCondition(t *testing.T) {
203203
}
204204

205205
condTreeParams := e.NewTreeParameters(&user, map[string]e.Audience{})
206-
result := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
206+
result, _ := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
207207
assert.True(t, result)
208208

209209
// Test match bool
@@ -212,7 +212,7 @@ func TestConditionTreeEvaluateNotCondition(t *testing.T) {
212212
"bool_true": false,
213213
},
214214
}
215-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
215+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
216216
assert.True(t, result)
217217

218218
// Test match both
@@ -222,7 +222,7 @@ func TestConditionTreeEvaluateNotCondition(t *testing.T) {
222222
"bool_true": false,
223223
},
224224
}
225-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
225+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
226226
assert.True(t, result)
227227

228228
// Test no match
@@ -232,7 +232,7 @@ func TestConditionTreeEvaluateNotCondition(t *testing.T) {
232232
"bool_true": true,
233233
},
234234
}
235-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
235+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
236236
assert.False(t, result)
237237
}
238238

@@ -282,7 +282,7 @@ func TestConditionTreeEvaluateMultipleMixedConditions(t *testing.T) {
282282
}
283283

284284
condTreeParams := e.NewTreeParameters(&user, map[string]e.Audience{})
285-
result := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
285+
result, _ := conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
286286
assert.True(t, result)
287287

288288
// Test only match the NOT condition
@@ -293,7 +293,7 @@ func TestConditionTreeEvaluateMultipleMixedConditions(t *testing.T) {
293293
"int_42": 43,
294294
},
295295
}
296-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
296+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
297297
assert.True(t, result)
298298

299299
// Test only match the int condition
@@ -304,7 +304,7 @@ func TestConditionTreeEvaluateMultipleMixedConditions(t *testing.T) {
304304
"int_42": 42,
305305
},
306306
}
307-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
307+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
308308
assert.True(t, result)
309309

310310
// Test no match
@@ -315,7 +315,7 @@ func TestConditionTreeEvaluateMultipleMixedConditions(t *testing.T) {
315315
"int_42": 43,
316316
},
317317
}
318-
result = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
318+
result, _ = conditionTreeEvaluator.Evaluate(conditionTree, condTreeParams)
319319
assert.False(t, result)
320320
}
321321

@@ -383,7 +383,7 @@ func TestConditionTreeEvaluateAnAudienceTreeSingleAudience(t *testing.T) {
383383
},
384384
AudienceMap: audienceMap,
385385
}
386-
result := conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
386+
result, _ := conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
387387
assert.True(t, result)
388388
}
389389

@@ -412,7 +412,7 @@ func TestConditionTreeEvaluateAnAudienceTreeMultipleAudiences(t *testing.T) {
412412
},
413413
AudienceMap: audienceMap,
414414
}
415-
result := conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
415+
result, _ := conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
416416
assert.True(t, result)
417417

418418
// Test only matches audience 11112
@@ -426,6 +426,6 @@ func TestConditionTreeEvaluateAnAudienceTreeMultipleAudiences(t *testing.T) {
426426
},
427427
AudienceMap: audienceMap,
428428
}
429-
result = conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
429+
result, _ = conditionTreeEvaluator.Evaluate(audienceTree, treeParams)
430430
assert.True(t, result)
431431
}

‎pkg/decision/evaluator/matchers/exact.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,23 @@ func (m ExactMatcher) Match(user entities.UserContext) (bool, error) {
3434
if stringValue, ok := m.Condition.Value.(string); ok {
3535
attributeValue, err := user.GetStringAttribute(m.Condition.Name)
3636
if err != nil {
37-
return false, nil
37+
return false, err
3838
}
3939
return stringValue == attributeValue, nil
4040
}
4141

4242
if boolValue, ok := m.Condition.Value.(bool); ok {
4343
attributeValue, err := user.GetBoolAttribute(m.Condition.Name)
4444
if err != nil {
45-
return false, nil
45+
return false, err
4646
}
4747
return boolValue == attributeValue, nil
4848
}
4949

5050
if floatValue, ok := utils.ToFloat(m.Condition.Value); ok {
5151
attributeValue, err := user.GetFloatAttribute(m.Condition.Name)
5252
if err != nil {
53-
return false, nil
53+
return false, err
5454
}
5555
return floatValue == attributeValue, nil
5656
}

‎pkg/decision/evaluator/matchers/exact_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func TestExactMatcherString(t *testing.T) {
6262
}
6363

6464
_, err = matcher.Match(user)
65-
assert.NoError(t, err)
65+
assert.Error(t, err)
6666
}
6767

6868
func TestExactMatcherBool(t *testing.T) {
@@ -103,7 +103,7 @@ func TestExactMatcherBool(t *testing.T) {
103103
}
104104

105105
_, err = matcher.Match(user)
106-
assert.NoError(t, err)
106+
assert.Error(t, err)
107107
}
108108

109109
func TestExactMatcherInt(t *testing.T) {
@@ -155,7 +155,7 @@ func TestExactMatcherInt(t *testing.T) {
155155
}
156156

157157
_, err = matcher.Match(user)
158-
assert.NoError(t, err)
158+
assert.Error(t, err)
159159
}
160160

161161
func TestExactMatcherFloat(t *testing.T) {
@@ -196,5 +196,5 @@ func TestExactMatcherFloat(t *testing.T) {
196196
}
197197

198198
_, err = matcher.Match(user)
199-
assert.NoError(t, err)
199+
assert.Error(t, err)
200200
}

‎pkg/decision/evaluator/matchers/exists.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,5 +28,6 @@ type ExistsMatcher struct {
2828

2929
// Match returns true if the user's attribute is in the condition
3030
func (m ExistsMatcher) Match(user entities.UserContext) (bool, error) {
31+
3132
return user.CheckAttributeExists(m.Condition.Name), nil
3233
}

‎pkg/decision/evaluator/matchers/gt.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func (m GtMatcher) Match(user entities.UserContext) (bool, error) {
3535
if floatValue, ok := utils.ToFloat(m.Condition.Value); ok {
3636
attributeValue, err := user.GetFloatAttribute(m.Condition.Name)
3737
if err != nil {
38-
return false, nil
38+
return false, err
3939
}
4040
return floatValue < attributeValue, nil
4141
}

‎pkg/decision/evaluator/matchers/gt_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func TestGtMatcherInt(t *testing.T) {
8484
}
8585

8686
_, err = matcher.Match(user)
87-
assert.NoError(t, err)
87+
assert.Error(t, err)
8888
}
8989

9090
func TestGtMatcherFloat(t *testing.T) {
@@ -135,5 +135,5 @@ func TestGtMatcherFloat(t *testing.T) {
135135
}
136136

137137
_, err = matcher.Match(user)
138-
assert.NoError(t, err)
138+
assert.Error(t, err)
139139
}

‎pkg/decision/evaluator/matchers/lt.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func (m LtMatcher) Match(user entities.UserContext) (bool, error) {
3535
if floatValue, ok := utils.ToFloat(m.Condition.Value); ok {
3636
attributeValue, err := user.GetFloatAttribute(m.Condition.Name)
3737
if err != nil {
38-
return false, nil
38+
return false, err
3939
}
4040
return floatValue > attributeValue, nil
4141
}

0 commit comments

Comments
 (0)