From 50bffda2f2d152d6e8dd98b37fc0160a7f1c1358 Mon Sep 17 00:00:00 2001 From: "docmerlin (j. Emrys Landivar)" Date: Tue, 19 May 2020 11:37:41 -0500 Subject: [PATCH] fix: error instead of panic on int div-by-zero --- eval.go | 5 +- integrations/streamer_test.go | 21 +- .../TestStream_EvalDivisionByZero.srpl | 3 + tick/stateful/expr.go | 33 +- tick/stateful/expr_dynamic_test.go | 323 ++++++++++-------- 5 files changed, 227 insertions(+), 158 deletions(-) create mode 100644 integrations/testdata/TestStream_EvalDivisionByZero.srpl diff --git a/eval.go b/eval.go index 3b68632b3..72044e9de 100644 --- a/eval.go +++ b/eval.go @@ -91,8 +91,7 @@ func (n *EvalNode) newGroup() *evalGroup { } } -func (n *EvalNode) eval(expressions []stateful.Expression, p edge.FieldsTagsTimeSetter) error { - +func (n *EvalNode) eval(expressions []stateful.Expression, p edge.FieldsTagsTimeSetter) (err error) { vars := n.scopePool.Get() defer n.scopePool.Put(vars) @@ -172,7 +171,7 @@ func (n *EvalNode) eval(expressions []stateful.Expression, p edge.FieldsTagsTime } p.SetFields(newFields) p.SetTags(newTags) - return nil + return } type evalGroup struct { diff --git a/integrations/streamer_test.go b/integrations/streamer_test.go index f97967d79..c6560927e 100644 --- a/integrations/streamer_test.go +++ b/integrations/streamer_test.go @@ -6,8 +6,6 @@ import ( "encoding/json" "flag" "fmt" - "github.com/influxdata/kapacitor/services/discord" - "github.com/influxdata/kapacitor/services/discord/discordtest" "html" "io/ioutil" "math/rand" @@ -23,6 +21,9 @@ import ( "text/template" "time" + "github.com/influxdata/kapacitor/services/discord" + "github.com/influxdata/kapacitor/services/discord/discordtest" + "github.com/davecgh/go-spew/spew" "github.com/docker/docker/api/types/swarm" "github.com/google/go-cmp/cmp" @@ -2691,6 +2692,20 @@ stream testStreamerWithOutput(t, "TestStream_EvalAllTypes", script, 2*time.Second, er, false, nil) } +func TestStream_EvalDivisionByZero(t *testing.T) { + var script = ` +stream + |from() + .measurement('data') + |eval(lambda: 10/"n") + .as('n') + |httpOut('TestStream_EvalDivisionByZero') +` + + testStreamerNoOutput(t, "TestStream_EvalDivisionByZero", script, 2*time.Second, nil) + +} + func TestStream_Eval_KeepAll(t *testing.T) { var script = ` stream @@ -12868,6 +12883,7 @@ func testStreamer( <-chan error, *kapacitor.TaskMaster, ) { + t.Helper() if testing.Verbose() { wlog.SetLevel(wlog.DEBUG) } else { @@ -13027,6 +13043,7 @@ func testStreamerWithOutput( ignoreOrder bool, tmInit func(tm *kapacitor.TaskMaster), ) { + t.Helper() clock, et, replayErr, tm := testStreamer(t, name, script, tmInit) defer tm.Close() diff --git a/integrations/testdata/TestStream_EvalDivisionByZero.srpl b/integrations/testdata/TestStream_EvalDivisionByZero.srpl new file mode 100644 index 000000000..ba0329264 --- /dev/null +++ b/integrations/testdata/TestStream_EvalDivisionByZero.srpl @@ -0,0 +1,3 @@ +dbname +rpname +data,t=t1 n=0i 0000003600 diff --git a/tick/stateful/expr.go b/tick/stateful/expr.go index 80374009c..8883317d0 100644 --- a/tick/stateful/expr.go +++ b/tick/stateful/expr.go @@ -1,6 +1,7 @@ package stateful import ( + "errors" "fmt" "time" @@ -88,50 +89,64 @@ func (se *expression) EvalMissing(scope *Scope) (*ast.Missing, error) { return se.nodeEvaluator.EvalMissing(scope, se.executionState) } -func (se *expression) Eval(scope *Scope) (interface{}, error) { - typ, err := se.nodeEvaluator.Type(scope) +func (se *expression) Eval(scope *Scope) (result interface{}, err error) { + defer func() { + if r := recover(); r != nil { + switch r := r.(type) { + case string: + err = errors.New(r) + case error: + err = r + case fmt.Stringer: + err = errors.New(r.String()) + } + } + }() + var typ ast.ValueType + typ, err = se.nodeEvaluator.Type(scope) if err != nil { return nil, err } switch typ { case ast.TInt: - result, err := se.EvalInt(scope) + result, err = se.EvalInt(scope) if err != nil { return nil, err } return result, err case ast.TFloat: - result, err := se.EvalFloat(scope) + result, err = se.EvalFloat(scope) if err != nil { return nil, err } return result, err case ast.TString: - result, err := se.EvalString(scope) + result, err = se.EvalString(scope) if err != nil { return nil, err } return result, err case ast.TBool: - result, err := se.EvalBool(scope) + result, err = se.EvalBool(scope) if err != nil { return nil, err } return result, err case ast.TDuration: - result, err := se.EvalDuration(scope) + result, err = se.EvalDuration(scope) if err != nil { return nil, err } return result, err case ast.TMissing: - result, err := se.EvalMissing(scope) + result, err = se.EvalMissing(scope) if err != nil { return nil, err } return result, err default: - return nil, fmt.Errorf("expression returned unexpected type %s", typ) + err = fmt.Errorf("expression returned unexpected type %s", typ) + return } } diff --git a/tick/stateful/expr_dynamic_test.go b/tick/stateful/expr_dynamic_test.go index a979064e3..30bb3986b 100644 --- a/tick/stateful/expr_dynamic_test.go +++ b/tick/stateful/expr_dynamic_test.go @@ -30,6 +30,7 @@ type testCase struct { // runDyanmicTestCase is when we want to change the "dyanmism" of // a node - type change or value change func runDynamicTestCase(t *testing.T, tc testCase) { + t.Helper() se := mustCompileExpression(tc.Node) for i, expectation := range tc.Expectations { @@ -50,8 +51,11 @@ func runDynamicTestCase(t *testing.T, tc testCase) { result, err = se.Eval(scope) } - if err != nil { - if expectation.ExpectedError == nil { + if err != nil || expectation.ExpectedError != nil { + if err == nil { + t.Errorf("%s, %s, Iteration: %v: Expected %v error, but got nil", tc.Title, evaluationType, (i + 1), expectation.ExpectedError) + + } else if expectation.ExpectedError == nil { t.Errorf("%s: %s: Iteration %v: Got unexpected error while expecting for result:\n %v\n", tc.Title, evaluationType, (i + 1), err) continue } else if err.Error() != expectation.ExpectedError.Error() { @@ -67,94 +71,56 @@ func runDynamicTestCase(t *testing.T, tc testCase) { } func TestExpression_BinaryNode_DynamicTestCases(t *testing.T) { + t.Run("BinaryNode - EvalBool supports numeric type changes", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), - runDynamicTestCase(t, testCase{ - Title: "BinaryNode - EvalBool supports numeric type changes", - - Node: &ast.BinaryNode{ - Operator: ast.TokenGreater, - Left: &ast.ReferenceNode{ - Reference: "value", + Node: &ast.BinaryNode{ + Operator: ast.TokenGreater, + Left: &ast.ReferenceNode{ + Reference: "value", + }, + Right: &ast.NumberNode{ + IsFloat: true, + Float64: float64(10), + }, }, - Right: &ast.NumberNode{ - IsFloat: true, - Float64: float64(10), + + Expectations: []valueExpectation{ + {IsEvalBool: true, Value: float64(20), ExpectedError: nil, ExpectedResult: true}, + {IsEvalBool: true, Value: int64(5), ExpectedError: nil, ExpectedResult: false}, }, - }, + }) - Expectations: []valueExpectation{ - {IsEvalBool: true, Value: float64(20), ExpectedError: nil, ExpectedResult: true}, - {IsEvalBool: true, Value: int64(5), ExpectedError: nil, ExpectedResult: false}, - }, }) - - runDynamicTestCase(t, testCase{ - Title: "BinaryNode - EvalNum supports numeric type changes", - - Node: &ast.BinaryNode{ - Operator: ast.TokenPlus, - Left: &ast.ReferenceNode{ - Reference: "value", - }, - Right: &ast.NumberNode{ - IsFloat: true, - Float64: float64(10), - }, - }, - - Expectations: []valueExpectation{ - { - IsEvalNum: true, - Value: float64(20), - ExpectedError: nil, - ExpectedResult: float64(30), - }, - { - IsEvalNum: true, - Value: int64(5), - ExpectedError: errors.New("mismatched type to binary operator. got int + float. see bool(), int(), float(), string(), duration()"), - ExpectedResult: nil, + t.Run("BinaryNode - Eval errors on integer division by zero", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), + + Node: &ast.BinaryNode{ + Operator: ast.TokenDiv, + Left: &ast.NumberNode{ + IsInt: true, + Int64: 10, + }, + Right: &ast.NumberNode{ + IsInt: true, + Int64: 0, + }, }, - }, - }) - - runDynamicTestCase(t, testCase{ - Title: "BinaryNode - EvalNum supports numeric value changes", - Node: &ast.BinaryNode{ - Operator: ast.TokenGreater, - Left: &ast.ReferenceNode{ - Reference: "value", - }, - Right: &ast.NumberNode{ - IsFloat: true, - Float64: float64(10), + Expectations: []valueExpectation{ + {ExpectedError: errors.New("runtime error: integer divide by zero"), IsEvalNum: true}, }, - }, - - Expectations: []valueExpectation{ - { - IsEvalBool: true, - Value: float64(20), - ExpectedError: nil, - ExpectedResult: true, - }, - { - IsEvalBool: true, - Value: int64(5), - ExpectedError: nil, - ExpectedResult: false, - }, - }, + }) }) - runDynamicTestCase(t, testCase{ - Title: "BinaryNode - Nested BinaryNodes can determine correct type", + t.Run("BinaryNode - EvalNum supports numeric type changes", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), - Node: &ast.BinaryNode{ - Operator: ast.TokenAnd, - Left: &ast.BinaryNode{ - Operator: ast.TokenLess, + Node: &ast.BinaryNode{ + Operator: ast.TokenPlus, Left: &ast.ReferenceNode{ Reference: "value", }, @@ -163,92 +129,161 @@ func TestExpression_BinaryNode_DynamicTestCases(t *testing.T) { Float64: float64(10), }, }, - Right: &ast.BinaryNode{ + + Expectations: []valueExpectation{ + { + IsEvalNum: true, + Value: float64(20), + ExpectedError: nil, + ExpectedResult: float64(30), + }, + { + IsEvalNum: true, + Value: int64(5), + ExpectedError: errors.New("mismatched type to binary operator. got int + float. see bool(), int(), float(), string(), duration()"), + ExpectedResult: nil, + }, + }, + }) + }) + + t.Run("BinaryNode - EvalNum supports numeric type changes", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), + + Node: &ast.BinaryNode{ Operator: ast.TokenGreater, Left: &ast.ReferenceNode{ Reference: "value", }, Right: &ast.NumberNode{ IsFloat: true, - Float64: float64(7), + Float64: float64(10), }, }, - }, - - Expectations: []valueExpectation{ - { - IsEvalBool: true, - Value: float64(8), - ExpectedError: nil, - ExpectedResult: true, + + Expectations: []valueExpectation{ + { + IsEvalBool: true, + Value: float64(20), + ExpectedError: nil, + ExpectedResult: true, + }, + { + IsEvalBool: true, + Value: int64(5), + ExpectedError: nil, + ExpectedResult: false, + }, }, - { - IsEvalBool: true, - Value: int64(5), - ExpectedError: nil, - ExpectedResult: false, + }) + }) + + t.Run("BinaryNode - Nested BinaryNodes can determine correct type", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), + Node: &ast.BinaryNode{ + Operator: ast.TokenAnd, + Left: &ast.BinaryNode{ + Operator: ast.TokenLess, + Left: &ast.ReferenceNode{ + Reference: "value", + }, + Right: &ast.NumberNode{ + IsFloat: true, + Float64: float64(10), + }, + }, + Right: &ast.BinaryNode{ + Operator: ast.TokenGreater, + Left: &ast.ReferenceNode{ + Reference: "value", + }, + Right: &ast.NumberNode{ + IsFloat: true, + Float64: float64(7), + }, + }, }, - { - IsEvalBool: true, - Value: int64(11), - ExpectedError: nil, - ExpectedResult: false, + + Expectations: []valueExpectation{ + { + IsEvalBool: true, + Value: float64(8), + ExpectedError: nil, + ExpectedResult: true, + }, + { + IsEvalBool: true, + Value: int64(5), + ExpectedError: nil, + ExpectedResult: false, + }, + { + IsEvalBool: true, + Value: int64(11), + ExpectedError: nil, + ExpectedResult: false, + }, }, - }, - }) + }) + }) } - func TestExpression_UnaryNode_DyanmicTestCases(t *testing.T) { - runDynamicTestCase(t, testCase{ - Title: "UnaryNode - EvalNum supports numeric type changes", + t.Run("UnaryNode - EvalNum supports numeric type changes", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), - Node: &ast.UnaryNode{ - Operator: ast.TokenMinus, - Node: &ast.ReferenceNode{ - Reference: "value", - }, - }, - - Expectations: []valueExpectation{ - { - IsEvalNum: true, - Value: float64(20), - ExpectedError: nil, - ExpectedResult: float64(-20), + Node: &ast.UnaryNode{ + Operator: ast.TokenMinus, + Node: &ast.ReferenceNode{ + Reference: "value", + }, }, - { - IsEvalNum: true, - Value: int64(20), - ExpectedError: nil, - ExpectedResult: int64(-20), + + Expectations: []valueExpectation{ + { + IsEvalNum: true, + Value: float64(20), + ExpectedError: nil, + ExpectedResult: float64(-20), + }, + { + IsEvalNum: true, + Value: int64(20), + ExpectedError: nil, + ExpectedResult: int64(-20), + }, }, - }, + }) }) - runDynamicTestCase(t, testCase{ - Title: "UnaryNode - EvalBool supports boolean value changes", + t.Run("UnaryNode - EvalBool supports boolean value changes", func(t *testing.T) { + runDynamicTestCase(t, testCase{ + Title: t.Name(), - Node: &ast.UnaryNode{ - Operator: ast.TokenNot, - Node: &ast.ReferenceNode{ - Reference: "value", - }, - }, - - Expectations: []valueExpectation{ - { - IsEvalBool: true, - Value: bool(true), - ExpectedError: nil, - ExpectedResult: bool(false), + Node: &ast.UnaryNode{ + Operator: ast.TokenNot, + Node: &ast.ReferenceNode{ + Reference: "value", + }, }, - { - IsEvalBool: true, - Value: bool(false), - ExpectedError: nil, - ExpectedResult: bool(true), + + Expectations: []valueExpectation{ + { + IsEvalBool: true, + Value: true, + ExpectedError: nil, + ExpectedResult: false, + }, + { + IsEvalBool: true, + Value: false, + ExpectedError: nil, + ExpectedResult: true, + }, }, - }, + }) }) }