From 2de32756c257941703f22d0e1afbaa523dee55b1 Mon Sep 17 00:00:00 2001 From: Pavel Bakulev Date: Thu, 20 Dec 2018 17:12:47 +0200 Subject: [PATCH] Returned id for alert notifications which were created without uid --- pkg/services/alerting/extractor_test.go | 129 ++++++++++-------- pkg/services/alerting/rule.go | 33 ++++- pkg/services/alerting/rule_test.go | 127 ++++++++++++----- .../alerting/testdata/dash-without-id.json | 3 + .../alerting/testdata/influxdb-alert.json | 5 +- pkg/services/sqlstore/alert_notification.go | 1 + 6 files changed, 197 insertions(+), 101 deletions(-) diff --git a/pkg/services/alerting/extractor_test.go b/pkg/services/alerting/extractor_test.go index 9665a657bb7..66adf951269 100644 --- a/pkg/services/alerting/extractor_test.go +++ b/pkg/services/alerting/extractor_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" . "github.com/smartystreets/goconvey/convey" ) @@ -197,74 +198,86 @@ func TestAlertRuleExtraction(t *testing.T) { }) }) - Convey("Parse and validate dashboard containing influxdb alert", func() { - json, err := ioutil.ReadFile("./testdata/influxdb-alert.json") + Convey("Alert notifications are in DB", func() { + sqlstore.InitTestDB(t) + err := sqlstore.CreateOrg(&m.CreateOrgCommand{Name: "Main Org."}) + So(err, ShouldBeNil) + firstNotification := m.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} + err = sqlstore.CreateAlertNotificationCommand(&firstNotification) + So(err, ShouldBeNil) + secondNotification := m.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} + err = sqlstore.CreateAlertNotificationCommand(&secondNotification) So(err, ShouldBeNil) - dashJson, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - dash := m.NewDashboardFromJson(dashJson) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { + Convey("Parse and validate dashboard containing influxdb alert", func() { + json, err := ioutil.ReadFile("./testdata/influxdb-alert.json") So(err, ShouldBeNil) - }) - Convey("should be able to read interval", func() { - So(len(alerts), ShouldEqual, 1) - - for _, alert := range alerts { - So(alert.DashboardId, ShouldEqual, 4) - - conditions := alert.Settings.Get("conditions").MustArray() - cond := simplejson.NewFromAny(conditions[0]) - - So(cond.Get("query").Get("model").Get("interval").MustString(), ShouldEqual, ">10s") - } - }) - }) - - Convey("Should be able to extract collapsed panels", func() { - json, err := ioutil.ReadFile("./testdata/collapsed-panels.json") - So(err, ShouldBeNil) - - dashJson, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - - dash := m.NewDashboardFromJson(dashJson) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { + dashJson, err := simplejson.NewJson(json) So(err, ShouldBeNil) + dash := m.NewDashboardFromJson(dashJson) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + + Convey("Get rules without error", func() { + So(err, ShouldBeNil) + }) + + Convey("should be able to read interval", func() { + So(len(alerts), ShouldEqual, 1) + + for _, alert := range alerts { + So(alert.DashboardId, ShouldEqual, 4) + + conditions := alert.Settings.Get("conditions").MustArray() + cond := simplejson.NewFromAny(conditions[0]) + + So(cond.Get("query").Get("model").Get("interval").MustString(), ShouldEqual, ">10s") + } + }) }) - Convey("should be able to extract collapsed alerts", func() { - So(len(alerts), ShouldEqual, 4) - }) - }) - - Convey("Parse and validate dashboard without id and containing an alert", func() { - json, err := ioutil.ReadFile("./testdata/dash-without-id.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - dash := m.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - err = extractor.ValidateAlerts() - - Convey("Should validate without error", func() { + Convey("Should be able to extract collapsed panels", func() { + json, err := ioutil.ReadFile("./testdata/collapsed-panels.json") So(err, ShouldBeNil) + + dashJson, err := simplejson.NewJson(json) + So(err, ShouldBeNil) + + dash := m.NewDashboardFromJson(dashJson) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + + Convey("Get rules without error", func() { + So(err, ShouldBeNil) + }) + + Convey("should be able to extract collapsed alerts", func() { + So(len(alerts), ShouldEqual, 4) + }) }) - Convey("Should fail on save", func() { - _, err := extractor.GetAlerts() - So(err.Error(), ShouldEqual, "Alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1") + Convey("Parse and validate dashboard without id and containing an alert", func() { + json, err := ioutil.ReadFile("./testdata/dash-without-id.json") + So(err, ShouldBeNil) + + dashJSON, err := simplejson.NewJson(json) + So(err, ShouldBeNil) + dash := m.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + err = extractor.ValidateAlerts() + + Convey("Should validate without error", func() { + So(err, ShouldBeNil) + }) + + Convey("Should fail on save", func() { + _, err := extractor.GetAlerts() + So(err.Error(), ShouldEqual, "Alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1") + }) }) }) }) diff --git a/pkg/services/alerting/rule.go b/pkg/services/alerting/rule.go index 75901ac5e2e..529f3b9ff95 100644 --- a/pkg/services/alerting/rule.go +++ b/pkg/services/alerting/rule.go @@ -9,6 +9,8 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" + + "github.com/grafana/grafana/pkg/bus" ) var ( @@ -126,12 +128,33 @@ func NewRuleFromDBAlert(ruleDef *m.Alert) (*Rule, error) { for _, v := range ruleDef.Settings.Get("notifications").MustArray() { jsonModel := simplejson.NewFromAny(v) - uid, err := jsonModel.Get("uid").String() - if err != nil { - return nil, ValidationError{Reason: "Invalid notification schema", DashboardId: model.DashboardId, Alertid: model.Id, PanelId: model.PanelId} - } - model.Notifications = append(model.Notifications, uid) + if id, err := jsonModel.Get("id").Int64(); err == nil { + cmd := m.GetAlertNotificationsQuery{ + Id: id, + OrgId: ruleDef.OrgId, + } + if err = bus.Dispatch(&cmd); err != nil { + return nil, err + } + + if cmd.Result == nil { + errString := fmt.Sprintf("Alert notification id %d doesn't exist", id) + return nil, ValidationError{Reason: errString, DashboardId: model.DashboardId, Alertid: model.Id, PanelId: model.PanelId} + } + + if cmd.Result.Uid == "" { + errString := fmt.Sprintf("Alert notification id %d has empty uid", id) + return nil, ValidationError{Reason: errString, DashboardId: model.DashboardId, Alertid: model.Id, PanelId: model.PanelId} + } + model.Notifications = append(model.Notifications, cmd.Result.Uid) + } else { + if uid, err := jsonModel.Get("uid").String(); err != nil { + return nil, ValidationError{Reason: "Neither id nor uid is specified", DashboardId: model.DashboardId, Alertid: model.Id, PanelId: model.PanelId} + } else { + model.Notifications = append(model.Notifications, uid) + } + } } for index, condition := range ruleDef.Settings.Get("conditions").MustArray() { diff --git a/pkg/services/alerting/rule_test.go b/pkg/services/alerting/rule_test.go index 5ebae97caed..9b99278a5aa 100644 --- a/pkg/services/alerting/rule_test.go +++ b/pkg/services/alerting/rule_test.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/sqlstore" . "github.com/smartystreets/goconvey/convey" ) @@ -45,6 +46,7 @@ func TestAlertRuleFrequencyParsing(t *testing.T) { } func TestAlertRuleModel(t *testing.T) { + sqlstore.InitTestDB(t) Convey("Testing alert rule", t, func() { RegisterCondition("test", func(model *simplejson.Json, index int) (Condition, error) { @@ -57,46 +59,59 @@ func TestAlertRuleModel(t *testing.T) { }) Convey("can construct alert rule model", func() { - json := ` - { - "name": "name2", - "description": "desc2", - "handler": 0, - "noDataMode": "critical", - "enabled": true, - "frequency": "60s", - "conditions": [ - { - "type": "test", - "prop": 123 - } - ], - "notifications": [ - {"uid": "1134"}, - {"uid": "22"} - ] - } - ` - - alertJSON, jsonErr := simplejson.NewJson([]byte(json)) - So(jsonErr, ShouldBeNil) - - alert := &m.Alert{ - Id: 1, - OrgId: 1, - DashboardId: 1, - PanelId: 1, - - Settings: alertJSON, - } - - alertRule, err := NewRuleFromDBAlert(alert) + err := sqlstore.CreateOrg(&m.CreateOrgCommand{Name: "Main Org."}) + So(err, ShouldBeNil) + firstNotification := m.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} + err = sqlstore.CreateAlertNotificationCommand(&firstNotification) + So(err, ShouldBeNil) + secondNotification := m.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} + err = sqlstore.CreateAlertNotificationCommand(&secondNotification) So(err, ShouldBeNil) - So(len(alertRule.Conditions), ShouldEqual, 1) + Convey("with notification id and uid", func() { + json := ` + { + "name": "name2", + "description": "desc2", + "handler": 0, + "noDataMode": "critical", + "enabled": true, + "frequency": "60s", + "conditions": [ + { + "type": "test", + "prop": 123 + } + ], + "notifications": [ + {"id": 1}, + {"uid": "notifier2"} + ] + } + ` - Convey("Can read notifications", func() { - So(len(alertRule.Notifications), ShouldEqual, 2) + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + So(jsonErr, ShouldBeNil) + + alert := &m.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + + Settings: alertJSON, + } + + alertRule, err := NewRuleFromDBAlert(alert) + So(err, ShouldBeNil) + + So(len(alertRule.Conditions), ShouldEqual, 1) + + Convey("Can read notifications", func() { + So(len(alertRule.Notifications), ShouldEqual, 2) + So(alertRule.Notifications, ShouldContain, "notifier1") + So(alertRule.Notifications, ShouldContain, "notifier2") + }) }) }) @@ -130,5 +145,43 @@ func TestAlertRuleModel(t *testing.T) { So(alertRule.Frequency, ShouldEqual, 60) }) + Convey("raise error in case of missing notification id and uid", func() { + json := ` + { + "name": "name2", + "description": "desc2", + "noDataMode": "critical", + "enabled": true, + "frequency": "60s", + "conditions": [ + { + "type": "test", + "prop": 123 + } + ], + "notifications": [ + {"not_id_uid": "1134"} + ] + } + ` + + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + So(jsonErr, ShouldBeNil) + + alert := &m.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + Frequency: 0, + + Settings: alertJSON, + } + + _, err := NewRuleFromDBAlert(alert) + So(err, ShouldNotBeNil) + So(err.Error(), ShouldEqual, "Alert validation error: Neither id nor uid is specified AlertId: 1 PanelId: 1 DashboardId: 1") + }) + }) } diff --git a/pkg/services/alerting/testdata/dash-without-id.json b/pkg/services/alerting/testdata/dash-without-id.json index 59fbd679cf2..02cd2c002f0 100644 --- a/pkg/services/alerting/testdata/dash-without-id.json +++ b/pkg/services/alerting/testdata/dash-without-id.json @@ -45,6 +45,9 @@ "notifications": [ { "uid": "notifier1" + }, + { + "id": 2 } ] }, diff --git a/pkg/services/alerting/testdata/influxdb-alert.json b/pkg/services/alerting/testdata/influxdb-alert.json index 2dcec447b15..29f1a0c8e5e 100644 --- a/pkg/services/alerting/testdata/influxdb-alert.json +++ b/pkg/services/alerting/testdata/influxdb-alert.json @@ -45,7 +45,10 @@ "noDataState": "no_data", "notifications": [ { - "uid": "notifier1" + "id": 1 + }, + { + "uid": "notifier2" } ] }, diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index 9231c896cf1..0b958489624 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -130,6 +130,7 @@ func getAlertNotificationInternal(query *m.GetAlertNotificationsQuery, sess *DBS sql.WriteString(`SELECT alert_notification.id, + alert_notification.uid, alert_notification.org_id, alert_notification.name, alert_notification.type,