AlertNotifications: Translate notifications IDs to UIDs in Rule builder (#19882)

* AlertNotifications: Translate notifications IDs to UIDs in alert Rule builder

* Avoid shadowing errors, raise validation error on non-existing notification id

* create a cache for notification Uids to minimize db overhead

* add cache usage test

* avoid caching empty notification Uids

* isolate db in alert notificationUid caching tests

pkg/models/alert_notifications.go

@@ -91,6 +91,13 @@ type DeleteAlertNotificationWithUidCommand struct {
 	OrgId int64
 }
 
+type GetAlertNotificationUidQuery struct {
+	Id    int64
+	OrgId int64
+
+	Result string
+}
+
 type GetAlertNotificationsQuery struct {
 	Name  string
 	Id    int64

pkg/services/alerting/rule.go

@@ -7,6 +7,7 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/simplejson"
 	"github.com/grafana/grafana/pkg/models"
 )
@@ -137,13 +138,15 @@ func NewRuleFromDBAlert(ruleDef *models.Alert) (*Rule, error) {
 	for _, v := range ruleDef.Settings.Get("notifications").MustArray() {
 		jsonModel := simplejson.NewFromAny(v)
 		if id, err := jsonModel.Get("id").Int64(); err == nil {
-			model.Notifications = append(model.Notifications, fmt.Sprintf("%09d", id))
-		} else {
-			uid, err := jsonModel.Get("uid").String()
+			uid, err := translateNotificationIDToUID(id, ruleDef.OrgId)
 			if err != nil {
-				return nil, ValidationError{Reason: "Neither id nor uid is specified in 'notifications' block, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID}
+				return nil, ValidationError{Reason: "Unable to translate notification id to uid, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID}
 			}
 			model.Notifications = append(model.Notifications, uid)
+		} else if uid, err := jsonModel.Get("uid").String(); err == nil {
+			model.Notifications = append(model.Notifications, uid)
+		} else {
+			return nil, ValidationError{Reason: "Neither id nor uid is specified in 'notifications' block, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID}
 		}
 	}
 	model.AlertRuleTags = ruleDef.GetTagsFromSettings()
@@ -169,6 +172,29 @@ func NewRuleFromDBAlert(ruleDef *models.Alert) (*Rule, error) {
 	return model, nil
 }
 
+func translateNotificationIDToUID(id int64, orgID int64) (string, error) {
+	notificationUid, err := getAlertNotificationUidByIDAndOrgID(id, orgID)
+	if err != nil {
+		logger.Debug("Failed to translate Notification Id to Uid", "orgID", orgID, "Id", id)
+		return "", err
+	}
+
+	return notificationUid, nil
+}
+
+func getAlertNotificationUidByIDAndOrgID(notificationID int64, orgID int64) (string, error) {
+	query := &models.GetAlertNotificationUidQuery{
+		OrgId: orgID,
+		Id:    notificationID,
+	}
+
+	if err := bus.Dispatch(query); err != nil {
+		return "", err
+	}
+
+	return query.Result, nil
+}
+
 // ConditionFactory is the function signature for creating `Conditions`.
 type ConditionFactory func(model *simplejson.Json, index int) (Condition, error)
 

pkg/services/alerting/rule_test.go

@@ -60,7 +60,7 @@ func TestAlertRuleModel(t *testing.T) {
 		})
 
 		Convey("can construct alert rule model", func() {
-			firstNotification := models.CreateAlertNotificationCommand{OrgId: 1, Name: "1"}
+			firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"}
 			err := sqlstore.CreateAlertNotificationCommand(&firstNotification)
 			So(err, ShouldBeNil)
 			secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"}
@@ -105,15 +105,50 @@ func TestAlertRuleModel(t *testing.T) {
 				So(err, ShouldBeNil)
 
 				So(len(alertRule.Conditions), ShouldEqual, 1)
+				So(len(alertRule.Notifications), ShouldEqual, 2)
 
-				Convey("Can read notifications", func() {
-					So(len(alertRule.Notifications), ShouldEqual, 2)
-					So(alertRule.Notifications, ShouldContain, "000000001")
+				Convey("Can read Id and Uid notifications (translate Id to Uid)", func() {
 					So(alertRule.Notifications, ShouldContain, "notifier2")
+					So(alertRule.Notifications, ShouldContain, "notifier1")
 				})
 			})
 		})
 
+		Convey("with non existing notification id", func() {
+			json := `
+				{
+					"name": "name3",
+					"description": "desc3",
+					"handler": 0,
+					"noDataMode": "critical",
+					"enabled": true,
+					"frequency": "60s",
+					"conditions": [{"type": "test", "prop": 123 }],
+					"notifications": [
+						{"id": 999}
+					]
+				}
+				`
+
+			alertJSON, jsonErr := simplejson.NewJson([]byte(json))
+			So(jsonErr, ShouldBeNil)
+
+			alert := &models.Alert{
+				Id:          1,
+				OrgId:       1,
+				DashboardId: 1,
+				PanelId:     1,
+
+				Settings: alertJSON,
+			}
+
+			_, err := NewRuleFromDBAlert(alert)
+			Convey("raises an error", func() {
+				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldEqual, "Alert validation error: Unable to translate notification id to uid, Alert notification [ Id: 999, OrgId: 1 ] not found AlertId: 1 PanelId: 1 DashboardId: 1")
+			})
+		})
+
 		Convey("can construct alert rule model with invalid frequency", func() {
 			json := `
 			{

pkg/services/sqlstore/alert_notification.go

@@ -67,6 +67,32 @@ func GetAlertNotifications(query *models.GetAlertNotificationsQuery) error {
 	return getAlertNotificationInternal(query, newSession())
 }
 
+func (ss *SqlStore) addAlertNotificationUidByIdHandler() {
+	bus.AddHandler("sql", ss.GetAlertNotificationUidWithId)
+}
+
+func (ss *SqlStore) GetAlertNotificationUidWithId(query *models.GetAlertNotificationUidQuery) error {
+	cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id)
+
+	if cached, found := ss.CacheService.Get(cacheKey); found {
+		query.Result = cached.(string)
+		return nil
+	}
+
+	err := getAlertNotificationUidInternal(query, newSession())
+	if err != nil {
+		return err
+	}
+
+	ss.CacheService.Set(cacheKey, query.Result, -1) //Infinite, never changes
+
+	return nil
+}
+
+func newAlertNotificationUidCacheKey(orgID, notificationId int64) string {
+	return fmt.Sprintf("notification-uid-by-org-%d-and-id-%d", orgID, notificationId)
+}
+
 func GetAlertNotificationsWithUid(query *models.GetAlertNotificationsWithUidQuery) error {
 	return getAlertNotificationWithUidInternal(query, newSession())
 }
@@ -124,6 +150,35 @@ func GetAlertNotificationsWithUidToSend(query *models.GetAlertNotificationsWithU
 	return nil
 }
 
+func getAlertNotificationUidInternal(query *models.GetAlertNotificationUidQuery, sess *DBSession) error {
+	var sql bytes.Buffer
+	params := make([]interface{}, 0)
+
+	sql.WriteString(`SELECT
+										alert_notification.uid
+										FROM alert_notification
+	  							`)
+
+	sql.WriteString(` WHERE alert_notification.org_id = ?`)
+	params = append(params, query.OrgId)
+
+	sql.WriteString(` AND alert_notification.id = ?`)
+	params = append(params, query.Id)
+
+	results := make([]string, 0)
+	if err := sess.SQL(sql.String(), params...).Find(&results); err != nil {
+		return err
+	}
+
+	if len(results) == 0 {
+		return fmt.Errorf("Alert notification [ Id: %v, OrgId: %v ] not found", query.Id, query.OrgId)
+	}
+
+	query.Result = results[0]
+
+	return nil
+}
+
 func getAlertNotificationInternal(query *models.GetAlertNotificationsQuery, sess *DBSession) error {
 	var sql bytes.Buffer
 	params := make([]interface{}, 0)

pkg/services/sqlstore/alert_notification_test.go

@@ -321,5 +321,71 @@ func TestAlertNotificationSQLAccess(t *testing.T) {
 				So(len(query.Result), ShouldEqual, 4)
 			})
 		})
+
+		Convey("Notification Uid by Id Caching", func() {
+			ss := InitTestDB(t)
+
+			notification := &models.CreateAlertNotificationCommand{Uid: "aNotificationUid", OrgId: 1, Name: "aNotificationUid"}
+			err := CreateAlertNotificationCommand(notification)
+			So(err, ShouldBeNil)
+
+			byUidQuery := &models.GetAlertNotificationsWithUidQuery{
+				Uid:   notification.Uid,
+				OrgId: notification.OrgId,
+			}
+
+			notificationByUidErr := GetAlertNotificationsWithUid(byUidQuery)
+			So(notificationByUidErr, ShouldBeNil)
+
+			Convey("Can cache notification Uid", func() {
+				byIdQuery := &models.GetAlertNotificationUidQuery{
+					Id:    byUidQuery.Result.Id,
+					OrgId: byUidQuery.Result.OrgId,
+				}
+
+				cacheKey := newAlertNotificationUidCacheKey(byIdQuery.OrgId, byIdQuery.Id)
+
+				resultBeforeCaching, foundBeforeCaching := ss.CacheService.Get(cacheKey)
+				So(foundBeforeCaching, ShouldBeFalse)
+				So(resultBeforeCaching, ShouldBeNil)
+
+				notificationByIdErr := ss.GetAlertNotificationUidWithId(byIdQuery)
+				So(notificationByIdErr, ShouldBeNil)
+
+				resultAfterCaching, foundAfterCaching := ss.CacheService.Get(cacheKey)
+				So(foundAfterCaching, ShouldBeTrue)
+				So(resultAfterCaching, ShouldEqual, notification.Uid)
+			})
+
+			Convey("Retrieves from cache when exists", func() {
+				query := &models.GetAlertNotificationUidQuery{
+					Id:    999,
+					OrgId: 100,
+				}
+				cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id)
+				ss.CacheService.Set(cacheKey, "a-cached-uid", -1)
+
+				err := ss.GetAlertNotificationUidWithId(query)
+				So(err, ShouldBeNil)
+				So(query.Result, ShouldEqual, "a-cached-uid")
+			})
+
+			Convey("Returns an error without populating cache when the notification doesn't exist in the database", func() {
+				query := &models.GetAlertNotificationUidQuery{
+					Id:    -1,
+					OrgId: 100,
+				}
+
+				err := ss.GetAlertNotificationUidWithId(query)
+				So(query.Result, ShouldEqual, "")
+				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldEqual, "Alert notification [ Id: -1, OrgId: 100 ] not found")
+
+				cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id)
+				result, found := ss.CacheService.Get(cacheKey)
+				So(found, ShouldBeFalse)
+				So(result, ShouldBeNil)
+			})
+		})
 	})
 }

pkg/services/sqlstore/sqlstore.go

@@ -99,6 +99,7 @@ func (ss *SqlStore) Init() error {
 
 	// Register handlers
 	ss.addUserQueryAndCommandHandlers()
+	ss.addAlertNotificationUidByIdHandler()
 
 	if ss.skipEnsureDefaultOrgAndUser {
 		return nil