From 505efb718628d406d56ad55e4316baccd86d5edd Mon Sep 17 00:00:00 2001 From: Scott Holodak Date: Wed, 12 Aug 2026 17:02:42 -0400 Subject: [PATCH] fix: do not log an error per evaluation for an absent string comparison property Fixes #2018. A `starts_with` / `ends_with` rule referencing a context attribute the client did not send logged at error level, with a stack trace, on every evaluation. jsonLogic resolves a missing `var` to nil, and the parse helper could not tell that apart from a wrong-typed operand, so both took the same error path. Two properties of that path made it expensive. It runs per evaluation, so its volume tracks request rate rather than the number of bad rules; and zap.Config.Build attaches a stack trace at error level, turning each occurrence into roughly forty log lines. A rule that ORs several version prefixes multiplies again -- one of ours had eight starts_with calls, so a single bulk OFREP evaluation produced several hundred lines. That combination exhausted our organisation's daily log-index quota in about nine minutes, stopping log indexing for every service we run until the quota reset. The flag itself resolved correctly throughout; the entire cost was log volume. parseStringComparisonEvaluationData now returns a distinct sentinel when the property operand is nil, letting the two cases be reported differently: an absent attribute at debug, since referencing an optional attribute is ordinary and the rule simply does not match, and a malformed rule at warn, which still surfaces a real misconfiguration but without the stack trace. Both continue to return nil to jsonLogic, so evaluation results are unchanged. The two evaluators now share the comparison and logging path, which keeps that policy in one place. Verified against the reproduction in #2018: evaluation output is byte-identical for absent, matching and wrong-typed operands, while the absent case now emits no log line at default level and the wrong-typed case emits one warn with no stack trace -- previously an error plus ~40 stack frames each. Signed-off-by: Scott Holodak --- core/pkg/evaluator/string_comparison.go | 45 ++++++++++++++++---- core/pkg/evaluator/string_comparison_test.go | 37 ++++++++++++++++ 2 files changed, 74 insertions(+), 8 deletions(-) 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)) +}