alerting: propagate alert validation issues to the user instead of just 'invalid alert data' message

pkg/api/dashboard.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path"
 
+	"github.com/grafana/grafana/pkg/services/alerting"
 	"github.com/grafana/grafana/pkg/services/dashboards"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
@@ -251,8 +252,8 @@ func PostDashboard(c *m.ReqContext, cmd m.SaveDashboardCommand) Response {
 		return Error(403, err.Error(), err)
 	}
 
-	if err == m.ErrDashboardContainsInvalidAlertData {
+	if validationErr, ok := err.(alerting.ValidationError); ok {
-		return Error(500, "Invalid alert data. Cannot save dashboard", err)
+		return Error(422, validationErr.Error(), nil)
 	}
 
 	if err != nil {

pkg/api/dashboard_test.go

@@ -9,6 +9,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/alerting"
 	"github.com/grafana/grafana/pkg/services/dashboards"
 	"github.com/grafana/grafana/pkg/setting"
 
@@ -725,7 +726,7 @@ func TestDashboardApiEndpoint(t *testing.T) {
 				{SaveError: m.ErrDashboardVersionMismatch, ExpectedStatusCode: 412},
 				{SaveError: m.ErrDashboardTitleEmpty, ExpectedStatusCode: 400},
 				{SaveError: m.ErrDashboardFolderCannotHaveParent, ExpectedStatusCode: 400},
-				{SaveError: m.ErrDashboardContainsInvalidAlertData, ExpectedStatusCode: 500},
+				{SaveError: alerting.ValidationError{Reason: "Mu"}, ExpectedStatusCode: 422},
 				{SaveError: m.ErrDashboardFailedToUpdateAlertData, ExpectedStatusCode: 500},
 				{SaveError: m.ErrDashboardFailedGenerateUniqueUid, ExpectedStatusCode: 500},
 				{SaveError: m.ErrDashboardTypeMismatch, ExpectedStatusCode: 400},

pkg/models/dashboards.go

@@ -21,7 +21,6 @@ var (
 	ErrDashboardVersionMismatch                = errors.New("The dashboard has been changed by someone else")
 	ErrDashboardTitleEmpty                     = errors.New("Dashboard title cannot be empty")
 	ErrDashboardFolderCannotHaveParent         = errors.New("A Dashboard Folder cannot be added to another folder")
-	ErrDashboardContainsInvalidAlertData       = errors.New("Invalid alert data. Cannot save dashboard")
 	ErrDashboardFailedToUpdateAlertData        = errors.New("Failed to save alert data")
 	ErrDashboardsWithSameSlugExists            = errors.New("Multiple dashboards with the same slug exists")
 	ErrDashboardFailedGenerateUniqueUid        = errors.New("Failed to generate unique dashboard id")

pkg/services/alerting/extractor.go

@@ -82,8 +82,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json,
 		if collapsed && collapsedJSON.MustBool() {
 
 			// extract alerts from sub panels for collapsed panels
-			alertSlice, err := e.getAlertFromPanels(panel,
+			alertSlice, err := e.getAlertFromPanels(panel, validateAlertFunc)
-				validateAlertFunc)
 			if err != nil {
 				return nil, err
 			}
@@ -100,7 +99,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json,
 
 		panelID, err := panel.Get("id").Int64()
 		if err != nil {
-			return nil, fmt.Errorf("panel id is required. err %v", err)
+			return nil, ValidationError{Reason: "A numeric panel id property is missing"}
 		}
 
 		// backward compatibility check, can be removed later
@@ -146,7 +145,8 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json,
 
 			datasource, err := e.lookupDatasourceID(dsName)
 			if err != nil {
-				return nil, err
+				e.log.Debug("Error looking up datasource", "error", err)
+				return nil, ValidationError{Reason: fmt.Sprintf("Data source used by alert rule not found, alertName=%v, datasource=%s", alert.Name, dsName)}
 			}
 
 			jsonQuery.SetPath([]string{"datasourceId"}, datasource.Id)
@@ -167,8 +167,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json,
 		}
 
 		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, ValidationError{Reason: fmt.Sprintf("Panel id is not correct, alertName=%v, panelId=%v", alert.Name, alert.PanelId)}
-			return nil, m.ErrDashboardContainsInvalidAlertData
 		}
 
 		alerts = append(alerts, alert)

pkg/services/alerting/extractor_test.go

@@ -258,7 +258,7 @@ func TestAlertRuleExtraction(t *testing.T) {
 
 			Convey("Should fail on save", func() {
 				_, err := extractor.GetAlerts()
-				So(err, ShouldEqual, m.ErrDashboardContainsInvalidAlertData)
+				So(err.Error(), ShouldEqual, "Alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1")
 			})
 		})
 	})

pkg/services/alerting/rule.go

@@ -36,13 +36,13 @@ type ValidationError struct {
 }
 
 func (e ValidationError) Error() string {
-	extraInfo := ""
+	extraInfo := e.Reason
 	if e.Alertid != 0 {
 		extraInfo = fmt.Sprintf("%s AlertId: %v", extraInfo, e.Alertid)
 	}
 
 	if e.PanelId != 0 {
-		extraInfo = fmt.Sprintf("%s PanelId: %v ", extraInfo, e.PanelId)
+		extraInfo = fmt.Sprintf("%s PanelId: %v", extraInfo, e.PanelId)
 	}
 
 	if e.DashboardId != 0 {
@@ -50,10 +50,10 @@ func (e ValidationError) Error() string {
 	}
 
 	if e.Err != nil {
-		return fmt.Sprintf("%s %s%s", e.Err.Error(), e.Reason, extraInfo)
+		return fmt.Sprintf("Alert validation error: %s%s", e.Err.Error(), extraInfo)
 	}
 
-	return fmt.Sprintf("Failed to extract alert.Reason: %s %s", e.Reason, extraInfo)
+	return fmt.Sprintf("Alert validation error: %s", extraInfo)
 }
 
 var (

pkg/services/dashboards/dashboard_service.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/grafana/grafana/pkg/bus"
+	"github.com/grafana/grafana/pkg/log"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/guardian"
 	"github.com/grafana/grafana/pkg/util"
@@ -25,7 +26,9 @@ type DashboardProvisioningService interface {
 
 // NewService factory for creating a new dashboard service
 var NewService = func() DashboardService {
-	return &dashboardServiceImpl{}
+	return &dashboardServiceImpl{
+		log: log.New("dashboard-service"),
+	}
 }
 
 // NewProvisioningService factory for creating a new dashboard provisioning service
@@ -45,6 +48,7 @@ type SaveDashboardDTO struct {
 type dashboardServiceImpl struct {
 	orgId int64
 	user  *models.SignedInUser
+	log   log.Logger
 }
 
 func (dr *dashboardServiceImpl) GetProvisionedDashboardData(name string) ([]*models.DashboardProvisioning, error) {
@@ -89,7 +93,7 @@ func (dr *dashboardServiceImpl) buildSaveDashboardCommand(dto *SaveDashboardDTO,
 		}
 
 		if err := bus.Dispatch(&validateAlertsCmd); err != nil {
-			return nil, models.ErrDashboardContainsInvalidAlertData
+			return nil, err
 		}
 	}
 

pkg/services/dashboards/dashboard_service_test.go

@@ -117,12 +117,12 @@ func TestDashboardService(t *testing.T) {
 				})
 
 				bus.AddHandler("test", func(cmd *models.ValidateDashboardAlertsCommand) error {
-					return errors.New("error")
+					return errors.New("Alert validation error")
 				})
 
 				dto.Dashboard = models.NewDashboard("Dash")
 				_, err := service.SaveDashboard(dto)
-				So(err, ShouldEqual, models.ErrDashboardContainsInvalidAlertData)
+				So(err.Error(), ShouldEqual, "Alert validation error")
 			})
 		})