[Alerting]: Fix updating rule group and add tests (#33074)

* [Alerting]: Fix updating rule group and add test

* Fix updating rule labels
* Set default values for rule no data and error states
if they are missing
* Add test for updating rule

* Test updating annotations

* Apply suggestions from code review

Co-authored-by: gotjosh <josue@grafana.com>

* add test for posting an unknown rule UID

* Fix alert rule validation and add tests

* Remove org id from PostableGrafanaRule

This field was not used; each rule gets the organisation of the user making
the rerquest

* Update pkg/tests/api/alerting/api_alertmanager_test.go

Co-authored-by: gotjosh <josue@grafana.com>

pkg/services/ngalert/api/api_ruler.go

@@ -203,6 +203,8 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf
 	}); err != nil {
 		if errors.Is(err, ngmodels.ErrAlertRuleNotFound) {
 			return response.Error(http.StatusNotFound, "failed to update rule group", err)
+		} else if errors.Is(err, ngmodels.ErrAlertRuleFailedValidation) {
+			return response.Error(http.StatusBadRequest, "failed to update rule group", err)
 		}
 		return response.Error(http.StatusInternalServerError, "failed to update rule group", err)
 	}
@@ -240,7 +242,6 @@ func toGettableExtendedRuleNode(r ngmodels.AlertRule, namespaceID int64) apimode
 func toPostableExtendedRuleNode(r ngmodels.AlertRule) apimodels.PostableExtendedRuleNode {
 	postableExtendedRuleNode := apimodels.PostableExtendedRuleNode{
 		GrafanaManagedAlert: &apimodels.PostableGrafanaRule{
-			OrgID:        r.OrgID,
 			Title:        r.Title,
 			Condition:    r.Condition,
 			Data:         r.Data,

pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go

@@ -295,7 +295,6 @@ const (
 
 // swagger:model
 type PostableGrafanaRule struct {
-	OrgID        int64               `json:"-" yaml:"-"`
 	Title        string              `json:"title" yaml:"title"`
 	Condition    string              `json:"condition" yaml:"condition"`
 	Data         []models.AlertQuery `json:"data" yaml:"data"`

pkg/services/ngalert/models/alert_rule.go

@@ -15,6 +15,8 @@ var (
 	ErrCannotEditNamespace = errors.New("user does not have permissions to edit the namespace")
 	// ErrRuleGroupNamespaceNotFound
 	ErrRuleGroupNamespaceNotFound = errors.New("rule group not found under this namespace")
+	// ErrAlertRuleFailedValidation
+	ErrAlertRuleFailedValidation = errors.New("invalid alert rule")
 )
 
 type NoDataState string

pkg/services/ngalert/store/alert_rule.go

@@ -53,7 +53,6 @@ type RuleStore interface {
 	GetAlertInstance(*ngmodels.GetAlertInstanceQuery) error
 	ListAlertInstances(cmd *ngmodels.ListAlertInstancesQuery) error
 	SaveAlertInstance(cmd *ngmodels.SaveAlertInstanceCommand) error
-	ValidateAlertRule(ngmodels.AlertRule, bool) error
 }
 
 func getAlertRuleByUID(sess *sqlstore.DBSession, alertRuleUID string, orgID int64) (*ngmodels.AlertRule, error) {
@@ -188,7 +187,17 @@ func (st DBstore) UpsertAlertRules(rules []UpsertRule) error {
 
 				r.New.Version = 1
 
-				if err := st.ValidateAlertRule(r.New, true); err != nil {
+				if r.New.NoDataState == "" {
+					// set default no data state
+					r.New.NoDataState = ngmodels.NoData
+				}
+
+				if r.New.ExecErrState == "" {
+					// set default error state
+					r.New.ExecErrState = ngmodels.AlertingErrState
+				}
+
+				if err := st.validateAlertRule(r.New); err != nil {
 					return err
 				}
 
@@ -222,11 +231,19 @@ func (st DBstore) UpsertAlertRules(rules []UpsertRule) error {
 				r.New.RuleGroup = r.Existing.RuleGroup
 				r.New.Version = r.Existing.Version + 1
 
-				r.New.For = r.Existing.For
-				r.New.Annotations = r.Existing.Annotations
-				r.New.Labels = r.Existing.Labels
+				if r.New.For == 0 {
+					r.New.For = r.Existing.For
+				}
 
-				if err := st.ValidateAlertRule(r.New, true); err != nil {
+				if len(r.New.Annotations) == 0 {
+					r.New.Annotations = r.Existing.Annotations
+				}
+
+				if len(r.New.Labels) == 0 {
+					r.New.Labels = r.Existing.Labels
+				}
+
+				if err := st.validateAlertRule(r.New); err != nil {
 					return err
 				}
 
@@ -383,33 +400,32 @@ func generateNewAlertRuleUID(sess *sqlstore.DBSession, orgID int64) (string, err
 	return "", ngmodels.ErrAlertRuleFailedGenerateUniqueUID
 }
 
-// ValidateAlertRule validates the alert rule interval and organisation.
-// If requireData is true checks that it contains at least one alert query
-func (st DBstore) ValidateAlertRule(alertRule ngmodels.AlertRule, requireData bool) error {
-	if !requireData && len(alertRule.Data) == 0 {
-		return fmt.Errorf("no queries or expressions are found")
+// validateAlertRule validates the alert rule interval and organisation.
+func (st DBstore) validateAlertRule(alertRule ngmodels.AlertRule) error {
+	if len(alertRule.Data) == 0 {
+		return fmt.Errorf("%w: no queries or expressions are found", ngmodels.ErrAlertRuleFailedValidation)
 	}
 
 	if alertRule.Title == "" {
-		return ErrEmptyTitleError
+		return fmt.Errorf("%w: title is empty", ngmodels.ErrAlertRuleFailedValidation)
 	}
 
 	if alertRule.IntervalSeconds%int64(st.BaseInterval.Seconds()) != 0 {
-		return fmt.Errorf("invalid interval: %v: interval should be divided exactly by scheduler interval: %v", time.Duration(alertRule.IntervalSeconds)*time.Second, st.BaseInterval)
+		return fmt.Errorf("%w: interval (%v) should be divided exactly by scheduler interval: %v", ngmodels.ErrAlertRuleFailedValidation, time.Duration(alertRule.IntervalSeconds)*time.Second, st.BaseInterval)
 	}
 
 	// enfore max name length in SQLite
 	if len(alertRule.Title) > AlertRuleMaxTitleLength {
-		return fmt.Errorf("name length should not be greater than %d", AlertRuleMaxTitleLength)
+		return fmt.Errorf("%w: name length should not be greater than %d", ngmodels.ErrAlertRuleFailedValidation, AlertRuleMaxTitleLength)
 	}
 
-	// enfore max name length in SQLite
+	// enfore max rule group name length in SQLite
 	if len(alertRule.RuleGroup) > AlertRuleMaxRuleGroupNameLength {
-		return fmt.Errorf("name length should not be greater than %d", AlertRuleMaxRuleGroupNameLength)
+		return fmt.Errorf("%w: rule group name length should not be greater than %d", ngmodels.ErrAlertRuleFailedValidation, AlertRuleMaxRuleGroupNameLength)
 	}
 
 	if alertRule.OrgID == 0 {
-		return fmt.Errorf("no organisation is found")
+		return fmt.Errorf("%w: no organisation is found", ngmodels.ErrAlertRuleFailedValidation)
 	}
 
 	return nil

pkg/services/ngalert/store/database.go

@@ -19,9 +19,6 @@ var TimeNow = time.Now
 // AlertDefinitionMaxTitleLength is the maximum length of the alert definition title
 const AlertDefinitionMaxTitleLength = 190
 
-// ErrEmptyTitleError is an error returned if the alert definition title is empty
-var ErrEmptyTitleError = errors.New("title is empty")
-
 // Store is the interface for persisting alert definitions and instances
 type Store interface {
 	DeleteAlertDefinitionByUID(*models.DeleteAlertDefinitionByUIDCommand) error
@@ -329,7 +326,7 @@ func (st DBstore) ValidateAlertDefinition(alertDefinition *models.AlertDefinitio
 	}
 
 	if alertDefinition.Title == "" {
-		return ErrEmptyTitleError
+		return fmt.Errorf("title is empty")
 	}
 
 	if alertDefinition.IntervalSeconds%int64(st.BaseInterval.Seconds()) != 0 {

pkg/services/ngalert/tests/database_test.go

@@ -74,7 +74,7 @@ func TestCreatingAlertDefinition(t *testing.T) {
 			desc:                 "should fail to create an alert definition with empty title",
 			inputIntervalSeconds: &customIntervalSeconds,
 			inputTitle:           "",
-			expectedError:        store.ErrEmptyTitleError,
+			expectedError:        fmt.Errorf("title is empty"),
 		},
 	}
 

pkg/services/ngalert/tests/util.go

@@ -77,7 +77,6 @@ func createTestAlertRule(t *testing.T, dbstore *store.DBstore, intervalSeconds i
 			Rules: []apimodels.PostableExtendedRuleNode{
 				{
 					GrafanaManagedAlert: &apimodels.PostableGrafanaRule{
-						OrgID:     1,
 						Title:     fmt.Sprintf("an alert definition %d", d),
 						Condition: "A",
 						Data: []models.AlertQuery{
@@ -126,8 +125,7 @@ func updateTestAlertRuleIntervalSeconds(t *testing.T, dbstore *store.DBstore, ex
 			Rules: []apimodels.PostableExtendedRuleNode{
 				{
 					GrafanaManagedAlert: &apimodels.PostableGrafanaRule{
-						OrgID: 1,
-						UID:   existingRule.UID,
+						UID: existingRule.UID,
 					},
 				},
 			},