Merge pull request #14813 from bergquist/zero_frequency

Make sure that frequency is never set to zero

pkg/services/alerting/extractor.go

@@ -112,7 +112,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json,
 
 		frequency, err := getTimeDurationStringToSeconds(jsonAlert.Get("frequency").MustString())
 		if err != nil {
-			return nil, ValidationError{Reason: "Could not parse frequency"}
+			return nil, ValidationError{Reason: err.Error()}
 		}
 
 		rawFor := jsonAlert.Get("for").MustString()

pkg/services/alerting/rule.go

@@ -1,16 +1,21 @@
 package alerting
 
 import (
+	"errors"
 	"fmt"
 	"regexp"
 	"strconv"
 	"time"
 
 	"github.com/grafana/grafana/pkg/components/simplejson"
-
 	m "github.com/grafana/grafana/pkg/models"
 )
 
+var (
+	ErrFrequencyCannotBeZeroOrLess = errors.New(`"evaluate every" cannot be zero or below`)
+	ErrFrequencyCouldNotBeParsed   = errors.New(`"evaluate every" field could not be parsed`)
+)
+
 type Rule struct {
 	Id                  int64
 	OrgId               int64
@@ -76,7 +81,7 @@ func getTimeDurationStringToSeconds(str string) (int64, error) {
 	matches := ValueFormatRegex.FindAllString(str, 1)
 
 	if len(matches) <= 0 {
-		return 0, fmt.Errorf("Frequency could not be parsed")
+		return 0, ErrFrequencyCouldNotBeParsed
 	}
 
 	value, err := strconv.Atoi(matches[0])
@@ -84,6 +89,10 @@ func getTimeDurationStringToSeconds(str string) (int64, error) {
 		return 0, err
 	}
 
+	if value == 0 {
+		return 0, ErrFrequencyCannotBeZeroOrLess
+	}
+
 	unit := UnitFormatRegex.FindAllString(str, 1)[0]
 
 	if val, ok := unitMultiplier[unit]; ok {
@@ -101,7 +110,6 @@ func NewRuleFromDBAlert(ruleDef *m.Alert) (*Rule, error) {
 	model.PanelId = ruleDef.PanelId
 	model.Name = ruleDef.Name
 	model.Message = ruleDef.Message
-	model.Frequency = ruleDef.Frequency
 	model.State = ruleDef.State
 	model.LastStateChange = ruleDef.NewStateDate
 	model.For = ruleDef.For
@@ -109,6 +117,13 @@ func NewRuleFromDBAlert(ruleDef *m.Alert) (*Rule, error) {
 	model.ExecutionErrorState = m.ExecutionErrorOption(ruleDef.Settings.Get("executionErrorState").MustString("alerting"))
 	model.StateChanges = ruleDef.StateChanges
 
+	model.Frequency = ruleDef.Frequency
+	// frequency cannot be zero since that would not execute the alert rule.
+	// so we fallback to 60 seconds if `Freqency` is missing
+	if model.Frequency == 0 {
+		model.Frequency = 60
+	}
+
 	for _, v := range ruleDef.Settings.Get("notifications").MustArray() {
 		jsonModel := simplejson.NewFromAny(v)
 		id, err := jsonModel.Get("id").Int64()

pkg/services/alerting/rule_test.go

@@ -14,6 +14,36 @@ func (f *FakeCondition) Eval(context *EvalContext) (*ConditionResult, error) {
 	return &ConditionResult{}, nil
 }
 
+func TestAlertRuleFrequencyParsing(t *testing.T) {
+	tcs := []struct {
+		input  string
+		err    error
+		result int64
+	}{
+		{input: "10s", result: 10},
+		{input: "10m", result: 600},
+		{input: "1h", result: 3600},
+		{input: "1o", result: 1},
+		{input: "0s", err: ErrFrequencyCannotBeZeroOrLess},
+		{input: "0m", err: ErrFrequencyCannotBeZeroOrLess},
+		{input: "0h", err: ErrFrequencyCannotBeZeroOrLess},
+		{input: "0", err: ErrFrequencyCannotBeZeroOrLess},
+		{input: "-1s", err: ErrFrequencyCouldNotBeParsed},
+	}
+
+	for _, tc := range tcs {
+		r, err := getTimeDurationStringToSeconds(tc.input)
+		if err != tc.err {
+			t.Errorf("expected error: '%v' got: '%v'", tc.err, err)
+			return
+		}
+
+		if r != tc.result {
+			t.Errorf("expected result: %d got %d", tc.result, r)
+		}
+	}
+}
+
 func TestAlertRuleModel(t *testing.T) {
 	Convey("Testing alert rule", t, func() {
 
@@ -21,26 +51,6 @@ func TestAlertRuleModel(t *testing.T) {
 			return &FakeCondition{}, nil
 		})
 
-		Convey("Can parse seconds", func() {
-			seconds, _ := getTimeDurationStringToSeconds("10s")
-			So(seconds, ShouldEqual, 10)
-		})
-
-		Convey("Can parse minutes", func() {
-			seconds, _ := getTimeDurationStringToSeconds("10m")
-			So(seconds, ShouldEqual, 600)
-		})
-
-		Convey("Can parse hours", func() {
-			seconds, _ := getTimeDurationStringToSeconds("1h")
-			So(seconds, ShouldEqual, 3600)
-		})
-
-		Convey("defaults to seconds", func() {
-			seconds, _ := getTimeDurationStringToSeconds("1o")
-			So(seconds, ShouldEqual, 1)
-		})
-
 		Convey("should return err for empty string", func() {
 			_, err := getTimeDurationStringToSeconds("")
 			So(err, ShouldNotBeNil)
@@ -89,5 +99,35 @@ func TestAlertRuleModel(t *testing.T) {
 				So(len(alertRule.Notifications), ShouldEqual, 2)
 			})
 		})
+
+		Convey("can construct alert rule model with invalid frequency", func() {
+			json := `
+			{
+				"name": "name2",
+				"description": "desc2",
+				"noDataMode": "critical",
+				"enabled": true,
+				"frequency": "0s",
+        		"conditions": [ { "type": "test", "prop": 123 } ],
+        		"notifications": []
+			}`
+
+			alertJSON, jsonErr := simplejson.NewJson([]byte(json))
+			So(jsonErr, ShouldBeNil)
+
+			alert := &m.Alert{
+				Id:          1,
+				OrgId:       1,
+				DashboardId: 1,
+				PanelId:     1,
+				Frequency:   0,
+
+				Settings: alertJSON,
+			}
+
+			alertRule, err := NewRuleFromDBAlert(alert)
+			So(err, ShouldBeNil)
+			So(alertRule.Frequency, ShouldEqual, 60)
+		})
 	})
 }