diff --git a/pkg/services/ngalert/database.go b/pkg/services/ngalert/database.go index bde570c4d5e..4cc55c8b17b 100644 --- a/pkg/services/ngalert/database.go +++ b/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 } diff --git a/pkg/services/ngalert/database_mig.go b/pkg/services/ngalert/database_mig.go index e7b868ba0d5..38478afc3aa 100644 --- a/pkg/services/ngalert/database_mig.go +++ b/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) { diff --git a/pkg/services/ngalert/database_test.go b/pkg/services/ngalert/database_test.go index 9b0bd54a17a..6016d1e800f 100644 --- a/pkg/services/ngalert/database_test.go +++ b/pkg/services/ngalert/database_test.go @@ -38,7 +38,8 @@ func TestCreatingAlertDefinition(t *testing.T) { inputTitle string expectedError error expectedInterval int64 - expectedUpdated time.Time + + expectedUpdated time.Time }{ { desc: "should create successfuly an alert definition with default interval", @@ -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", + UID: (*alertDefinition).UID, 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) diff --git a/pkg/services/ngalert/eval/alert_query.go b/pkg/services/ngalert/eval/alert_query.go index 818e8ba25d4..fb829952e66 100644 --- a/pkg/services/ngalert/eval/alert_query.go +++ b/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. diff --git a/pkg/services/ngalert/eval/alert_query_test.go b/pkg/services/ngalert/eval/alert_query_test.go index 191a342f8ad..12054b3b03f 100644 --- a/pkg/services/ngalert/eval/alert_query_test.go +++ b/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) diff --git a/pkg/services/ngalert/instance.go b/pkg/services/ngalert/instance.go index 69a5008d5a9..6c5f6380be7 100644 --- a/pkg/services/ngalert/instance.go +++ b/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, diff --git a/pkg/services/ngalert/models.go b/pkg/services/ngalert/models.go index ad54a0c5205..f3f5556a460 100644 --- a/pkg/services/ngalert/models.go +++ b/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 { diff --git a/pkg/services/ngalert/validator.go b/pkg/services/ngalert/validator.go index e4aaaf36acb..745e0816d90 100644 --- a/pkg/services/ngalert/validator.go +++ b/pkg/services/ngalert/validator.go @@ -1,6 +1,7 @@ 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 {