Skip to content

Commit

Permalink
fix: various edge cases in targeting (#1041)
Browse files Browse the repository at this point in the history 
- returning variants from targeting which are not in the variants list is now an error
- minor spec changes

---------

Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
  • Loading branch information
@toddbaert @beeme1mr
toddbaert and beeme1mr authored Dec 5, 2023
1 parent 2293f0e commit ca38c16
Show file tree
Hide file tree
Showing 10 changed files with 261 additions and 118 deletions.
74 changes: 24 additions & 50 deletions core/pkg/eval/fractional_evaluation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ func TestFractionalEvaluation(t *testing.T) {
}

tests := map[string]struct {
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedError error
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedErrorCode string
}{
"[email protected]": {
flags: flags,
Expand Down Expand Up @@ -247,38 +247,6 @@ func TestFractionalEvaluation(t *testing.T) {
expectedValue: "#FF0000",
expectedReason: model.DefaultReason,
},
"fallback to default variant if invalid variant as result of fractional evaluation": {
flags: Flags{
Flags: map[string]model.Flag{
"headerColor": {
State: "ENABLED",
DefaultVariant: "red",
Variants: map[string]any{
"red": "#FF0000",
"blue": "#0000FF",
"green": "#00FF00",
"yellow": "#FFFF00",
},
Targeting: []byte(`{
"fractional": [
{"var": "email"},
[
"black",
100
]
]
}`),
},
},
},
flagKey: "headerColor",
context: map[string]any{
"email": "[email protected]",
},
expectedVariant: "red",
expectedValue: "#FF0000",
expectedReason: model.DefaultReason,
},
"fallback to default variant if percentages don't sum to 100": {
flags: Flags{
Flags: map[string]model.Flag{
Expand Down Expand Up @@ -379,8 +347,11 @@ func TestFractionalEvaluation(t *testing.T) {
t.Errorf("expected reason '%s', got '%s'", tt.expectedReason, reason)
}

if err != tt.expectedError {
t.Errorf("expected err '%v', got '%v'", tt.expectedError, err)
if err != nil {
errorCode := err.Error()
if errorCode != tt.expectedErrorCode {
t.Errorf("expected err '%v', got '%v'", tt.expectedErrorCode, err)
}
}
})
}
Expand Down Expand Up @@ -433,13 +404,13 @@ func BenchmarkFractionalEvaluation(b *testing.B) {
}

tests := map[string]struct {
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedError error
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedErrorCode string
}{
"[email protected]": {
flags: flags,
Expand Down Expand Up @@ -509,8 +480,11 @@ func BenchmarkFractionalEvaluation(b *testing.B) {
b.Errorf("expected reason '%s', got '%s'", tt.expectedReason, reason)
}

if err != tt.expectedError {
b.Errorf("expected err '%v', got '%v'", tt.expectedError, err)
if err != nil {
errorCode := err.Error()
if errorCode != tt.expectedErrorCode {
b.Errorf("expected err '%v', got '%v'", tt.expectedErrorCode, err)
}
}
}
})
Expand Down
20 changes: 12 additions & 8 deletions core/pkg/eval/json_evaluator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,21 +341,25 @@ func (je *JSONEvaluator) evaluateVariant(reqID string, flagKey string, context m
je.Logger.ErrorWithID(reqID, fmt.Sprintf("error applying rules: %s", err))
return "", flag.Variants, model.ErrorReason, metadata, errors.New(model.ParseErrorCode)
}

// check if string is "null" before we strip quotes, so we can differentiate between JSON null and "null"
trimmed := strings.TrimSpace(result.String())
if trimmed == "null" {
return flag.DefaultVariant, flag.Variants, model.DefaultReason, metadata, nil
}

// strip whitespace and quotes from the variant
variant = strings.ReplaceAll(strings.TrimSpace(result.String()), "\"", "")
variant = strings.ReplaceAll(trimmed, "\"", "")

// if this is a valid variant, return it
if _, ok := flag.Variants[variant]; ok {
return variant, flag.Variants, model.TargetingMatchReason, metadata, nil
}

je.Logger.DebugWithID(reqID, fmt.Sprintf("returning default variant for flagKey: %s, variant is not valid", flagKey))
reason = model.DefaultReason
} else {
reason = model.StaticReason
je.Logger.ErrorWithID(reqID,
fmt.Sprintf("invalid or missing variant: %s for flagKey: %s, variant is not valid", variant, flagKey))
return "", flag.Variants, model.ErrorReason, metadata, errors.New(model.ParseErrorCode)
}

return flag.DefaultVariant, flag.Variants, reason, metadata, nil
return flag.DefaultVariant, flag.Variants, model.StaticReason, metadata, nil
}

func (je *JSONEvaluator) setFlagdProperties(
Expand Down
95 changes: 95 additions & 0 deletions core/pkg/eval/json_evaluator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1281,3 +1281,98 @@ func TestFlagdAmbientProperties(t *testing.T) {
}
})
}

func TestTargetingVariantBehavior(t *testing.T) {
t.Run("missing variant error", func(t *testing.T) {
evaluator := eval.NewJSONEvaluator(logger.NewLogger(nil, false), store.NewFlags())

_, _, err := evaluator.SetState(sync.DataSync{FlagData: `{
"flags": {
"missing-variant": {
"state": "ENABLED",
"variants": {
"foo": true,
"bar": false
},
"defaultVariant": "foo",
"targeting": {
"if": [ true, "buz", "baz"]
}
}
}
}`})
if err != nil {
t.Fatal(err)
}

_, _, _, _, err = evaluator.ResolveBooleanValue(context.Background(), "default", "missing-variant", nil)
if err == nil {
t.Fatal("missing variant did not result in error")
}
})

t.Run("null fallback", func(t *testing.T) {
evaluator := eval.NewJSONEvaluator(logger.NewLogger(nil, false), store.NewFlags())

_, _, err := evaluator.SetState(sync.DataSync{FlagData: `{
"flags": {
"null-fallback": {
"state": "ENABLED",
"variants": {
"foo": true,
"bar": false
},
"defaultVariant": "foo",
"targeting": {
"if": [ true, null, "baz"]
}
}
}
}`})
if err != nil {
t.Fatal(err)
}

value, variant, reason, _, err := evaluator.ResolveBooleanValue(context.Background(), "default", "null-fallback", nil)
if err != nil {
t.Fatal(err)
}

if !value || variant != "foo" || reason != model.DefaultReason {
t.Fatal("did not fallback to defaultValue")
}
})

t.Run("match booleans", func(t *testing.T) {
evaluator := eval.NewJSONEvaluator(logger.NewLogger(nil, false), store.NewFlags())

//nolint:dupword
_, _, err := evaluator.SetState(sync.DataSync{FlagData: `{
"flags": {
"match-boolean": {
"state": "ENABLED",
"variants": {
"false": 1,
"true": 2
},
"defaultVariant": "false",
"targeting": {
"if": [ true, true, false]
}
}
}
}`})
if err != nil {
t.Fatal(err)
}

value, variant, reason, _, err := evaluator.ResolveIntValue(context.Background(), "default", "match-boolean", nil)
if err != nil {
t.Fatal(err)
}

if value != 2 || variant != "true" || reason != model.TargetingMatchReason {
t.Fatal("did not map to stringified boolean")
}
})
}
39 changes: 22 additions & 17 deletions core/pkg/eval/legacy_fractional_evaluation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ func TestLegacyFractionalEvaluation(t *testing.T) {
}

tests := map[string]struct {
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedError error
flags Flags
flagKey string
context map[string]any
expectedValue string
expectedVariant string
expectedReason string
expectedErrorCode string
}{
"[email protected]": {
flags: flags,
Expand Down Expand Up @@ -188,11 +188,12 @@ func TestLegacyFractionalEvaluation(t *testing.T) {
},
},
},
flagKey: "headerColor",
context: map[string]any{},
expectedVariant: "red",
expectedValue: "#FF0000",
expectedReason: model.DefaultReason,
flagKey: "headerColor",
context: map[string]any{},
expectedVariant: "",
expectedValue: "",
expectedReason: model.ErrorReason,
expectedErrorCode: model.ParseErrorCode,
},
"fallback to default variant if invalid variant as result of fractional evaluation": {
flags: Flags{
Expand Down Expand Up @@ -222,9 +223,10 @@ func TestLegacyFractionalEvaluation(t *testing.T) {
context: map[string]any{
"email": "[email protected]",
},
expectedVariant: "red",
expectedValue: "#FF0000",
expectedReason: model.DefaultReason,
expectedVariant: "",
expectedValue: "",
expectedReason: model.ErrorReason,
expectedErrorCode: model.ParseErrorCode,
},
"fallback to default variant if percentages don't sum to 100": {
flags: Flags{
Expand Down Expand Up @@ -291,8 +293,11 @@ func TestLegacyFractionalEvaluation(t *testing.T) {
t.Errorf("expected reason '%s', got '%s'", tt.expectedReason, reason)
}

if err != tt.expectedError {
t.Errorf("expected err '%v', got '%v'", tt.expectedError, err)
if err != nil {
errorCode := err.Error()
if errorCode != tt.expectedErrorCode {
t.Errorf("expected err '%v', got '%v'", tt.expectedErrorCode, err)
}
}
})
}
Expand Down
4 changes: 2 additions & 2 deletions core/pkg/eval/semver_evaluation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ func (je *SemVerComparisonEvaluator) SemVerEvaluation(values, _ interface{}) int
actualVersion, targetVersion, operator, err := parseSemverEvaluationData(values)
if err != nil {
je.Logger.Error(fmt.Sprintf("parse sem_ver evaluation data: %v", err))
return nil
return false
}
res, err := operator.compare(actualVersion, targetVersion)
if err != nil {
je.Logger.Error(fmt.Sprintf("sem_ver evaluation: %v", err))
return nil
return false
}
return res
}
Expand Down
2 changes: 1 addition & 1 deletion core/pkg/eval/string_comparison_evaluation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (sce StringComparisonEvaluator) EndsWithEvaluation(values, _ interface{}) i
propertyValue, target, err := parseStringComparisonEvaluationData(values)
if err != nil {
sce.Logger.Error(fmt.Sprintf("parse ends_with evaluation data: %v", err))
return nil
return false
}
return strings.HasSuffix(propertyValue, target)
}
Expand Down
Loading

0 comments on commit ca38c16

Please sign in to comment.