AlertingNG: Enforce unique alert definition title (non empty)/UID per organisation (#30380)

* Enforce unique alert definition title/uid per org

* Remove print statement from test

* Do not allow empty alert definition titles

* update error message on dup title

* also add title error to update

* CamelCase json properties

* Add test for title unique enforcement in updates

Co-authored-by: kyle <kyle@grafana.com>

pkg/services/ngalert/database.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/util"
@@ -91,6 +92,9 @@ func (ng *AlertNG) saveAlertDefinition(cmd *saveAlertDefinitionCommand) error {
 		}
 
 		if _, err := sess.Insert(alertDefinition); err != nil {
+			if ng.SQLStore.Dialect.IsUniqueConstraintViolation(err) && strings.Contains(err.Error(), "title") {
+				return fmt.Errorf("an alert definition with the title '%s' already exists: %w", cmd.Title, err)
+			}
 			return err
 		}
 
@@ -165,6 +169,9 @@ func (ng *AlertNG) updateAlertDefinition(cmd *updateAlertDefinitionCommand) erro
 
 		_, err = sess.ID(existingAlertDefinition.ID).Update(alertDefinition)
 		if err != nil {
+			if ng.SQLStore.Dialect.IsUniqueConstraintViolation(err) && strings.Contains(err.Error(), "title") {
+				return fmt.Errorf("an alert definition with the title '%s' already exists: %w", cmd.Title, err)
+			}
 			return err
 		}
 

pkg/services/ngalert/database_mig.go

@@ -36,6 +36,16 @@ func addAlertDefinitionMigrations(mg *migrator.Migrator) {
 
 	mg.AddMigration("alter alert_definition table data column to mediumtext in mysql", migrator.NewRawSQLMigration("").
 		Mysql("ALTER TABLE alert_definition MODIFY data MEDIUMTEXT;"))
+
+	mg.AddMigration("drop index in alert_definition on org_id and title columns", migrator.NewDropIndexMigration(alertDefinition, alertDefinition.Indices[0]))
+	mg.AddMigration("drop index in alert_definition on org_id and uid columns", migrator.NewDropIndexMigration(alertDefinition, alertDefinition.Indices[1]))
+
+	uniqueIndices := []*migrator.Index{
+		{Cols: []string{"org_id", "title"}, Type: migrator.UniqueIndex},
+		{Cols: []string{"org_id", "uid"}, Type: migrator.UniqueIndex},
+	}
+	mg.AddMigration("add unique index in alert_definition on org_id and title columns", migrator.NewAddIndexMigration(alertDefinition, uniqueIndices[0]))
+	mg.AddMigration("add unique index in alert_definition on org_id and uid columns", migrator.NewAddIndexMigration(alertDefinition, uniqueIndices[1]))
 }
 
 func addAlertDefinitionVersionMigrations(mg *migrator.Migrator) {

pkg/services/ngalert/database_test.go

@@ -38,6 +38,7 @@ func TestCreatingAlertDefinition(t *testing.T) {
 		inputTitle           string
 		expectedError        error
 		expectedInterval     int64
+
 		expectedUpdated time.Time
 	}{
 		{
@@ -57,9 +58,15 @@ func TestCreatingAlertDefinition(t *testing.T) {
 		{
 			desc:                 "should fail to create an alert definition with too big name",
 			inputIntervalSeconds: &customIntervalSeconds,
-			inputTitle:           getLongString(alertDefinitionMaxNameLength + 1),
+			inputTitle:           getLongString(alertDefinitionMaxTitleLength + 1),
 			expectedError:        errors.New(""),
 		},
+		{
+			desc:                 "should fail to create an alert definition with empty title",
+			inputIntervalSeconds: &customIntervalSeconds,
+			inputTitle:           "",
+			expectedError:        errEmptyTitleError,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -104,6 +111,41 @@ func TestCreatingAlertDefinition(t *testing.T) {
 		})
 	}
 }
+func TestCreatingConflictionAlertDefinition(t *testing.T) {
+	t.Run("Should fail to create alert definition with conflicting org_id, title", func(t *testing.T) {
+		ng := setupTestEnv(t)
+		t.Cleanup(registry.ClearOverrides)
+
+		q := saveAlertDefinitionCommand{
+			OrgID: 1,
+			Title: "title",
+			Condition: eval.Condition{
+				RefID: "B",
+				QueriesAndExpressions: []eval.AlertQuery{
+					{
+						Model: json.RawMessage(`{
+								"datasource": "__expr__",
+								"type":"math",
+								"expression":"2 + 3 > 1"
+							}`),
+						RefID: "B",
+						RelativeTimeRange: eval.RelativeTimeRange{
+							From: eval.Duration(time.Duration(5) * time.Hour),
+							To:   eval.Duration(time.Duration(3) * time.Hour),
+						},
+					},
+				},
+			},
+		}
+
+		err := ng.saveAlertDefinition(&q)
+		require.NoError(t, err)
+
+		err = ng.saveAlertDefinition(&q)
+		require.Error(t, err)
+		assert.True(t, ng.SQLStore.Dialect.IsUniqueConstraintViolation(err))
+	})
+}
 
 func TestUpdatingAlertDefinition(t *testing.T) {
 	t.Run("zero rows affected when updating unknown alert", func(t *testing.T) {
@@ -160,6 +202,7 @@ func TestUpdatingAlertDefinition(t *testing.T) {
 			expectedError           error
 			expectedIntervalSeconds int64
 			expectedUpdated         time.Time
+			expectedTitle           string
 		}{
 			{
 				desc:                    "should not update previous interval if it's not provided",
@@ -168,6 +211,7 @@ func TestUpdatingAlertDefinition(t *testing.T) {
 				inputTitle:              "something completely different",
 				expectedIntervalSeconds: initialInterval,
 				expectedUpdated:         time.Unix(1, 0).UTC(),
+				expectedTitle:           "something completely different",
 			},
 			{
 				desc:                    "should update interval if it's provided",
@@ -176,6 +220,7 @@ func TestUpdatingAlertDefinition(t *testing.T) {
 				inputTitle:              "something completely different",
 				expectedIntervalSeconds: customInterval,
 				expectedUpdated:         time.Unix(2, 0).UTC(),
+				expectedTitle:           "something completely different",
 			},
 			{
 				desc:                    "should not update organisation if it's provided",
@@ -184,19 +229,28 @@ func TestUpdatingAlertDefinition(t *testing.T) {
 				inputTitle:              "something completely different",
 				expectedIntervalSeconds: customInterval,
 				expectedUpdated:         time.Unix(3, 0).UTC(),
+				expectedTitle:           "something completely different",
 			},
 			{
-				desc:          "should not update alert definition if the name it's too big",
+				desc:          "should not update alert definition if the title it's too big",
 				inputInterval: &customInterval,
 				inputOrgID:    0,
-				inputTitle:    getLongString(alertDefinitionMaxNameLength + 1),
+				inputTitle:    getLongString(alertDefinitionMaxTitleLength + 1),
 				expectedError: errors.New(""),
 			},
+			{
+				desc:                    "should not update alert definition title if the title is empty",
+				inputInterval:           &customInterval,
+				inputOrgID:              0,
+				inputTitle:              "",
+				expectedIntervalSeconds: customInterval,
+				expectedUpdated:         time.Unix(4, 0).UTC(),
+				expectedTitle:           "something completely different",
+			},
 		}
 
 		q := updateAlertDefinitionCommand{
 			UID: (*alertDefinition).UID,
-			Title: "something completely different",
 			Condition: eval.Condition{
 				RefID: "B",
 				QueriesAndExpressions: []eval.AlertQuery{
@@ -268,6 +322,46 @@ func TestUpdatingAlertDefinition(t *testing.T) {
 	})
 }
 
+func TestUpdatingConflictingAlertDefinition(t *testing.T) {
+	t.Run("should fail to update alert definition with reserved title", func(t *testing.T) {
+		mockTimeNow()
+		defer resetTimeNow()
+
+		ng := setupTestEnv(t)
+		t.Cleanup(registry.ClearOverrides)
+
+		var initialInterval int64 = 120
+		alertDef1 := createTestAlertDefinition(t, ng, initialInterval)
+		alertDef2 := createTestAlertDefinition(t, ng, initialInterval)
+
+		q := updateAlertDefinitionCommand{
+			UID:   (*alertDef2).UID,
+			Title: alertDef1.Title,
+			Condition: eval.Condition{
+				RefID: "B",
+				QueriesAndExpressions: []eval.AlertQuery{
+					{
+						Model: json.RawMessage(`{
+							"datasource": "__expr__",
+							"type":"math",
+							"expression":"2 + 3 > 1"
+						}`),
+						RefID: "B",
+						RelativeTimeRange: eval.RelativeTimeRange{
+							From: eval.Duration(5 * time.Hour),
+							To:   eval.Duration(3 * time.Hour),
+						},
+					},
+				},
+			},
+		}
+
+		err := ng.updateAlertDefinition(&q)
+		require.Error(t, err)
+		assert.True(t, ng.SQLStore.Dialect.IsUniqueConstraintViolation(err))
+	})
+}
+
 func TestDeletingAlertDefinition(t *testing.T) {
 	t.Run("zero rows affected when deleting unknown alert", func(t *testing.T) {
 		ng := setupTestEnv(t)

pkg/services/ngalert/eval/alert_query.go

@@ -40,8 +40,8 @@ func (d *Duration) UnmarshalJSON(b []byte) error {
 // RelativeTimeRange is the per query start and end time
 // for requests.
 type RelativeTimeRange struct {
-	From Duration
-	To   Duration
+	From Duration `json:"from"`
+	To   Duration `json:"to"`
 }
 
 // isValid checks that From duration is greater than To duration.

pkg/services/ngalert/eval/alert_query_test.go

@@ -204,7 +204,6 @@ func TestAlertQuery(t *testing.T) {
 				err = json.Unmarshal(blob, &model)
 				require.NoError(t, err)
 
-				fmt.Printf(">>>>>>> %+v %+v\n", tc.alertQuery, model)
 				i, ok := model["datasource"]
 				require.True(t, ok)
 				datasource, ok := i.(string)

pkg/services/ngalert/instance.go

@@ -63,14 +63,14 @@ type listAlertInstancesQuery struct {
 
 // listAlertInstancesQueryResult represents the result of listAlertInstancesQuery.
 type listAlertInstancesQueryResult struct {
-	DefinitionOrgID   int64  `xorm:"def_org_id"`
-	DefinitionUID     string `xorm:"def_uid"`
-	DefinitionTitle   string `xorm:"def_title"`
-	Labels            InstanceLabels
-	LabelsHash        string
-	CurrentState      InstanceStateType
-	CurrentStateSince time.Time
-	LastEvalTime      time.Time
+	DefinitionOrgID   int64             `xorm:"def_org_id" json:"definitionOrgId"`
+	DefinitionUID     string            `xorm:"def_uid" json:"definitionUid"`
+	DefinitionTitle   string            `xorm:"def_title" json:"definitionTitle"`
+	Labels            InstanceLabels    `json:"labels"`
+	LabelsHash        string            `json:"labeHash"`
+	CurrentState      InstanceStateType `json:"currentState"`
+	CurrentStateSince time.Time         `json:"currentStateSince"`
+	LastEvalTime      time.Time         `json:"lastEvalTime"`
 }
 
 // validateAlertInstance validates that the alert instance contains an alert definition id,

pkg/services/ngalert/models.go

@@ -12,15 +12,15 @@ var errAlertDefinitionFailedGenerateUniqueUID = errors.New("failed to generate a
 
 // AlertDefinition is the model for alert definitions in Alerting NG.
 type AlertDefinition struct {
-	ID              int64 `xorm:"pk autoincr 'id'"`
-	OrgID           int64 `xorm:"org_id"`
-	Title           string
-	Condition       string
-	Data            []eval.AlertQuery
-	Updated         time.Time
-	IntervalSeconds int64
-	Version         int64
-	UID             string `xorm:"uid"`
+	ID              int64             `xorm:"pk autoincr 'id'" json:"id"`
+	OrgID           int64             `xorm:"org_id" json:"orgId"`
+	Title           string            `json:"title"`
+	Condition       string            `json:"condition"`
+	Data            []eval.AlertQuery `json:"data"`
+	Updated         time.Time         `json:"updated"`
+	IntervalSeconds int64             `json:"intervalSeconds"`
+	Version         int64             `json:"version"`
+	UID             string            `xorm:"uid" json:"uid"`
 }
 
 type alertDefinitionKey struct {

pkg/services/ngalert/validator.go

 package ngalert
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
@@ -8,7 +9,9 @@ import (
 	"github.com/grafana/grafana/pkg/services/ngalert/eval"
 )
 
-const alertDefinitionMaxNameLength = 190
+const alertDefinitionMaxTitleLength = 190
+
+var errEmptyTitleError = errors.New("title is empty")
 
 // validateAlertDefinition validates the alert definition interval and organisation.
 // If requireData is true checks that it contains at least one alert query
@@ -17,13 +20,17 @@ func (ng *AlertNG) validateAlertDefinition(alertDefinition *AlertDefinition, req
 		return fmt.Errorf("no queries or expressions are found")
 	}
 
+	if alertDefinition.Title == "" {
+		return errEmptyTitleError
+	}
+
 	if alertDefinition.IntervalSeconds%int64(ng.schedule.baseInterval.Seconds()) != 0 {
 		return fmt.Errorf("invalid interval: %v: interval should be divided exactly by scheduler interval: %v", time.Duration(alertDefinition.IntervalSeconds)*time.Second, ng.schedule.baseInterval)
 	}
 
 	// enfore max name length in SQLite
-	if len(alertDefinition.Title) > alertDefinitionMaxNameLength {
-		return fmt.Errorf("name length should not be greater than %d", alertDefinitionMaxNameLength)
+	if len(alertDefinition.Title) > alertDefinitionMaxTitleLength {
+		return fmt.Errorf("name length should not be greater than %d", alertDefinitionMaxTitleLength)
 	}
 
 	if alertDefinition.OrgID == 0 {