dashboard: allow alerts to be saved for new/provisioned dashboards

This changes the logic for the alert validation in the extractor. Validation
is executed twice, once before attempting to save the alerts and once after
saving the dashboard while saving the alerts to the db. The first validation
will not require that the alert has a dashboard id which allows new dashboards
with alerts to be saved.

Fixes #11247

pkg/services/alerting/commands.go

@@ -13,11 +13,7 @@ func init() {
 func validateDashboardAlerts(cmd *m.ValidateDashboardAlertsCommand) error {
 	extractor := NewDashAlertExtractor(cmd.Dashboard, cmd.OrgId)
 
-	if _, err := extractor.GetAlerts(); err != nil {
-		return err
-	}
-
-	return nil
+	return extractor.ValidateAlerts()
 }
 
 func updateDashboardAlerts(cmd *m.UpdateDashboardAlertsCommand) error {
@@ -29,15 +25,12 @@ func updateDashboardAlerts(cmd *m.UpdateDashboardAlertsCommand) error {
 
 	extractor := NewDashAlertExtractor(cmd.Dashboard, cmd.OrgId)
 
-	if alerts, err := extractor.GetAlerts(); err != nil {
-		return err
-	} else {
-		saveAlerts.Alerts = alerts
-	}
-
-	if err := bus.Dispatch(&saveAlerts); err != nil {
+	alerts, err := extractor.GetAlerts()
+	if err != nil {
 		return err
 	}
 
-	return nil
+	saveAlerts.Alerts = alerts
+
+	return bus.Dispatch(&saveAlerts)
 }

pkg/services/alerting/extractor.go

@@ -11,76 +11,78 @@ import (
 	m "github.com/grafana/grafana/pkg/models"
 )
 
+// DashAlertExtractor extracts alerts from the dashboard json
 type DashAlertExtractor struct {
 	Dash  *m.Dashboard
-	OrgId int64
+	OrgID int64
 	log   log.Logger
 }
 
-func NewDashAlertExtractor(dash *m.Dashboard, orgId int64) *DashAlertExtractor {
+// NewDashAlertExtractor returns a new DashAlertExtractor
+func NewDashAlertExtractor(dash *m.Dashboard, orgID int64) *DashAlertExtractor {
 	return &DashAlertExtractor{
 		Dash:  dash,
-		OrgId: orgId,
+		OrgID: orgID,
 		log:   log.New("alerting.extractor"),
 	}
 }
 
-func (e *DashAlertExtractor) lookupDatasourceId(dsName string) (*m.DataSource, error) {
+func (e *DashAlertExtractor) lookupDatasourceID(dsName string) (*m.DataSource, error) {
 	if dsName == "" {
-		query := &m.GetDataSourcesQuery{OrgId: e.OrgId}
+		query := &m.GetDataSourcesQuery{OrgId: e.OrgID}
 		if err := bus.Dispatch(query); err != nil {
 			return nil, err
-		} else {
-			for _, ds := range query.Result {
-				if ds.IsDefault {
-					return ds, nil
-				}
+		}
+
+		for _, ds := range query.Result {
+			if ds.IsDefault {
+				return ds, nil
 			}
 		}
 	} else {
-		query := &m.GetDataSourceByNameQuery{Name: dsName, OrgId: e.OrgId}
+		query := &m.GetDataSourceByNameQuery{Name: dsName, OrgId: e.OrgID}
 		if err := bus.Dispatch(query); err != nil {
 			return nil, err
-		} else {
-			return query.Result, nil
 		}
+
+		return query.Result, nil
 	}
 
 	return nil, errors.New("Could not find datasource id for " + dsName)
 }
 
-func findPanelQueryByRefId(panel *simplejson.Json, refId string) *simplejson.Json {
+func findPanelQueryByRefID(panel *simplejson.Json, refID string) *simplejson.Json {
 	for _, targetsObj := range panel.Get("targets").MustArray() {
 		target := simplejson.NewFromAny(targetsObj)
 
-		if target.Get("refId").MustString() == refId {
+		if target.Get("refId").MustString() == refID {
 			return target
 		}
 	}
 	return nil
 }
 
-func copyJson(in *simplejson.Json) (*simplejson.Json, error) {
-	rawJson, err := in.MarshalJSON()
+func copyJSON(in *simplejson.Json) (*simplejson.Json, error) {
+	rawJSON, err := in.MarshalJSON()
 	if err != nil {
 		return nil, err
 	}
 
-	return simplejson.NewJson(rawJson)
+	return simplejson.NewJson(rawJSON)
 }
 
-func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json) ([]*m.Alert, error) {
+func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, validateAlertFunc func(*m.Alert) bool) ([]*m.Alert, error) {
 	alerts := make([]*m.Alert, 0)
 
 	for _, panelObj := range jsonWithPanels.Get("panels").MustArray() {
 		panel := simplejson.NewFromAny(panelObj)
 
-		collapsedJson, collapsed := panel.CheckGet("collapsed")
+		collapsedJSON, collapsed := panel.CheckGet("collapsed")
 		// check if the panel is collapsed
-		if collapsed && collapsedJson.MustBool() {
+		if collapsed && collapsedJSON.MustBool() {
 
 			// extract alerts from sub panels for collapsed panels
-			als, err := e.GetAlertFromPanels(panel)
+			als, err := e.getAlertFromPanels(panel, validateAlertFunc)
 			if err != nil {
 				return nil, err
 			}
@@ -95,7 +97,7 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json)
 			continue
 		}
 
-		panelId, err := panel.Get("id").Int64()
+		panelID, err := panel.Get("id").Int64()
 		if err != nil {
 			return nil, fmt.Errorf("panel id is required. err %v", err)
 		}
@@ -113,8 +115,8 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json)
 
 		alert := &m.Alert{
 			DashboardId: e.Dash.Id,
-			OrgId:       e.OrgId,
-			PanelId:     panelId,
+			OrgId:       e.OrgID,
+			PanelId:     panelID,
 			Id:          jsonAlert.Get("id").MustInt64(),
 			Name:        jsonAlert.Get("name").MustString(),
 			Handler:     jsonAlert.Get("handler").MustInt64(),
@@ -126,11 +128,11 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json)
 			jsonCondition := simplejson.NewFromAny(condition)
 
 			jsonQuery := jsonCondition.Get("query")
-			queryRefId := jsonQuery.Get("params").MustArray()[0].(string)
-			panelQuery := findPanelQueryByRefId(panel, queryRefId)
+			queryRefID := jsonQuery.Get("params").MustArray()[0].(string)
+			panelQuery := findPanelQueryByRefID(panel, queryRefID)
 
 			if panelQuery == nil {
-				reason := fmt.Sprintf("Alert on PanelId: %v refers to query(%s) that cannot be found", alert.PanelId, queryRefId)
+				reason := fmt.Sprintf("Alert on PanelId: %v refers to query(%s) that cannot be found", alert.PanelId, queryRefID)
 				return nil, ValidationError{Reason: reason}
 			}
 
@@ -141,12 +143,13 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json)
 				dsName = panel.Get("datasource").MustString()
 			}
 
-			if datasource, err := e.lookupDatasourceId(dsName); err != nil {
+			datasource, err := e.lookupDatasourceID(dsName)
+			if err != nil {
 				return nil, err
-			} else {
-				jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id)
 			}
 
+			jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id)
+
 			if interval, err := panel.Get("interval").String(); err == nil {
 				panelQuery.Set("interval", interval)
 			}
@@ -162,21 +165,28 @@ func (e *DashAlertExtractor) GetAlertFromPanels(jsonWithPanels *simplejson.Json)
 			return nil, err
 		}
 
-		if alert.ValidToSave() {
-			alerts = append(alerts, alert)
-		} else {
+		if !validateAlertFunc(alert) {
 			e.log.Debug("Invalid Alert Data. Dashboard, Org or Panel ID is not correct", "alertName", alert.Name, "panelId", alert.PanelId)
 			return nil, m.ErrDashboardContainsInvalidAlertData
 		}
+
+		alerts = append(alerts, alert)
 	}
 
 	return alerts, nil
 }
 
-func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) {
-	e.log.Debug("GetAlerts")
+func validateAlertRule(alert *m.Alert) bool {
+	return alert.ValidToSave()
+}
 
-	dashboardJson, err := copyJson(e.Dash.Data)
+// GetAlerts extracts alerts from the dashboard json and does full validation on the alert json data
+func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) {
+	return e.extractAlerts(validateAlertRule)
+}
+
+func (e *DashAlertExtractor) extractAlerts(validateFunc func(alert *m.Alert) bool) ([]*m.Alert, error) {
+	dashboardJSON, err := copyJSON(e.Dash.Data)
 	if err != nil {
 		return nil, err
 	}
@@ -185,11 +195,11 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) {
 
 	// We extract alerts from rows to be backwards compatible
 	// with the old dashboard json model.
-	rows := dashboardJson.Get("rows").MustArray()
+	rows := dashboardJSON.Get("rows").MustArray()
 	if len(rows) > 0 {
 		for _, rowObj := range rows {
 			row := simplejson.NewFromAny(rowObj)
-			a, err := e.GetAlertFromPanels(row)
+			a, err := e.getAlertFromPanels(row, validateFunc)
 			if err != nil {
 				return nil, err
 			}
@@ -197,7 +207,7 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) {
 			alerts = append(alerts, a...)
 		}
 	} else {
-		a, err := e.GetAlertFromPanels(dashboardJson)
+		a, err := e.getAlertFromPanels(dashboardJSON, validateFunc)
 		if err != nil {
 			return nil, err
 		}
@@ -208,3 +218,10 @@ func (e *DashAlertExtractor) GetAlerts() ([]*m.Alert, error) {
 	e.log.Debug("Extracted alerts from dashboard", "alertCount", len(alerts))
 	return alerts, nil
 }
+
+// ValidateAlerts validates alerts in the dashboard json but does not require a valid dashboard id
+// in the first validation pass
+func (e *DashAlertExtractor) ValidateAlerts() error {
+	_, err := e.extractAlerts(func(alert *m.Alert) bool { return alert.OrgId != 0 && alert.PanelId != 0 })
+	return err
+}

pkg/services/alerting/extractor_test.go

@@ -240,5 +240,26 @@ func TestAlertRuleExtraction(t *testing.T) {
 				So(len(alerts), ShouldEqual, 4)
 			})
 		})
+
+		Convey("Parse and validate dashboard without id and containing an alert", func() {
+			json, err := ioutil.ReadFile("./test-data/dash-without-id.json")
+			So(err, ShouldBeNil)
+
+			dashJSON, err := simplejson.NewJson(json)
+			So(err, ShouldBeNil)
+			dash := m.NewDashboardFromJson(dashJSON)
+			extractor := NewDashAlertExtractor(dash, 1)
+
+			err = extractor.ValidateAlerts()
+
+			Convey("Should validate without error", func() {
+				So(err, ShouldBeNil)
+			})
+
+			Convey("Should fail on save", func() {
+				_, err := extractor.GetAlerts()
+				So(err, ShouldEqual, m.ErrDashboardContainsInvalidAlertData)
+			})
+		})
 	})
 }

pkg/services/alerting/test-data/dash-without-id.json

@@ -0,0 +1,281 @@
+{
+    "title": "Influxdb",
+    "tags": [
+      "apa"
+    ],
+    "style": "dark",
+    "timezone": "browser",
+    "editable": true,
+    "hideControls": false,
+    "sharedCrosshair": false,
+    "rows": [
+      {
+        "collapse": false,
+        "editable": true,
+        "height": "450px",
+        "panels": [
+          {
+            "alert": {
+              "conditions": [
+                {
+                  "evaluator": {
+                    "params": [
+                      10
+                    ],
+                    "type": "gt"
+                  },
+                  "query": {
+                    "params": [
+                      "B",
+                      "5m",
+                      "now"
+                    ]
+                  },
+                  "reducer": {
+                    "params": [],
+                    "type": "avg"
+                  },
+                  "type": "query"
+                }
+              ],
+              "frequency": "3s",
+              "handler": 1,
+              "name": "Influxdb",
+              "noDataState": "no_data",
+              "notifications": [
+                {
+                  "id": 6
+                }
+              ]
+            },
+            "alerting": {},
+            "aliasColors": {
+              "logins.count.count": "#890F02"
+            },
+            "bars": false,
+            "datasource": "InfluxDB",
+            "editable": true,
+            "error": false,
+            "fill": 1,
+            "grid": {},
+            "id": 1,
+            "interval": ">10s",
+            "isNew": true,
+            "legend": {
+              "avg": false,
+              "current": false,
+              "max": false,
+              "min": false,
+              "show": true,
+              "total": false,
+              "values": false
+            },
+            "lines": true,
+            "linewidth": 2,
+            "links": [],
+            "nullPointMode": "connected",
+            "percentage": false,
+            "pointradius": 5,
+            "points": false,
+            "renderer": "flot",
+            "seriesOverrides": [],
+            "span": 10,
+            "stack": false,
+            "steppedLine": false,
+            "targets": [
+              {
+                "groupBy": [
+                  {
+                    "params": [
+                      "$interval"
+                    ],
+                    "type": "time"
+                  },
+                  {
+                    "params": [
+                      "datacenter"
+                    ],
+                    "type": "tag"
+                  },
+                  {
+                    "params": [
+                      "none"
+                    ],
+                    "type": "fill"
+                  }
+                ],
+                "hide": false,
+                "measurement": "logins.count",
+                "policy": "default",
+                "query": "SELECT 8 * count(\"value\") FROM \"logins.count\" WHERE $timeFilter GROUP BY time($interval), \"datacenter\" fill(none)",
+                "rawQuery": true,
+                "refId": "B",
+                "resultFormat": "time_series",
+                "select": [
+                  [
+                    {
+                      "params": [
+                        "value"
+                      ],
+                      "type": "field"
+                    },
+                    {
+                      "params": [],
+                      "type": "count"
+                    }
+                  ]
+                ],
+                "tags": []
+              },
+              {
+                "groupBy": [
+                  {
+                    "params": [
+                      "$interval"
+                    ],
+                    "type": "time"
+                  },
+                  {
+                    "params": [
+                      "null"
+                    ],
+                    "type": "fill"
+                  }
+                ],
+                "hide": true,
+                "measurement": "cpu",
+                "policy": "default",
+                "refId": "A",
+                "resultFormat": "time_series",
+                "select": [
+                  [
+                    {
+                      "params": [
+                        "value"
+                      ],
+                      "type": "field"
+                    },
+                    {
+                      "params": [],
+                      "type": "mean"
+                    }
+                  ],
+                  [
+                    {
+                      "params": [
+                        "value"
+                      ],
+                      "type": "field"
+                    },
+                    {
+                      "params": [],
+                      "type": "sum"
+                    }
+                  ]
+                ],
+                "tags": []
+              }
+            ],
+            "thresholds": [
+              {
+                "colorMode": "critical",
+                "fill": true,
+                "line": true,
+                "op": "gt",
+                "value": 10
+              }
+            ],
+            "timeFrom": null,
+            "timeShift": null,
+            "title": "Panel Title",
+            "tooltip": {
+              "msResolution": false,
+              "ordering": "alphabetical",
+              "shared": true,
+              "sort": 0,
+              "value_type": "cumulative"
+            },
+            "type": "graph",
+            "xaxis": {
+              "mode": "time",
+              "name": null,
+              "show": true,
+              "values": []
+            },
+            "yaxes": [
+              {
+                "format": "short",
+                "logBase": 1,
+                "max": null,
+                "min": null,
+                "show": true
+              },
+              {
+                "format": "short",
+                "logBase": 1,
+                "max": null,
+                "min": null,
+                "show": true
+              }
+            ]
+          },
+          {
+            "editable": true,
+            "error": false,
+            "id": 2,
+            "isNew": true,
+            "limit": 10,
+            "links": [],
+            "show": "current",
+            "span": 2,
+            "stateFilter": [
+              "alerting"
+            ],
+            "title": "Alert status",
+            "type": "alertlist"
+          }
+        ],
+        "title": "Row"
+      }
+    ],
+    "time": {
+      "from": "now-5m",
+      "to": "now"
+    },
+    "timepicker": {
+      "now": true,
+      "refresh_intervals": [
+        "5s",
+        "10s",
+        "30s",
+        "1m",
+        "5m",
+        "15m",
+        "30m",
+        "1h",
+        "2h",
+        "1d"
+      ],
+      "time_options": [
+        "5m",
+        "15m",
+        "1h",
+        "6h",
+        "12h",
+        "24h",
+        "2d",
+        "7d",
+        "30d"
+      ]
+    },
+    "templating": {
+      "list": []
+    },
+    "annotations": {
+      "list": []
+    },
+    "schemaVersion": 13,
+    "version": 120,
+    "links": [],
+    "gnetId": null
+  }

pkg/services/alerting/test-data/influxdb-alert.json

@@ -279,4 +279,4 @@
     "version": 120,
     "links": [],
     "gnetId": null
-  }
+  }