From 10a9add1db38fee1561e146d0ad7644225bdd140 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Mon, 7 Aug 2023 11:50:14 +0500 Subject: [PATCH 1/9] feat(expression): added full expression validation - included tests and benchmarks --- expression/expression.go | 66 ++++++- expression/expression_test.go | 176 ++++++++++++++---- go.mod | 4 +- go.sum | 5 +- .../expression/expression_test.go | 84 ++++++++- 5 files changed, 291 insertions(+), 44 deletions(-) diff --git a/expression/expression.go b/expression/expression.go index 690a4c486..7db85dc62 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -4,9 +4,13 @@ import ( "fmt" "strings" + "github.com/antonmedv/expr" + "github.com/antonmedv/expr/ast" + "github.com/antonmedv/expr/vm" "github.com/patrickmn/go-cache" "github.com/Knetic/govaluate" + "github.com/moira-alert/moira" ) @@ -77,6 +81,20 @@ func (triggerExpression TriggerExpression) Get(name string) (interface{}, error) } } +// Visit implements expr.Visitor interface. +// +// It replaces all identifiers (t1, t2, ..tN) with Get("t1"), Get("t2"), ..Get("tN") +func (triggerExpression TriggerExpression) Visit(node *ast.Node) { + if n, ok := (*node).(*ast.IdentifierNode); ok { + ast.Patch(node, &ast.CallNode{ + Arguments: []ast.Node{ + &ast.StringNode{Value: n.Value}, + }, + Callee: &ast.IdentifierNode{Value: "Get"}, + }) + } +} + // Evaluate gets trigger expression and evaluates it for given parameters using govaluate func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { expr, err := getExpression(triggerExpression) @@ -95,7 +113,51 @@ func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { } } -func validateUserExpression(triggerExpression *TriggerExpression, userExpression *govaluate.EvaluableExpression) (*govaluate.EvaluableExpression, error) { +// Validate gets trigger expression and validates it for given parameters using expr +func (triggerExpression *TriggerExpression) Validate() error { + expression, err := getExpression(triggerExpression) + if err != nil { + return ErrInvalidExpression{internalError: err} + } + + cacheKey := fmt.Sprintf("[VALIDATED]%s", expression.String()) + pr, found := exprCache.Get(cacheKey) + program, ok := pr.(*vm.Program) + if !ok { + found = false + exprCache.Delete(cacheKey) + } + if !found { + program, err = expr.Compile( + expression.String(), + expr.Env(map[string]interface{}{ + "Get": triggerExpression.Get, + }), + expr.ConstExpr("Get"), + expr.Patch(triggerExpression), // patch identifiers with call to Get + expr.Optimize(true), // collapse evaluation branches + ) + if err != nil { + return err + } + exprCache.Set(cacheKey, program, cache.NoExpiration) + } + result, err := expr.Run(program, nil) + if err != nil { + return err + } + switch result.(type) { + case moira.State: + return nil + default: + return ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} + } +} + +func validateExpressionVariables( + triggerExpression *TriggerExpression, + userExpression *govaluate.EvaluableExpression, +) (*govaluate.EvaluableExpression, error) { for _, v := range userExpression.Vars() { if _, err := triggerExpression.Get(v); err != nil { return nil, fmt.Errorf("invalid variable value: %w", err) @@ -115,7 +177,7 @@ func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableEx return nil, err } - return validateUserExpression(triggerExpression, userExpression) + return validateExpressionVariables(triggerExpression, userExpression) } return getSimpleExpression(triggerExpression) } diff --git a/expression/expression_test.go b/expression/expression_test.go index 090f0d9b2..450af7c64 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -4,8 +4,9 @@ import ( "fmt" "testing" - "github.com/moira-alert/moira" . "github.com/smartystreets/goconvey/convey" + + "github.com/moira-alert/moira" ) type getExpressionValuesTest struct { @@ -19,82 +20,137 @@ func TestExpression(t *testing.T) { Convey("Test Default", t, func() { warnValue := 60.0 errorValue := 90.0 - result, err := (&TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression := &TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} + result, err1 := expression.Evaluate() + err2 := expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) - result, err = (&TriggerExpression{MainTargetValue: 60.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 60.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) - result, err = (&TriggerExpression{MainTargetValue: 90.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 90.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) warnValue = 30.0 errorValue = 10.0 - result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) - result, err = (&TriggerExpression{MainTargetValue: 20.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 20.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) - result, err = (&TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) - result, err = (&TriggerExpression{MainTargetValue: 10.0, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("error value and warning value can not be empty")}) - So(err.Error(), ShouldResemble, "error value and warning value can not be empty") + expression = &TriggerExpression{MainTargetValue: 10.0, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1.Error(), ShouldResemble, "error value and warning value can not be empty") + So(err2.Error(), ShouldResemble, "error value and warning value can not be empty") So(result, ShouldBeEmpty) warnValue = 30.0 - result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.RisingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.RisingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) warnValue = 30.0 - result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) errorValue = 30.0 - result, err = (&TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) errorValue = 30.0 - result, err = (&TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() - So(err, ShouldBeNil) + expression = &TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} + result, err1 = expression.Evaluate() + err2 = expression.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) }) Convey("Test Custom", t, func() { expression := "t1 > 10 && t2 > 3 ? ERROR : OK" - result, err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldBeNil) + trigger := &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} + result, err1 := trigger.Evaluate() + err2 := trigger.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) expression = "min(t1, t2) > 10 ? ERROR : OK" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) + trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} + result, err1 = trigger.Evaluate() + err2 = trigger.Validate() + So(err1, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) + So(err2, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) So(result, ShouldBeEmpty) expression = "PREV_STATE" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA}).Evaluate() - So(err, ShouldBeNil) + trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA} + result, err1 = trigger.Evaluate() + err2 = trigger.Validate() + So(err1, ShouldBeNil) + So(err2, ShouldBeNil) So(result, ShouldResemble, moira.StateNODATA) expression = "t1 > 10 && t2 > 3 ? OK : ddd" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name ddd"))}) + trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} + result, err1 = trigger.Evaluate() + err2 = trigger.Validate() + So( + err1, + ShouldResemble, + ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name ddd"))}, + ) + So(err2, ShouldNotBeNil) So(result, ShouldBeEmpty) expression = "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name t2"))}) + trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger} + result, err1 = trigger.Evaluate() + err2 = trigger.Validate() + So( + err1, + ShouldResemble, + ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name t2"))}, + ) + So(err2, ShouldNotBeNil) So(result, ShouldBeEmpty) }) } @@ -225,6 +281,58 @@ func TestGetExpressionValue(t *testing.T) { }) } +func TestExpressionValidate(t *testing.T) { + warnValue := float64(60) + errorValue := float64(90) + Convey("Default Expressions", t, func() { + expressions := []string{ + "t1 >= ERROR_VALUE ? ERROR : (t1 >= WARN_VALUE ? WARN : OK)", + "t1 <= ERROR_VALUE ? ERROR : (t1 <= WARN_VALUE ? WARN : OK)", + "t1 >= WARN_VALUE ? WARN : OK", + "t1 >= ERROR_VALUE ? ERROR : OK", + "t1 <= WARN_VALUE ? WARN : OK", + "t1 <= ERROR_VALUE ? ERROR : OK", + "WARN", + } + for _, expr := range expressions { + triggerExpression := TriggerExpression{ + Expression: ptr(expr), + WarnValue: &warnValue, + ErrorValue: &errorValue, + TriggerType: moira.ExpressionTrigger, + } + err := triggerExpression.Validate() + So(err, ShouldBeNil) + } + }) + Convey("Invalids", t, func() { + expressions := []string{ + "t1 > 10 ? ok : (t2 * 10 : ddd ? warn)", + "t1 < WARN_VALUE ? ok ?", + "t1 > 10 ? ok : (t2 * 10 : error ? warn)", + } + for _, expr := range expressions { + triggerExpression := TriggerExpression{ + Expression: ptr(expr), + WarnValue: &warnValue, + ErrorValue: &errorValue, + TriggerType: moira.ExpressionTrigger, + AdditionalTargetsValues: map[string]float64{ + "t2": 2, + "t3": 3, + }, + } + err := triggerExpression.Validate() + t.Log(err) + So(err, ShouldNotBeNil) + } + }) +} + +func ptr[T any](obj T) *T { + return &obj +} + func runGetExpressionValuesTest(getExpressionValuesTests []getExpressionValuesTest) { for _, getExpressionValuesTest := range getExpressionValuesTests { result, err := getExpressionValuesTest.values.Get(getExpressionValuesTest.name) diff --git a/go.mod b/go.mod index 14e7107fe..a65f16f8f 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible github.com/PagerDuty/go-pagerduty v1.5.1 github.com/ansel1/merry v1.6.2 + github.com/antonmedv/expr v1.12.7 github.com/aws/aws-sdk-go v1.44.219 github.com/blevesearch/bleve/v2 v2.3.8 github.com/bwmarrin/discordgo v0.25.0 @@ -27,6 +28,7 @@ require ( github.com/gregdel/pushover v1.1.0 github.com/karriereat/blackfriday-slack v0.1.0 github.com/mattermost/mattermost-server/v6 v6.0.0-20230405170428-2a75f997ee6c // it is last commit of 7.9.2 (https://github.com/mattermost/mattermost-server/commits/v7.9.2). Can't use v7, issue https://github.com/mattermost/mattermost-server/issues/20817 + github.com/mitchellh/mapstructure v1.5.0 github.com/moira-alert/go-chart v0.0.0-20230220064910-812fb2829b9b github.com/opsgenie/opsgenie-go-sdk-v2 v1.2.13 github.com/patrickmn/go-cache v2.1.0+incompatible @@ -47,8 +49,6 @@ require ( gopkg.in/yaml.v2 v2.4.0 ) -require github.com/mitchellh/mapstructure v1.5.0 - require ( bitbucket.org/tebeka/strftime v0.0.0-20140926081919-2194253a23c0 // indirect github.com/JaderDias/movingmedian v0.0.0-20220813210630-d8c6b6de8835 // indirect diff --git a/go.sum b/go.sum index d38ec7ff7..f3b17f54c 100644 --- a/go.sum +++ b/go.sum @@ -438,13 +438,14 @@ github.com/ansel1/merry/v2 v2.1.1 h1:Ax0gQh7Z/GfimoVg2EDBAU6CJIieWwVvhtBKJdkCE1M github.com/ansel1/merry/v2 v2.1.1/go.mod h1:4p/FFyQbCgqlDbseWOVQaL5USpgkE9sr5xh4V6Ry0JU= github.com/ansel1/vespucci/v4 v4.1.1/go.mod h1:zzdrO4IgBfgcGMbGTk/qNGL8JPslmW3nPpcBHKReFYY= github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY= +github.com/antonmedv/expr v1.12.7 h1:jfV/l/+dHWAadLwAtESXNxXdfbK9bE4+FNMHYCMntwk= +github.com/antonmedv/expr v1.12.7/go.mod h1:FPC8iWArxls7axbVLsW+kpg1mz29A1b2M6jt+hZfDkU= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= github.com/aws/aws-sdk-go v1.44.219 h1:YOFxTUQZvdRzgwb6XqLFRwNHxoUdKBuunITC7IFhvbc= github.com/aws/aws-sdk-go v1.44.219/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= github.com/barkimedes/go-deepcopy v0.0.0-20220514131651-17c30cfc62df h1:GSoSVRLoBaFpOOds6QyY1L8AX7uoY+Ln3BHc22W40X0= github.com/barkimedes/go-deepcopy v0.0.0-20220514131651-17c30cfc62df/go.mod h1:hiVxq5OP2bUGBRNS3Z/bt/reCLFNbdcST6gISi1fiOM= github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8= -github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= @@ -689,7 +690,6 @@ github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= -github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs= github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= @@ -1111,7 +1111,6 @@ go.uber.org/atomic v1.10.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0 go.uber.org/automaxprocs v1.5.1 h1:e1YG66Lrk73dn4qhg8WFSvhF0JuFQF0ERIp4rpuV8Qk= go.uber.org/automaxprocs v1.5.1/go.mod h1:BF4eumQw0P9GtnuxxovUd06vwm1o18oMzFtK66vU6XU= go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI= -go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ= go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU= go.uber.org/multierr v1.8.0 h1:dg6GjLku4EH+249NNmoIciG9N/jURbDG+pFlTkhzIC8= go.uber.org/multierr v1.8.0/go.mod h1:7EAYxJLBy9rStEaz58O2t4Uvip6FSURkq8/ppBp95ak= diff --git a/perfomance_tests/expression/expression_test.go b/perfomance_tests/expression/expression_test.go index d979a479a..9add4af8e 100644 --- a/perfomance_tests/expression/expression_test.go +++ b/perfomance_tests/expression/expression_test.go @@ -3,6 +3,7 @@ package expression import ( "testing" + "github.com/moira-alert/moira" "github.com/moira-alert/moira/expression" ) @@ -13,9 +14,13 @@ func BenchmarkDefault1Expr(b *testing.B) { MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, + TriggerType: moira.RisingTrigger, } for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } } } @@ -26,9 +31,13 @@ func BenchmarkDefault2Expr(b *testing.B) { MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, + TriggerType: moira.RisingTrigger, } for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } } } @@ -36,9 +45,78 @@ func BenchmarkCustomExpr(b *testing.B) { expressionStr := "t1 > 10 && t2 > 3 ? ERROR : OK" expr := &expression.TriggerExpression{ Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}} for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } + } +} + +func BenchmarkValidate(b *testing.B) { + expressionStr := "t1 > 10 && t2 > 3 ? ERROR : OK" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 11.0, + AdditionalTargetsValues: map[string]float64{"t2": 4.0}} + for i := 0; i < b.N; i++ { + err := expr.Validate() + if err != nil { + b.Log(err) + } + } +} + +func BenchmarkEvaluateComplex(b *testing.B) { + expressionStr := "(t1 % 2 == 0 && t2 % 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 4, + AdditionalTargetsValues: map[string]float64{ + "t2": 5, + "t3": 3, + "t4": 6, + "t5": 18, + "t6": 10, + "t7": 15, + "t8": 20, + "t9": 10, + }, + } + for i := 0; i < b.N; i++ { + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } + } +} + +func BenchmarkValidateComplex(b *testing.B) { + expressionStr := "(t1 % 2 == 0 && t2 % 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 4, + AdditionalTargetsValues: map[string]float64{ + "t2": 5, + "t3": 3, + "t4": 6, + "t5": 18, + "t6": 10, + "t7": 15, + "t8": 20, + "t9": 10, + }, + } + for i := 0; i < b.N; i++ { + err := expr.Validate() + if err != nil { + b.Log(err) + } } } From 7bb6f5af4c68bf4410663c5954f0834e391591c7 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Mon, 7 Aug 2023 11:59:16 +0500 Subject: [PATCH 2/9] fix(expression): avoid optimization of Get calls - optimizing get calls can make vm.Program return wrong values because variables have been replaced before caching, and new set `WARN_VALUE`, `t1, t2, ...` are not evaluated properly. - If validation of variables is not necessary, this commit can be reverted for speed purpose --- api/dto/triggers.go | 4 ++-- expression/expression.go | 10 ++++------ perfomance_tests/expression/expression_test.go | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/api/dto/triggers.go b/api/dto/triggers.go index 300df70af..128b83bd4 100644 --- a/api/dto/triggers.go +++ b/api/dto/triggers.go @@ -178,7 +178,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { return err } - if err := checkTTLSanity(trigger, metricsSource); err != nil { + if err = checkTTLSanity(trigger, metricsSource); err != nil { return api.ErrInvalidRequestContent{ValidationError: err} } @@ -195,7 +195,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { middleware.SetTimeSeriesNames(request, metricsDataNames) - if _, err := triggerExpression.Evaluate(); err != nil { + if err = triggerExpression.Validate(); err != nil { return err } diff --git a/expression/expression.go b/expression/expression.go index 7db85dc62..6b1aafdd3 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -130,19 +130,17 @@ func (triggerExpression *TriggerExpression) Validate() error { if !found { program, err = expr.Compile( expression.String(), - expr.Env(map[string]interface{}{ - "Get": triggerExpression.Get, - }), - expr.ConstExpr("Get"), expr.Patch(triggerExpression), // patch identifiers with call to Get - expr.Optimize(true), // collapse evaluation branches + expr.Optimize(true), ) if err != nil { return err } exprCache.Set(cacheKey, program, cache.NoExpiration) } - result, err := expr.Run(program, nil) + result, err := expr.Run(program, map[string]interface{}{ + "Get": triggerExpression.Get, + }) if err != nil { return err } diff --git a/perfomance_tests/expression/expression_test.go b/perfomance_tests/expression/expression_test.go index 9add4af8e..380db8112 100644 --- a/perfomance_tests/expression/expression_test.go +++ b/perfomance_tests/expression/expression_test.go @@ -72,7 +72,7 @@ func BenchmarkValidate(b *testing.B) { } func BenchmarkEvaluateComplex(b *testing.B) { - expressionStr := "(t1 % 2 == 0 && t2 % 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" expr := &expression.TriggerExpression{ Expression: &expressionStr, TriggerType: moira.ExpressionTrigger, @@ -97,7 +97,7 @@ func BenchmarkEvaluateComplex(b *testing.B) { } func BenchmarkValidateComplex(b *testing.B) { - expressionStr := "(t1 % 2 == 0 && t2 % 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" expr := &expression.TriggerExpression{ Expression: &expressionStr, TriggerType: moira.ExpressionTrigger, From 7f76f69756bbdcc722bc2790a13bc1017a10a2d4 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Tue, 8 Aug 2023 13:28:47 +0500 Subject: [PATCH 3/9] removed redundant ptr function --- expression/expression_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/expression/expression_test.go b/expression/expression_test.go index 450af7c64..14af1d257 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -296,7 +296,7 @@ func TestExpressionValidate(t *testing.T) { } for _, expr := range expressions { triggerExpression := TriggerExpression{ - Expression: ptr(expr), + Expression: &expr, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.ExpressionTrigger, @@ -313,7 +313,7 @@ func TestExpressionValidate(t *testing.T) { } for _, expr := range expressions { triggerExpression := TriggerExpression{ - Expression: ptr(expr), + Expression: &expr, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.ExpressionTrigger, @@ -329,10 +329,6 @@ func TestExpressionValidate(t *testing.T) { }) } -func ptr[T any](obj T) *T { - return &obj -} - func runGetExpressionValuesTest(getExpressionValuesTests []getExpressionValuesTest) { for _, getExpressionValuesTest := range getExpressionValuesTests { result, err := getExpressionValuesTest.values.Get(getExpressionValuesTest.name) From f32649f04bbbf8579039e6c778bbe4df997f22a2 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Tue, 8 Aug 2023 16:59:05 +0500 Subject: [PATCH 4/9] removed Validate function & limited expr validation to user expr --- api/dto/triggers.go | 2 +- expression/expression.go | 107 +++++------ expression/expression_test.go | 173 ++++-------------- .../expression/expression_test.go | 40 ---- 4 files changed, 87 insertions(+), 235 deletions(-) diff --git a/api/dto/triggers.go b/api/dto/triggers.go index 128b83bd4..f9bc797c4 100644 --- a/api/dto/triggers.go +++ b/api/dto/triggers.go @@ -195,7 +195,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { middleware.SetTimeSeriesNames(request, metricsDataNames) - if err = triggerExpression.Validate(); err != nil { + if _, err = triggerExpression.Evaluate(); err != nil { return err } diff --git a/expression/expression.go b/expression/expression.go index 6b1aafdd3..066e6fa40 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/antonmedv/expr" - "github.com/antonmedv/expr/ast" "github.com/antonmedv/expr/vm" "github.com/patrickmn/go-cache" @@ -81,20 +80,6 @@ func (triggerExpression TriggerExpression) Get(name string) (interface{}, error) } } -// Visit implements expr.Visitor interface. -// -// It replaces all identifiers (t1, t2, ..tN) with Get("t1"), Get("t2"), ..Get("tN") -func (triggerExpression TriggerExpression) Visit(node *ast.Node) { - if n, ok := (*node).(*ast.IdentifierNode); ok { - ast.Patch(node, &ast.CallNode{ - Arguments: []ast.Node{ - &ast.StringNode{Value: n.Value}, - }, - Callee: &ast.IdentifierNode{Value: "Get"}, - }) - } -} - // Evaluate gets trigger expression and evaluates it for given parameters using govaluate func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { expr, err := getExpression(triggerExpression) @@ -113,55 +98,62 @@ func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { } } -// Validate gets trigger expression and validates it for given parameters using expr -func (triggerExpression *TriggerExpression) Validate() error { - expression, err := getExpression(triggerExpression) - if err != nil { - return ErrInvalidExpression{internalError: err} - } - - cacheKey := fmt.Sprintf("[VALIDATED]%s", expression.String()) - pr, found := exprCache.Get(cacheKey) - program, ok := pr.(*vm.Program) - if !ok { - found = false - exprCache.Delete(cacheKey) - } - if !found { - program, err = expr.Compile( - expression.String(), - expr.Patch(triggerExpression), // patch identifiers with call to Get - expr.Optimize(true), - ) - if err != nil { - return err +// validateUserExpression uses expr package to validate trigger expressions +func validateUserExpression( + triggerExpression *TriggerExpression, + userExpression *govaluate.EvaluableExpression, +) (*govaluate.EvaluableExpression, error) { + expression := *triggerExpression.Expression + + cacheKey := fmt.Sprintf("[VALIDATED]%s", expression) + pr, validated := exprCache.Get(cacheKey) + if validated { + _, ok := pr.(*vm.Program) + if ok { + return userExpression, nil + } else { + validated = false + exprCache.Delete(cacheKey) } - exprCache.Set(cacheKey, program, cache.NoExpiration) } - result, err := expr.Run(program, map[string]interface{}{ - "Get": triggerExpression.Get, - }) + + env := map[string]interface{}{ + "ok": moira.StateOK, + "error": moira.StateERROR, + "warn": moira.StateWARN, + "warning": moira.StateWARN, + "nodata": moira.StateNODATA, + "t1": triggerExpression.MainTargetValue, + "prev_state": triggerExpression.PreviousState, + } + if triggerExpression.WarnValue != nil { + env["warn_value"] = *triggerExpression.WarnValue + } + if triggerExpression.ErrorValue != nil { + env["error_value"] = *triggerExpression.ErrorValue + } + for k, v := range triggerExpression.AdditionalTargetsValues { + env[k] = v + } + program, err := expr.Compile( + strings.ToLower(expression), + expr.Optimize(true), + expr.Env(env), + ) if err != nil { - return err + return nil, err + } + result, err := expr.Run(program, env) + if err != nil { + return nil, err } switch result.(type) { case moira.State: - return nil + exprCache.Set(cacheKey, program, cache.NoExpiration) + return userExpression, nil default: - return ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} - } -} - -func validateExpressionVariables( - triggerExpression *TriggerExpression, - userExpression *govaluate.EvaluableExpression, -) (*govaluate.EvaluableExpression, error) { - for _, v := range userExpression.Vars() { - if _, err := triggerExpression.Get(v); err != nil { - return nil, fmt.Errorf("invalid variable value: %w", err) - } + return nil, ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} } - return userExpression, nil } func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { @@ -174,8 +166,7 @@ func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableEx if err != nil { return nil, err } - - return validateExpressionVariables(triggerExpression, userExpression) + return validateUserExpression(triggerExpression, userExpression) } return getSimpleExpression(triggerExpression) } diff --git a/expression/expression_test.go b/expression/expression_test.go index 14af1d257..41ef59952 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -20,137 +20,86 @@ func TestExpression(t *testing.T) { Convey("Test Default", t, func() { warnValue := 60.0 errorValue := 90.0 - expression := &TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} - result, err1 := expression.Evaluate() - err2 := expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err := (&TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) - expression = &TriggerExpression{MainTargetValue: 60.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 60.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) - expression = &TriggerExpression{MainTargetValue: 90.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 90.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) warnValue = 30.0 errorValue = 10.0 - expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) - expression = &TriggerExpression{MainTargetValue: 20.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 20.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) - expression = &TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) - expression = &TriggerExpression{MainTargetValue: 10.0, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1.Error(), ShouldResemble, "error value and warning value can not be empty") - So(err2.Error(), ShouldResemble, "error value and warning value can not be empty") + result, err = (&TriggerExpression{MainTargetValue: 10.0, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("error value and warning value can not be empty")}) + So(err.Error(), ShouldResemble, "error value and warning value can not be empty") So(result, ShouldBeEmpty) warnValue = 30.0 - expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.RisingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.RisingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateWARN) warnValue = 30.0 - expression = &TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 40.0, WarnValue: &warnValue, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) errorValue = 30.0 - expression = &TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) errorValue = 30.0 - expression = &TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger} - result, err1 = expression.Evaluate() - err2 = expression.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{MainTargetValue: 40.0, ErrorValue: &errorValue, TriggerType: moira.FallingTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateOK) }) Convey("Test Custom", t, func() { expression := "t1 > 10 && t2 > 3 ? ERROR : OK" - trigger := &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} - result, err1 := trigger.Evaluate() - err2 := trigger.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateERROR) expression = "min(t1, t2) > 10 ? ERROR : OK" - trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} - result, err1 = trigger.Evaluate() - err2 = trigger.Validate() - So(err1, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) - So(err2, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) + result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() + So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("functions is forbidden")}) So(result, ShouldBeEmpty) expression = "PREV_STATE" - trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA} - result, err1 = trigger.Evaluate() - err2 = trigger.Validate() - So(err1, ShouldBeNil) - So(err2, ShouldBeNil) + result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA}).Evaluate() + So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateNODATA) expression = "t1 > 10 && t2 > 3 ? OK : ddd" - trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger} - result, err1 = trigger.Evaluate() - err2 = trigger.Validate() - So( - err1, - ShouldResemble, - ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name ddd"))}, - ) - So(err2, ShouldNotBeNil) + result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() + So(err.Error(), ShouldResemble, `unknown name ddd (1:26) + | t1 > 10 && t2 > 3 ? ok : ddd + | .........................^`) So(result, ShouldBeEmpty) expression = "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" - trigger = &TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger} - result, err1 = trigger.Evaluate() - err2 = trigger.Validate() - So( - err1, - ShouldResemble, - ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name t2"))}, - ) - So(err2, ShouldNotBeNil) + result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Evaluate() + So(err.Error(), ShouldResemble, `unknown name t2 (1:17) + | t1 > 10 ? ok : (t2 < 5 ? warn : error) + | ................^`) So(result, ShouldBeEmpty) }) } @@ -281,54 +230,6 @@ func TestGetExpressionValue(t *testing.T) { }) } -func TestExpressionValidate(t *testing.T) { - warnValue := float64(60) - errorValue := float64(90) - Convey("Default Expressions", t, func() { - expressions := []string{ - "t1 >= ERROR_VALUE ? ERROR : (t1 >= WARN_VALUE ? WARN : OK)", - "t1 <= ERROR_VALUE ? ERROR : (t1 <= WARN_VALUE ? WARN : OK)", - "t1 >= WARN_VALUE ? WARN : OK", - "t1 >= ERROR_VALUE ? ERROR : OK", - "t1 <= WARN_VALUE ? WARN : OK", - "t1 <= ERROR_VALUE ? ERROR : OK", - "WARN", - } - for _, expr := range expressions { - triggerExpression := TriggerExpression{ - Expression: &expr, - WarnValue: &warnValue, - ErrorValue: &errorValue, - TriggerType: moira.ExpressionTrigger, - } - err := triggerExpression.Validate() - So(err, ShouldBeNil) - } - }) - Convey("Invalids", t, func() { - expressions := []string{ - "t1 > 10 ? ok : (t2 * 10 : ddd ? warn)", - "t1 < WARN_VALUE ? ok ?", - "t1 > 10 ? ok : (t2 * 10 : error ? warn)", - } - for _, expr := range expressions { - triggerExpression := TriggerExpression{ - Expression: &expr, - WarnValue: &warnValue, - ErrorValue: &errorValue, - TriggerType: moira.ExpressionTrigger, - AdditionalTargetsValues: map[string]float64{ - "t2": 2, - "t3": 3, - }, - } - err := triggerExpression.Validate() - t.Log(err) - So(err, ShouldNotBeNil) - } - }) -} - func runGetExpressionValuesTest(getExpressionValuesTests []getExpressionValuesTest) { for _, getExpressionValuesTest := range getExpressionValuesTests { result, err := getExpressionValuesTest.values.Get(getExpressionValuesTest.name) diff --git a/perfomance_tests/expression/expression_test.go b/perfomance_tests/expression/expression_test.go index 380db8112..4b0b84d52 100644 --- a/perfomance_tests/expression/expression_test.go +++ b/perfomance_tests/expression/expression_test.go @@ -56,21 +56,6 @@ func BenchmarkCustomExpr(b *testing.B) { } } -func BenchmarkValidate(b *testing.B) { - expressionStr := "t1 > 10 && t2 > 3 ? ERROR : OK" - expr := &expression.TriggerExpression{ - Expression: &expressionStr, - TriggerType: moira.ExpressionTrigger, - MainTargetValue: 11.0, - AdditionalTargetsValues: map[string]float64{"t2": 4.0}} - for i := 0; i < b.N; i++ { - err := expr.Validate() - if err != nil { - b.Log(err) - } - } -} - func BenchmarkEvaluateComplex(b *testing.B) { expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" expr := &expression.TriggerExpression{ @@ -95,28 +80,3 @@ func BenchmarkEvaluateComplex(b *testing.B) { } } } - -func BenchmarkValidateComplex(b *testing.B) { - expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" - expr := &expression.TriggerExpression{ - Expression: &expressionStr, - TriggerType: moira.ExpressionTrigger, - MainTargetValue: 4, - AdditionalTargetsValues: map[string]float64{ - "t2": 5, - "t3": 3, - "t4": 6, - "t5": 18, - "t6": 10, - "t7": 15, - "t8": 20, - "t9": 10, - }, - } - for i := 0; i < b.N; i++ { - err := expr.Validate() - if err != nil { - b.Log(err) - } - } -} From 29b0d86bfd335585aa39b729dad6ff8ea765ec6c Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Tue, 8 Aug 2023 20:49:08 +0500 Subject: [PATCH 5/9] removed redundant call to expr.Run This is necessary because govalute is used to verify expression evaluation and `expr.Compile` is enough to validate expression. --- expression/expression.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/expression/expression.go b/expression/expression.go index 066e6fa40..ea4731654 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -143,17 +143,8 @@ func validateUserExpression( if err != nil { return nil, err } - result, err := expr.Run(program, env) - if err != nil { - return nil, err - } - switch result.(type) { - case moira.State: - exprCache.Set(cacheKey, program, cache.NoExpiration) - return userExpression, nil - default: - return nil, ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} - } + exprCache.Set(cacheKey, program, cache.NoExpiration) + return userExpression, nil } func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { From c4807b29fbf3553d3d49a237a0c2d164008a8e84 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Wed, 9 Aug 2023 14:06:21 +0500 Subject: [PATCH 6/9] added nil check to user expression validation --- expression/expression.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/expression/expression.go b/expression/expression.go index ea4731654..bc1f0b530 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -103,6 +103,9 @@ func validateUserExpression( triggerExpression *TriggerExpression, userExpression *govaluate.EvaluableExpression, ) (*govaluate.EvaluableExpression, error) { + if triggerExpression.Expression == nil { + return nil, fmt.Errorf("trigger_type set to expression, but no expression provided") + } expression := *triggerExpression.Expression cacheKey := fmt.Sprintf("[VALIDATED]%s", expression) From 65e4395367cb5d6dff7d43442f016540c1c4a868 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Fri, 11 Aug 2023 00:13:35 +0500 Subject: [PATCH 7/9] separated validate from evaluate for speed includes benchmarks --- api/dto/triggers.go | 2 +- expression/expression.go | 90 +++++++++++-------- expression/expression_test.go | 59 ++++++++++-- .../expression/expression_test.go | 25 ++++++ 4 files changed, 131 insertions(+), 45 deletions(-) diff --git a/api/dto/triggers.go b/api/dto/triggers.go index f9bc797c4..128b83bd4 100644 --- a/api/dto/triggers.go +++ b/api/dto/triggers.go @@ -195,7 +195,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { middleware.SetTimeSeriesNames(request, metricsDataNames) - if _, err = triggerExpression.Evaluate(); err != nil { + if err = triggerExpression.Validate(); err != nil { return err } diff --git a/expression/expression.go b/expression/expression.go index bc1f0b530..23ed168f7 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -2,10 +2,10 @@ package expression import ( "fmt" + "sort" "strings" "github.com/antonmedv/expr" - "github.com/antonmedv/expr/vm" "github.com/patrickmn/go-cache" "github.com/Knetic/govaluate" @@ -31,6 +31,10 @@ func (err ErrInvalidExpression) Error() string { return err.internalError.Error() } +type verificationResult struct { + internalError error +} + // TriggerExpression represents trigger expression handler parameters, what can be used for trigger expression handling type TriggerExpression struct { Expression *string @@ -80,40 +84,21 @@ func (triggerExpression TriggerExpression) Get(name string) (interface{}, error) } } -// Evaluate gets trigger expression and evaluates it for given parameters using govaluate -func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { - expr, err := getExpression(triggerExpression) - if err != nil { - return "", ErrInvalidExpression{internalError: err} +// Validate returns error if triggers of type moira.ExpressionTrigger are badly formatted otherwise nil +func (triggerExpression *TriggerExpression) Validate() error { + if triggerExpression.TriggerType != moira.ExpressionTrigger { + return fmt.Errorf("only advanced triggers should be validated") } - result, err := expr.Eval(triggerExpression) - if err != nil { - return "", ErrInvalidExpression{internalError: err} - } - switch res := result.(type) { - case moira.State: - return res, nil - default: - return "", ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} - } -} - -// validateUserExpression uses expr package to validate trigger expressions -func validateUserExpression( - triggerExpression *TriggerExpression, - userExpression *govaluate.EvaluableExpression, -) (*govaluate.EvaluableExpression, error) { if triggerExpression.Expression == nil { - return nil, fmt.Errorf("trigger_type set to expression, but no expression provided") + return fmt.Errorf("trigger_type set to expression, but no expression provided") } expression := *triggerExpression.Expression - - cacheKey := fmt.Sprintf("[VALIDATED]%s", expression) - pr, validated := exprCache.Get(cacheKey) + cacheKey := triggerExpression.validationCacheKey() + result, validated := exprCache.Get(cacheKey) if validated { - _, ok := pr.(*vm.Program) + err, ok := result.(verificationResult) if ok { - return userExpression, nil + return err.internalError } else { validated = false exprCache.Delete(cacheKey) @@ -138,16 +123,31 @@ func validateUserExpression( for k, v := range triggerExpression.AdditionalTargetsValues { env[k] = v } - program, err := expr.Compile( + _, err := expr.Compile( strings.ToLower(expression), expr.Optimize(true), expr.Env(env), ) + exprCache.Set(cacheKey, verificationResult{err}, cache.NoExpiration) + return err +} + +// Evaluate gets trigger expression and evaluates it for given parameters using govaluate +func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { + expr, err := getExpression(triggerExpression) if err != nil { - return nil, err + return "", ErrInvalidExpression{internalError: err} + } + result, err := expr.Eval(triggerExpression) + if err != nil { + return "", ErrInvalidExpression{internalError: err} + } + switch res := result.(type) { + case moira.State: + return res, nil + default: + return "", ErrInvalidExpression{internalError: fmt.Errorf("expression result must be state value")} } - exprCache.Set(cacheKey, program, cache.NoExpiration) - return userExpression, nil } func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { @@ -155,16 +155,28 @@ func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableEx if triggerExpression.Expression == nil || *triggerExpression.Expression == "" { return nil, fmt.Errorf("trigger_type set to expression, but no expression provided") } - - userExpression, err := getUserExpression(*triggerExpression.Expression) - if err != nil { - return nil, err - } - return validateUserExpression(triggerExpression, userExpression) + return getUserExpression(*triggerExpression.Expression) } return getSimpleExpression(triggerExpression) } +// validationCacheKey returns the key used to store TriggerExpression in the cache. +// +// Two TriggerExpressions, have the same validationCacheKey if they have the same expression +// and the same list of targets provided. +func (triggerExpression *TriggerExpression) validationCacheKey() string { + var expression string + if triggerExpression.Expression != nil { + expression = *triggerExpression.Expression + } + targets := []string{"t1"} + for k := range triggerExpression.AdditionalTargetsValues { + targets = append(targets, k) + } + sort.Strings(targets) + return fmt.Sprintf("[VALIDATED]%s;targets=%s", expression, strings.Join(targets, ",")) +} + func getSimpleExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { if triggerExpression.ErrorValue == nil && triggerExpression.WarnValue == nil { return nil, fmt.Errorf("error value and warning value can not be empty") diff --git a/expression/expression_test.go b/expression/expression_test.go index 41ef59952..4fc02ab9a 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -87,20 +87,69 @@ func TestExpression(t *testing.T) { result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA}).Evaluate() So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateNODATA) + }) +} - expression = "t1 > 10 && t2 > 3 ? OK : ddd" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() +func TestValidate(t *testing.T) { + Convey("Test valid expressions", t, func() { + expression := "t1 > 10 && t2 > 3 ? OK : ERROR" + err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldBeNil) + + expression = "t1 <= 0 ? PREV_STATE : (t1 >= 20 ? ERROR : (t1 >= 10 ? WARN : OK))" + err = (&TriggerExpression{PreviousState: moira.StateNODATA, Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldBeNil) + }) + Convey("Test bad expressions", t, func() { + expression := "" + err := (&TriggerExpression{Expression: &expression, TriggerType: moira.FallingTrigger}).Validate() + So(err, ShouldResemble, fmt.Errorf("only advanced triggers should be validated")) + err = (&TriggerExpression{Expression: nil, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldResemble, fmt.Errorf("trigger_type set to expression, but no expression provided")) + }) + Convey("Test invalid expressions", t, func() { + expression := "t1 > 10 && t2 > 3 ? OK : ddd" + err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldNotBeNil) So(err.Error(), ShouldResemble, `unknown name ddd (1:26) | t1 > 10 && t2 > 3 ? ok : ddd | .........................^`) - So(result, ShouldBeEmpty) expression = "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Evaluate() + err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldNotBeNil) So(err.Error(), ShouldResemble, `unknown name t2 (1:17) | t1 > 10 ? ok : (t2 < 5 ? warn : error) | ................^`) - So(result, ShouldBeEmpty) + }) + + Convey("Test validation caching", t, func() { + expression := "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" + trigger := TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger} + validateErr := trigger.Validate() + So(validateErr, ShouldNotBeNil) + // check cache + cacheKey := trigger.validationCacheKey() + cachedError, ok := exprCache.Get(cacheKey) + So(ok, ShouldBeTrue) + cachedErr, ok := cachedError.(verificationResult) + So(ok, ShouldBeTrue) + So(cachedErr, ShouldNotBeNil) + SoMsg("when error is not nil", cachedErr.internalError, ShouldResemble, validateErr) + + trigger2 := TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 5}, TriggerType: moira.ExpressionTrigger} + validateErr = trigger2.Validate() + So(validateErr, ShouldBeNil) + // check cache + cacheKey2 := trigger2.validationCacheKey() + cachedError, ok = exprCache.Get(cacheKey2) + So(ok, ShouldBeTrue) + cachedErr, ok = cachedError.(verificationResult) + So(ok, ShouldBeTrue) + SoMsg("when error is nil", cachedErr.internalError, ShouldResemble, validateErr) + + // cache key validations + SoMsg("same expression, different targets", cacheKey, ShouldNotEqual, cacheKey2) }) } diff --git a/perfomance_tests/expression/expression_test.go b/perfomance_tests/expression/expression_test.go index 4b0b84d52..92977e5b1 100644 --- a/perfomance_tests/expression/expression_test.go +++ b/perfomance_tests/expression/expression_test.go @@ -56,6 +56,31 @@ func BenchmarkCustomExpr(b *testing.B) { } } +func BenchmarkValidateComplex(b *testing.B) { + expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 4, + AdditionalTargetsValues: map[string]float64{ + "t2": 5, + "t3": 3, + "t4": 6, + "t5": 18, + "t6": 10, + "t7": 15, + "t8": 20, + "t9": 10, + }, + } + for i := 0; i < b.N; i++ { + err := expr.Validate() + if err != nil { + b.Log(err) + } + } +} + func BenchmarkEvaluateComplex(b *testing.B) { expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" expr := &expression.TriggerExpression{ From c302f68a4293f48b039f59b8a0eb63f8ff6269c9 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Fri, 11 Aug 2023 14:17:29 +0500 Subject: [PATCH 8/9] removed expression validation caching --- expression/expression.go | 39 ++--------------------------------- expression/expression_test.go | 38 +++++----------------------------- 2 files changed, 7 insertions(+), 70 deletions(-) diff --git a/expression/expression.go b/expression/expression.go index 23ed168f7..f8830900d 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -2,7 +2,6 @@ package expression import ( "fmt" - "sort" "strings" "github.com/antonmedv/expr" @@ -31,10 +30,6 @@ func (err ErrInvalidExpression) Error() string { return err.internalError.Error() } -type verificationResult struct { - internalError error -} - // TriggerExpression represents trigger expression handler parameters, what can be used for trigger expression handling type TriggerExpression struct { Expression *string @@ -87,24 +82,12 @@ func (triggerExpression TriggerExpression) Get(name string) (interface{}, error) // Validate returns error if triggers of type moira.ExpressionTrigger are badly formatted otherwise nil func (triggerExpression *TriggerExpression) Validate() error { if triggerExpression.TriggerType != moira.ExpressionTrigger { - return fmt.Errorf("only advanced triggers should be validated") + return nil } - if triggerExpression.Expression == nil { + if triggerExpression.Expression == nil || *triggerExpression.Expression == "" { return fmt.Errorf("trigger_type set to expression, but no expression provided") } expression := *triggerExpression.Expression - cacheKey := triggerExpression.validationCacheKey() - result, validated := exprCache.Get(cacheKey) - if validated { - err, ok := result.(verificationResult) - if ok { - return err.internalError - } else { - validated = false - exprCache.Delete(cacheKey) - } - } - env := map[string]interface{}{ "ok": moira.StateOK, "error": moira.StateERROR, @@ -128,7 +111,6 @@ func (triggerExpression *TriggerExpression) Validate() error { expr.Optimize(true), expr.Env(env), ) - exprCache.Set(cacheKey, verificationResult{err}, cache.NoExpiration) return err } @@ -160,23 +142,6 @@ func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableEx return getSimpleExpression(triggerExpression) } -// validationCacheKey returns the key used to store TriggerExpression in the cache. -// -// Two TriggerExpressions, have the same validationCacheKey if they have the same expression -// and the same list of targets provided. -func (triggerExpression *TriggerExpression) validationCacheKey() string { - var expression string - if triggerExpression.Expression != nil { - expression = *triggerExpression.Expression - } - targets := []string{"t1"} - for k := range triggerExpression.AdditionalTargetsValues { - targets = append(targets, k) - } - sort.Strings(targets) - return fmt.Sprintf("[VALIDATED]%s;targets=%s", expression, strings.Join(targets, ",")) -} - func getSimpleExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { if triggerExpression.ErrorValue == nil && triggerExpression.WarnValue == nil { return nil, fmt.Errorf("error value and warning value can not be empty") diff --git a/expression/expression_test.go b/expression/expression_test.go index 4fc02ab9a..857d546fa 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -99,12 +99,13 @@ func TestValidate(t *testing.T) { expression = "t1 <= 0 ? PREV_STATE : (t1 >= 20 ? ERROR : (t1 >= 10 ? WARN : OK))" err = (&TriggerExpression{PreviousState: moira.StateNODATA, Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Validate() So(err, ShouldBeNil) + + warnValue, errorValue := 60.0, 90.0 + err = (&TriggerExpression{PreviousState: moira.StateNODATA, Expression: nil, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger, MainTargetValue: 5}).Validate() + SoMsg("validating simple expression", err, ShouldBeNil) }) Convey("Test bad expressions", t, func() { - expression := "" - err := (&TriggerExpression{Expression: &expression, TriggerType: moira.FallingTrigger}).Validate() - So(err, ShouldResemble, fmt.Errorf("only advanced triggers should be validated")) - err = (&TriggerExpression{Expression: nil, TriggerType: moira.ExpressionTrigger}).Validate() + err := (&TriggerExpression{Expression: nil, TriggerType: moira.ExpressionTrigger}).Validate() So(err, ShouldResemble, fmt.Errorf("trigger_type set to expression, but no expression provided")) }) Convey("Test invalid expressions", t, func() { @@ -122,35 +123,6 @@ func TestValidate(t *testing.T) { | t1 > 10 ? ok : (t2 < 5 ? warn : error) | ................^`) }) - - Convey("Test validation caching", t, func() { - expression := "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" - trigger := TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger} - validateErr := trigger.Validate() - So(validateErr, ShouldNotBeNil) - // check cache - cacheKey := trigger.validationCacheKey() - cachedError, ok := exprCache.Get(cacheKey) - So(ok, ShouldBeTrue) - cachedErr, ok := cachedError.(verificationResult) - So(ok, ShouldBeTrue) - So(cachedErr, ShouldNotBeNil) - SoMsg("when error is not nil", cachedErr.internalError, ShouldResemble, validateErr) - - trigger2 := TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 5}, TriggerType: moira.ExpressionTrigger} - validateErr = trigger2.Validate() - So(validateErr, ShouldBeNil) - // check cache - cacheKey2 := trigger2.validationCacheKey() - cachedError, ok = exprCache.Get(cacheKey2) - So(ok, ShouldBeTrue) - cachedErr, ok = cachedError.(verificationResult) - So(ok, ShouldBeTrue) - SoMsg("when error is nil", cachedErr.internalError, ShouldResemble, validateErr) - - // cache key validations - SoMsg("same expression, different targets", cacheKey, ShouldNotEqual, cacheKey2) - }) } func TestGetExpressionValue(t *testing.T) { From 5245495ae50a3ac8c1f2b23030c193aceef5d1c4 Mon Sep 17 00:00:00 2001 From: Evans Owamoyo Date: Mon, 14 Aug 2023 12:04:54 +0500 Subject: [PATCH 9/9] wrapped validation errors with `ErrInvalidExpression` --- expression/expression.go | 12 ++++++++---- expression/expression_test.go | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/expression/expression.go b/expression/expression.go index f8830900d..6d7fdfec8 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -85,7 +85,9 @@ func (triggerExpression *TriggerExpression) Validate() error { return nil } if triggerExpression.Expression == nil || *triggerExpression.Expression == "" { - return fmt.Errorf("trigger_type set to expression, but no expression provided") + return ErrInvalidExpression{ + internalError: fmt.Errorf("trigger_type set to expression, but no expression provided"), + } } expression := *triggerExpression.Expression env := map[string]interface{}{ @@ -106,12 +108,14 @@ func (triggerExpression *TriggerExpression) Validate() error { for k, v := range triggerExpression.AdditionalTargetsValues { env[k] = v } - _, err := expr.Compile( + if _, err := expr.Compile( strings.ToLower(expression), expr.Optimize(true), expr.Env(env), - ) - return err + ); err != nil { + return ErrInvalidExpression{err} + } + return nil } // Evaluate gets trigger expression and evaluates it for given parameters using govaluate diff --git a/expression/expression_test.go b/expression/expression_test.go index 857d546fa..498768510 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -106,7 +106,7 @@ func TestValidate(t *testing.T) { }) Convey("Test bad expressions", t, func() { err := (&TriggerExpression{Expression: nil, TriggerType: moira.ExpressionTrigger}).Validate() - So(err, ShouldResemble, fmt.Errorf("trigger_type set to expression, but no expression provided")) + So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("trigger_type set to expression, but no expression provided")}) }) Convey("Test invalid expressions", t, func() { expression := "t1 > 10 && t2 > 3 ? OK : ddd"