From ded13d972517263f56554427f66b9cd5a0c8311a Mon Sep 17 00:00:00 2001 From: Tony Holdstock-Brown Date: Sat, 6 Jan 2024 10:31:20 -0800 Subject: [PATCH] Allow empty expression evaluables as always-matching items (#10) --- expr.go | 54 +++++++++++++++++++++++++++++------------ expr_test.go | 65 +++++++++++++++++++++++++++++++++++++------------- parser.go | 13 +++++++--- parser_test.go | 5 ++-- 4 files changed, 100 insertions(+), 37 deletions(-) diff --git a/expr.go b/expr.go index 24ff590..fdbeacb 100644 --- a/expr.go +++ b/expr.go @@ -131,6 +131,12 @@ func (a *aggregator) Evaluate(ctx context.Context, data map[string]any) ([]Evalu // TODO: Concurrently match constant expressions using a semaphore for capacity. for _, expr := range a.constants { atomic.AddInt32(&matched, 1) + + if expr.Evaluable.GetExpression() == "" { + result = append(result, expr.Evaluable) + continue + } + // NOTE: We don't need to add lifted expression variables, // because match.Parsed.Evaluable() returns the original expression // string. @@ -249,6 +255,14 @@ func (a *aggregator) Add(ctx context.Context, eval Evaluable) (bool, error) { return false, err } + if eval.GetExpression() == "" { + // This is an empty expression which always matches. + a.lock.Lock() + a.constants = append(a.constants, parsed) + a.lock.Unlock() + return false, nil + } + aggregateable := true for _, g := range parsed.RootGroups() { ok, err := a.iterGroup(ctx, g, parsed, a.addNode) @@ -273,6 +287,10 @@ func (a *aggregator) Add(ctx context.Context, eval Evaluable) (bool, error) { } func (a *aggregator) Remove(ctx context.Context, eval Evaluable) error { + if eval.GetExpression() == "" { + return a.removeConstantEvaluable(ctx, eval) + } + // parse the expression using our tree parser. parsed, err := a.parser.Parse(ctx, eval) if err != nil { @@ -289,22 +307,9 @@ func (a *aggregator) Remove(ctx context.Context, eval Evaluable) error { return err } if !ok && aggregateable { - // Find the index of the evaluable in constants and yank out. - idx := -1 - for n, item := range a.constants { - if item.Evaluable.GetID() == eval.GetID() { - idx = n - break - } - } - - if idx == -1 { - return ErrEvaluableNotFound + if err := a.removeConstantEvaluable(ctx, eval); err != nil { + return err } - - a.lock.Lock() - a.constants = append(a.constants[:idx], a.constants[idx+1:]...) - a.lock.Unlock() aggregateable = false } } @@ -316,6 +321,25 @@ func (a *aggregator) Remove(ctx context.Context, eval Evaluable) error { return nil } +func (a *aggregator) removeConstantEvaluable(ctx context.Context, eval Evaluable) error { + // Find the index of the evaluable in constants and yank out. + idx := -1 + for n, item := range a.constants { + if item.Evaluable.GetID() == eval.GetID() { + idx = n + break + } + } + if idx == -1 { + return ErrEvaluableNotFound + } + + a.lock.Lock() + a.constants = append(a.constants[:idx], a.constants[idx+1:]...) + a.lock.Unlock() + return nil +} + func (a *aggregator) iterGroup(ctx context.Context, node *Node, parsed *ParsedExpression, op nodeOp) (bool, error) { if len(node.Ors) > 0 { // If there are additional branches, don't bother to add this to the aggregate tree. diff --git a/expr_test.go b/expr_test.go index 16d307e..641136d 100644 --- a/expr_test.go +++ b/expr_test.go @@ -20,13 +20,9 @@ func BenchmarkCachingEvaluate1_000(b *testing.B) { } // func BenchmarkNonCachingEvaluate1_000(b *testing.B) { benchEval(1_000, EnvParser(newEnv()), b) } - func benchEval(i int, p CELParser, b *testing.B) { for n := 0; n < b.N; n++ { - parser, err := NewTreeParser(p) - if err != nil { - panic(err) - } + parser := NewTreeParser(p) _ = evaluate(b, i, parser) } } @@ -61,12 +57,11 @@ func evaluate(b *testing.B, i int, parser TreeParser) error { func TestEvaluate(t *testing.T) { ctx := context.Background() - parser, err := NewTreeParser(NewCachingParser(newEnv(), nil)) - require.NoError(t, err) + parser := NewTreeParser(NewCachingParser(newEnv(), nil)) e := NewAggregateEvaluator(parser, testBoolEvaluator) expected := tex(`event.data.account_id == "yes" && event.data.match == "true"`) - _, err = e.Add(ctx, expected) + _, err := e.Add(ctx, expected) require.NoError(t, err) n := 100_000 @@ -116,12 +111,11 @@ func TestEvaluate(t *testing.T) { func TestEvaluate_Concurrently(t *testing.T) { ctx := context.Background() - parser, err := NewTreeParser(NewCachingParser(newEnv(), nil)) - require.NoError(t, err) + parser := NewTreeParser(NewCachingParser(newEnv(), nil)) e := NewAggregateEvaluator(parser, testBoolEvaluator) expected := tex(`event.data.account_id == "yes" && event.data.match == "true"`) - _, err = e.Add(ctx, expected) + _, err := e.Add(ctx, expected) require.NoError(t, err) addOtherExpressions(100_000, e) @@ -152,12 +146,11 @@ func TestEvaluate_Concurrently(t *testing.T) { func TestEvaluate_ArrayIndexes(t *testing.T) { ctx := context.Background() - parser, err := NewTreeParser(NewCachingParser(newEnv(), nil)) - require.NoError(t, err) + parser := NewTreeParser(NewCachingParser(newEnv(), nil)) e := NewAggregateEvaluator(parser, testBoolEvaluator) expected := tex(`event.data.ids[1] == "id-b" && event.data.ids[2] == "id-c"`) - _, err = e.Add(ctx, expected) + _, err := e.Add(ctx, expected) require.NoError(t, err) t.Run("It doesn't return if arrays contain non-matching data", func(t *testing.T) { @@ -199,8 +192,7 @@ func TestEvaluate_ArrayIndexes(t *testing.T) { func TestEvaluate_Compound(t *testing.T) { ctx := context.Background() - parser, err := NewTreeParser(NewCachingParser(newEnv(), nil)) - require.NoError(t, err) + parser := NewTreeParser(NewCachingParser(newEnv(), nil)) e := NewAggregateEvaluator(parser, testBoolEvaluator) expected := tex(`event.data.a == "ok" && event.data.b == "yes" && event.data.c == "please"`) @@ -449,6 +441,47 @@ func TestAddRemove(t *testing.T) { }) } +func TestEmptyExpressions(t *testing.T) { + ctx := context.Background() + parser, err := newParser() + require.NoError(t, err) + + e := NewAggregateEvaluator(parser, testBoolEvaluator) + + empty := tex(``, "id-1") + + t.Run("Adding an empty expression succeeds", func(t *testing.T) { + ok, err := e.Add(ctx, empty) + require.NoError(t, err) + require.False(t, ok) + require.Equal(t, 1, e.Len()) + require.Equal(t, 1, e.ConstantLen()) + require.Equal(t, 0, e.AggregateableLen()) + }) + + t.Run("Empty expressions always match", func(t *testing.T) { + // Matching this expr should now fail. + eval, count, err := e.Evaluate(ctx, map[string]any{ + "event": map[string]any{ + "data": map[string]any{"any": true}, + }, + }) + require.NoError(t, err) + require.EqualValues(t, 1, count) + require.EqualValues(t, 1, len(eval)) + require.EqualValues(t, empty, eval[0]) + }) + + t.Run("Removing an empty expression succeeds", func(t *testing.T) { + err := e.Remove(ctx, empty) + require.NoError(t, err) + require.Equal(t, 0, e.Len()) + require.Equal(t, 0, e.ConstantLen()) + require.Equal(t, 0, e.AggregateableLen()) + }) + +} + // tex represents a test Evaluable expression func tex(expr string, ids ...string) Evaluable { return testEvaluable{ diff --git a/parser.go b/parser.go index 7e2c220..2f72233 100644 --- a/parser.go +++ b/parser.go @@ -46,11 +46,11 @@ func (e envparser) Parse(txt string) (*cel.Ast, *cel.Issues, LiftedArgs) { } // NewTreeParser returns a new tree parser for a given *cel.Env -func NewTreeParser(ep CELParser) (TreeParser, error) { +func NewTreeParser(ep CELParser) TreeParser { parser := &parser{ ep: ep, } - return parser, nil + return parser } type parser struct { @@ -63,7 +63,14 @@ type parser struct { } func (p *parser) Parse(ctx context.Context, eval Evaluable) (*ParsedExpression, error) { - ast, issues, vars := p.ep.Parse(eval.GetExpression()) + expression := eval.GetExpression() + if expression == "" { + return &ParsedExpression{ + Evaluable: eval, + }, nil + } + + ast, issues, vars := p.ep.Parse(expression) if issues != nil { return nil, issues.Err() } diff --git a/parser_test.go b/parser_test.go index 9a7b66a..4639baa 100644 --- a/parser_test.go +++ b/parser_test.go @@ -20,7 +20,7 @@ func newEnv() *cel.Env { } func newParser() (TreeParser, error) { - return NewTreeParser(EnvParser(newEnv())) + return NewTreeParser(EnvParser(newEnv())), nil } type parseTestInput struct { @@ -1093,10 +1093,9 @@ func TestParse_LiftedVars(t *testing.T) { t.Helper() for _, test := range tests { - p, err := NewTreeParser(cachingCelParser) + p := NewTreeParser(cachingCelParser) // overwrite rander so that the parser uses the same nil bytes p.(*parser).rander = rander - require.NoError(t, err) eval := tex(test.input) actual, err := p.Parse(ctx, eval)