Alerting: Do not update rule in database if it was not changed (#45980)

* do not include update if no diff
* refactor calculate changes to include diff (and log)

Co-authored-by: George Robinson <george.robinson@grafana.com>

pkg/services/ngalert/api/api_ruler.go

@@ -11,6 +11,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
 	"github.com/grafana/grafana/pkg/services/quota"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/util/cmputil"
 
 	"github.com/prometheus/common/model"
 
@@ -261,27 +262,48 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf
 func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *models.Folder, groupName string, rules []*ngmodels.AlertRule) response.Response {
 	// TODO add create rules authz logic
 
-	var changes *RuleChanges = nil
+	var groupChanges *changes = nil
 	err := srv.store.InTransaction(c.Req.Context(), func(tranCtx context.Context) error {
 		var err error
-		changes, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules)
+		groupChanges, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules)
 		if err != nil {
 			return err
 		}
 
-		// TODO add update/delete authz logic
-		err = srv.store.UpsertAlertRules(tranCtx, changes.Upsert)
-		if err != nil {
-			return fmt.Errorf("failed to add or update rules: %w", err)
+		if groupChanges.isEmpty() {
+			srv.log.Info("no changes detected in the request. Do nothing")
+			return nil
 		}
 
-		for _, rule := range changes.Delete {
+		if len(groupChanges.Update) > 0 || len(groupChanges.New) > 0 {
+			upsert := make([]store.UpsertRule, 0, len(groupChanges.Update)+len(groupChanges.New))
+			for _, update := range groupChanges.Update {
+				srv.log.Debug("updating rule", "uid", update.New.UID, "diff", update.Diff.String())
+				upsert = append(upsert, store.UpsertRule{
+					Existing: update.Existing,
+					New:      *update.New,
+				})
+			}
+			for _, rule := range groupChanges.New {
+				upsert = append(upsert, store.UpsertRule{
+					Existing: nil,
+					New:      *rule,
+				})
+			}
+			// TODO add update/delete authz logic
+			err = srv.store.UpsertAlertRules(tranCtx, upsert)
+			if err != nil {
+				return fmt.Errorf("failed to add or update rules: %w", err)
+			}
+		}
+
+		for _, rule := range groupChanges.Delete {
 			if err = srv.store.DeleteAlertRuleByUID(tranCtx, c.SignedInUser.OrgId, rule.UID); err != nil {
 				return fmt.Errorf("failed to delete rule %d with UID %s: %w", rule.ID, rule.UID, err)
 			}
 		}
 
-		if changes.newRules > 0 {
+		if len(groupChanges.New) > 0 {
 			limitReached, err := srv.QuotaService.CheckQuotaReached(tranCtx, "alert_rule", &quota.ScopeParameters{
 				OrgId:  c.OrgId,
 				UserId: c.UserId,
@@ -307,23 +329,24 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod
 		return ErrResp(http.StatusInternalServerError, err, "failed to update rule group")
 	}
 
-	// TODO uncomment when rules that are not changed will be filter out from the upsert list.
-	// for _, rule := range changes.Upsert {
-	// 	if rule.Existing != nil {
-	// 		srv.scheduleService.UpdateAlertRule(ngmodels.AlertRuleKey{
-	// 			OrgID: c.SignedInUser.OrgId,
-	// 			UID:   rule.Existing.UID,
-	// 		})
-	// 	}
-	// }
+	for _, rule := range groupChanges.Update {
+		srv.scheduleService.UpdateAlertRule(ngmodels.AlertRuleKey{
+			OrgID: c.SignedInUser.OrgId,
+			UID:   rule.Existing.UID,
+		})
+	}
 
-	for _, rule := range changes.Delete {
+	for _, rule := range groupChanges.Delete {
 		srv.scheduleService.DeleteAlertRule(ngmodels.AlertRuleKey{
 			OrgID: c.SignedInUser.OrgId,
 			UID:   rule.UID,
 		})
 	}
 
+	if groupChanges.isEmpty() {
+		return response.JSON(http.StatusAccepted, util.DynMap{"message": "no changes detected in the rule group"})
+	}
+
 	return response.JSON(http.StatusAccepted, util.DynMap{"message": "rule group updated successfully"})
 }
 
@@ -364,15 +387,25 @@ func toNamespaceErrorResponse(err error) response.Response {
 	return apierrors.ToFolderErrorResponse(err)
 }
 
-type RuleChanges struct {
-	newRules int
-	Upsert   []store.UpsertRule
-	Delete   []*ngmodels.AlertRule
+type ruleUpdate struct {
+	Existing *ngmodels.AlertRule
+	New      *ngmodels.AlertRule
+	Diff     cmputil.DiffReport
+}
+
+type changes struct {
+	New    []*ngmodels.AlertRule
+	Update []ruleUpdate
+	Delete []*ngmodels.AlertRule
+}
+
+func (c *changes) isEmpty() bool {
+	return len(c.Update)+len(c.New)+len(c.Delete) == 0
 }
 
 // calculateChanges calculates the difference between rules in the group in the database and the submitted rules. If a submitted rule has UID it tries to find it in the database (in other groups).
 // returns a list of rules that need to be added, updated and deleted. Deleted considered rules in the database that belong to the group but do not exist in the list of submitted rules.
-func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int64, namespace *models.Folder, ruleGroupName string, submittedRules []*ngmodels.AlertRule) (*RuleChanges, error) {
+func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int64, namespace *models.Folder, ruleGroupName string, submittedRules []*ngmodels.AlertRule) (*changes, error) {
 	q := &ngmodels.ListRuleGroupAlertRulesQuery{
 		OrgID:        orgId,
 		NamespaceUID: namespace.Uid,
@@ -388,9 +421,8 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6
 		existingGroupRulesUIDs[r.UID] = r
 	}
 
-	upsert := make([]store.UpsertRule, 0, len(submittedRules))
-	toDelete := make([]*ngmodels.AlertRule, 0, len(submittedRules))
-	newRules := 0
+	var toAdd, toDelete []*ngmodels.AlertRule
+	var toUpdate []ruleUpdate
 	for _, r := range submittedRules {
 		var existing *ngmodels.AlertRule = nil
 
@@ -414,19 +446,21 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6
 		}
 
 		if existing == nil {
-			upsert = append(upsert, store.UpsertRule{
-				Existing: nil,
-				New:      *r,
-			})
-			newRules++
+			toAdd = append(toAdd, r)
 			continue
 		}
 
 		ngmodels.PatchPartialAlertRule(existing, r)
-		// TODO diff between patched and existing, as well as between submitted
-		upsert = append(upsert, store.UpsertRule{
+
+		diff := existing.Diff(r, alertRuleFieldsToIgnoreInDiff...)
+		if len(diff) == 0 {
+			continue
+		}
+
+		toUpdate = append(toUpdate, ruleUpdate{
 			Existing: existing,
-			New:      *r,
+			New:      r,
+			Diff:     diff,
 		})
 		continue
 	}
@@ -435,9 +469,12 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6
 		toDelete = append(toDelete, rule)
 	}
 
-	return &RuleChanges{
-		Upsert:   upsert,
-		Delete:   toDelete,
-		newRules: newRules,
+	return &changes{
+		New:    toAdd,
+		Delete: toDelete,
+		Update: toUpdate,
 	}, nil
 }
+
+// alertRuleFieldsToIgnoreInDiff contains fields that the AlertRule.Diff should ignore
+var alertRuleFieldsToIgnoreInDiff = []string{"ID", "Version", "Updated"}

pkg/services/ngalert/api/api_ruler_test.go

@@ -7,7 +7,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
 
 	models2 "github.com/grafana/grafana/pkg/models"
@@ -29,23 +28,14 @@ func TestCalculateChanges(t *testing.T) {
 		changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted)
 		require.NoError(t, err)
 
-		require.Equal(t, changes.newRules, len(submitted))
+		require.Len(t, changes.New, len(submitted))
 		require.Empty(t, changes.Delete)
-		require.Len(t, changes.Upsert, len(submitted))
-		for _, rule := range changes.Upsert {
-			require.Nil(t, rule.Existing)
-		}
-
-		opts := []cmp.Option{
-			cmp.FilterPath(func(path cmp.Path) bool {
-				return path.String() == "Data.modelProps"
-			}, cmp.Ignore()),
-		}
+		require.Empty(t, changes.Update)
 
 	outerloop:
 		for _, expected := range submitted {
-			for _, rule := range changes.Upsert {
-				if cmp.Equal(*expected, rule.New, opts...) {
+			for _, rule := range changes.New {
+				if len(expected.Diff(rule)) == 0 {
 					continue outerloop
 				}
 			}
@@ -64,8 +54,8 @@ func TestCalculateChanges(t *testing.T) {
 		changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, make([]*models.AlertRule, 0))
 		require.NoError(t, err)
 
-		require.Equal(t, 0, changes.newRules)
-		require.Len(t, changes.Upsert, 0)
+		require.Empty(t, changes.New)
+		require.Empty(t, changes.Update)
 		require.Len(t, changes.Delete, len(inDatabaseMap))
 		for _, toDelete := range changes.Delete {
 			require.Contains(t, inDatabaseMap, toDelete.UID)
@@ -86,15 +76,44 @@ func TestCalculateChanges(t *testing.T) {
 		changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted)
 		require.NoError(t, err)
 
-		require.Len(t, changes.Upsert, len(inDatabase))
-		for _, upsert := range changes.Upsert {
+		require.Len(t, changes.Update, len(inDatabase))
+		for _, upsert := range changes.Update {
 			require.NotNil(t, upsert.Existing)
 			require.Equal(t, upsert.Existing.UID, upsert.New.UID)
 			require.Equal(t, inDatabaseMap[upsert.Existing.UID], upsert.Existing)
-			require.Equal(t, *submittedMap[upsert.Existing.UID], upsert.New)
+			require.Equal(t, submittedMap[upsert.Existing.UID], upsert.New)
+			require.NotEmpty(t, upsert.Diff)
+		}
+		require.Empty(t, changes.Delete)
+		require.Empty(t, changes.New)
+	})
+
+	t.Run("should include only if there are changes ignoring specific fields", func(t *testing.T) {
+		namespace := randFolder()
+		groupName := util.GenerateShortUID()
+		_, inDatabase := models.GenerateUniqueAlertRules(rand.Intn(5)+1, models.AlertRuleGen(withOrgID(orgId), withGroup(groupName), withNamespace(namespace)))
+
+		submitted := make([]*models.AlertRule, 0, len(inDatabase))
+		for _, rule := range inDatabase {
+			r := models.CopyRule(rule)
+
+			// Ignore difference in the following fields as submitted models do not have them set
+			r.ID = rand.Int63()
+			r.Version = rand.Int63()
+			r.Updated = r.Updated.Add(1 * time.Minute)
+
+			submitted = append(submitted, r)
 		}
-		require.Len(t, changes.Delete, 0)
-		require.Equal(t, 0, changes.newRules)
+
+		fakeStore := store.NewFakeRuleStore(t)
+		fakeStore.PutRule(context.Background(), inDatabase...)
+
+		changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted)
+		require.NoError(t, err)
+
+		require.Empty(t, changes.Update)
+		require.Empty(t, changes.Delete)
+		require.Empty(t, changes.New)
 	})
 
 	t.Run("should patch rule with UID specified by existing rule", func(t *testing.T) {
@@ -150,12 +169,12 @@ func TestCalculateChanges(t *testing.T) {
 				submitted := *expected
 				changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, []*models.AlertRule{&submitted})
 				require.NoError(t, err)
-				require.Len(t, changes.Upsert, 1)
-				ch := changes.Upsert[0]
+				require.Len(t, changes.Update, 1)
+				ch := changes.Update[0]
 				require.Equal(t, ch.Existing, dbRule)
 				fixed := *expected
 				models.PatchPartialAlertRule(dbRule, &fixed)
-				require.Equal(t, fixed, ch.New)
+				require.Equal(t, fixed, *ch.New)
 			})
 		}
 	})
@@ -173,14 +192,15 @@ func TestCalculateChanges(t *testing.T) {
 		changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted)
 		require.NoError(t, err)
 
-		require.Len(t, changes.Delete, 0)
-		require.Equal(t, 0, changes.newRules)
-		require.Len(t, changes.Upsert, len(submitted))
-		for _, upsert := range changes.Upsert {
-			require.NotNil(t, upsert.Existing)
-			require.Equal(t, upsert.Existing.UID, upsert.New.UID)
-			require.Equal(t, inDatabaseMap[upsert.Existing.UID], upsert.Existing)
-			require.Equal(t, *submittedMap[upsert.Existing.UID], upsert.New)
+		require.Empty(t, changes.Delete)
+		require.Empty(t, changes.New)
+		require.Len(t, changes.Update, len(submitted))
+		for _, update := range changes.Update {
+			require.NotNil(t, update.Existing)
+			require.Equal(t, update.Existing.UID, update.New.UID)
+			require.Equal(t, inDatabaseMap[update.Existing.UID], update.Existing)
+			require.Equal(t, submittedMap[update.Existing.UID], update.New)
+			require.NotEmpty(t, update.Diff)
 		}
 	})
 

pkg/tests/api/alerting/api_alertmanager_test.go

@@ -1787,7 +1787,7 @@ func TestAlertRuleCRUD(t *testing.T) {
 			}`, body)
 	}
 
-	// update the rule; keep title, condition, no data state, error state, queries and expressions if not provided
+	// update the rule; keep title, condition, no data state, error state, queries and expressions if not provided. should be noop
 	{
 		rules := apimodels.PostableRuleGroupConfig{
 			Name: "arulegroup",
@@ -1817,7 +1817,7 @@ func TestAlertRuleCRUD(t *testing.T) {
 		require.NoError(t, err)
 
 		assert.Equal(t, resp.StatusCode, 202)
-		require.JSONEq(t, `{"message":"rule group updated successfully"}`, string(b))
+		require.JSONEq(t, `{"message":"no changes detected in the rule group"}`, string(b))
 
 		// let's make sure that rule definitions are updated correctly.
 		u = fmt.Sprintf("http://grafana:password@%s/api/ruler/grafana/api/v1/rules/default", grafanaListedAddr)
@@ -1872,7 +1872,7 @@ func TestAlertRuleCRUD(t *testing.T) {
 					  ],
 					  "updated":"2021-02-21T01:10:30Z",
 					  "intervalSeconds":60,
-					  "version":4,
+					  "version":3,
 					  "uid":"uid",
 					  "namespace_uid":"nsuid",
 					  "namespace_id":1,