diff --git a/core/pkg/evaluator/string_comparison.go b/core/pkg/evaluator/string_comparison.go index 9449b9430..b8e6bdf40 100644 --- a/core/pkg/evaluator/string_comparison.go +++ b/core/pkg/evaluator/string_comparison.go @@ -13,6 +13,14 @@ const ( EndsWithEvaluationName = "ends_with" ) +// errPropertyAbsent signals that the property operand resolved to nil, meaning +// the evaluation context simply did not carry the attribute. Targeting rules +// routinely reference optional attributes, so this is an ordinary condition +// rather than a misconfiguration, and is reported separately from a genuine +// type error so the two can be logged differently. +var errPropertyAbsent = errors.New( + "[start/end]s_with evaluation: property is absent from the evaluation context") + type StringComparisonEvaluator struct { Logger *logger.Logger } @@ -41,12 +49,32 @@ func NewStringComparisonEvaluator(log *logger.Logger) *StringComparisonEvaluator // Note that the 'starts_with' evaluation rule must contain exactly two items, which both resolve to a // string value func (sce *StringComparisonEvaluator) StartsWithEvaluation(values, _ interface{}) interface{} { + return sce.evaluate(StartsWithEvaluationName, values, strings.HasPrefix) +} + +// evaluate applies cmp to the parsed operands, returning nil -- which jsonLogic +// treats as falsy -- when they cannot be used. +// +// The logging severity here is deliberate. This runs on every evaluation of a +// rule, so its volume tracks request rate rather than the number of bad rules, +// and error level additionally attaches a stack trace to each occurrence. A rule +// referencing an attribute that a client did not send would therefore emit tens +// of log lines per evaluation, for a condition that is entirely normal. An +// absent property is logged at debug; a genuinely malformed rule is logged at +// warn, which still surfaces it but without the stack trace. +func (sce *StringComparisonEvaluator) evaluate( + name string, values interface{}, cmp func(string, string) bool, +) interface{} { propertyValue, target, err := parseStringComparisonEvaluationData(values) if err != nil { - sce.Logger.Error(fmt.Sprintf("parse starts_with evaluation data: %v", err)) + if errors.Is(err, errPropertyAbsent) { + sce.Logger.Debug(fmt.Sprintf("%s: %v", name, err)) + } else { + sce.Logger.Warn(fmt.Sprintf("parse %s evaluation data: %v", name, err)) + } return nil } - return strings.HasPrefix(propertyValue, target) + return cmp(propertyValue, target) } // EndsWithEvaluation checks if the given property ends with a certain prefix. @@ -69,12 +97,7 @@ func (sce *StringComparisonEvaluator) StartsWithEvaluation(values, _ interface{} // Note that the 'ends_with' evaluation rule must contain exactly two items, which both resolve to a // string value func (sce *StringComparisonEvaluator) EndsWithEvaluation(values, _ interface{}) interface{} { - propertyValue, target, err := parseStringComparisonEvaluationData(values) - if err != nil { - sce.Logger.Error(fmt.Sprintf("parse ends_with evaluation data: %v", err)) - return nil - } - return strings.HasSuffix(propertyValue, target) + return sce.evaluate(EndsWithEvaluationName, values, strings.HasSuffix) } // parseStringComparisonEvaluationData tries to parse the input for the starts_with/ends_with evaluation. @@ -111,6 +134,12 @@ func parseStringComparisonEvaluationData(values interface{}) (string, string, er return "", "", errors.New("[start/end]s_with evaluation must contain a value and a comparison target") } + // jsonLogic resolves a `var` referencing a missing attribute to nil, so this + // distinguishes "the context did not carry it" from "it was the wrong type". + if parsed[0] == nil { + return "", "", errPropertyAbsent + } + property, ok := parsed[0].(string) if !ok { return "", "", errors.New("[start/end]s_with evaluation: property did not resolve to a string value") diff --git a/core/pkg/evaluator/string_comparison_test.go b/core/pkg/evaluator/string_comparison_test.go index 9049caf15..f206d1bfd 100644 --- a/core/pkg/evaluator/string_comparison_test.go +++ b/core/pkg/evaluator/string_comparison_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/open-feature/flagd/core/pkg/logger" "github.com/open-feature/flagd/core/pkg/model" "github.com/stretchr/testify/assert" ) @@ -372,3 +373,39 @@ func TestStringComparisonEvaluation_ErrorFallbackWhenUsedDirectly(t *testing.T) runErrorFallbackTests(t, ctx, source, "string-op-error-fallback", tests) } + +// A targeting rule routinely references an optional attribute that a given +// client did not send. jsonLogic resolves that to nil, which must be +// distinguishable from an attribute of the wrong type so the two can be +// reported at different severities. +func Test_parseStringComparisonEvaluationData_absentProperty(t *testing.T) { + _, _, err := parseStringComparisonEvaluationData([]interface{}{nil, "prefix"}) + + assert.ErrorIs(t, err, errPropertyAbsent) +} + +func Test_parseStringComparisonEvaluationData_wrongTypeIsNotAbsent(t *testing.T) { + _, _, err := parseStringComparisonEvaluationData([]interface{}{1, "prefix"}) + + assert.Error(t, err) + assert.NotErrorIs(t, err, errPropertyAbsent, + "a wrong-typed property is a misconfiguration and must not be treated as an absent one") +} + +// Whatever the operands, the rule must simply fail to match rather than error +// the evaluation. +func TestStringComparisonEvaluation_absentPropertyDoesNotMatch(t *testing.T) { + sce := NewStringComparisonEvaluator(logger.NewLogger(nil, false)) + + assert.Nil(t, sce.StartsWithEvaluation([]interface{}{nil, "prefix"}, nil)) + assert.Nil(t, sce.EndsWithEvaluation([]interface{}{nil, "suffix"}, nil)) +} + +func TestStringComparisonEvaluation_comparesStrings(t *testing.T) { + sce := NewStringComparisonEvaluator(logger.NewLogger(nil, false)) + + assert.Equal(t, true, sce.StartsWithEvaluation([]interface{}{"user@faas.com", "user@"}, nil)) + assert.Equal(t, false, sce.StartsWithEvaluation([]interface{}{"user@faas.com", "admin@"}, nil)) + assert.Equal(t, true, sce.EndsWithEvaluation([]interface{}{"user@faas.com", ".com"}, nil)) + assert.Equal(t, false, sce.EndsWithEvaluation([]interface{}{"user@faas.com", ".org"}, nil)) +}