diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index df2897208d7..fa6d6cde63f 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -350,29 +350,7 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey finalChanges = store.UpdateCalculatedRuleFields(groupChanges) logger.Debug("updating database with the authorized changes", "add", len(finalChanges.New), "update", len(finalChanges.New), "delete", len(finalChanges.Delete)) - if len(finalChanges.Update) > 0 || len(finalChanges.New) > 0 { - updates := make([]ngmodels.UpdateRule, 0, len(finalChanges.Update)) - inserts := make([]ngmodels.AlertRule, 0, len(finalChanges.New)) - for _, update := range finalChanges.Update { - logger.Debug("updating rule", "rule_uid", update.New.UID, "diff", update.Diff.String()) - updates = append(updates, ngmodels.UpdateRule{ - Existing: update.Existing, - New: *update.New, - }) - } - for _, rule := range finalChanges.New { - inserts = append(inserts, *rule) - } - _, err = srv.store.InsertAlertRules(tranCtx, inserts) - if err != nil { - return fmt.Errorf("failed to add rules: %w", err) - } - err = srv.store.UpdateAlertRules(tranCtx, updates) - if err != nil { - return fmt.Errorf("failed to update rules: %w", err) - } - } - + // Delete first as this could prevent future unique constraint violations. if len(finalChanges.Delete) > 0 { UIDs := make([]string, 0, len(finalChanges.Delete)) for _, rule := range finalChanges.Delete { @@ -384,6 +362,32 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey } } + if len(finalChanges.Update) > 0 { + updates := make([]ngmodels.UpdateRule, 0, len(finalChanges.Update)) + for _, update := range finalChanges.Update { + logger.Debug("updating rule", "rule_uid", update.New.UID, "diff", update.Diff.String()) + updates = append(updates, ngmodels.UpdateRule{ + Existing: update.Existing, + New: *update.New, + }) + } + err = srv.store.UpdateAlertRules(tranCtx, updates) + if err != nil { + return fmt.Errorf("failed to update rules: %w", err) + } + } + + if len(finalChanges.New) > 0 { + inserts := make([]ngmodels.AlertRule, 0, len(finalChanges.New)) + for _, rule := range finalChanges.New { + inserts = append(inserts, *rule) + } + _, err = srv.store.InsertAlertRules(tranCtx, inserts) + if err != nil { + return fmt.Errorf("failed to add rules: %w", err) + } + } + if len(finalChanges.New) > 0 { limitReached, err := srv.QuotaService.CheckQuotaReached(tranCtx, ngmodels.QuotaTargetSrv, "a.ScopeParameters{ OrgID: c.OrgID, diff --git a/pkg/services/ngalert/provisioning/alert_rules.go b/pkg/services/ngalert/provisioning/alert_rules.go index b673be172f0..049035f584d 100644 --- a/pkg/services/ngalert/provisioning/alert_rules.go +++ b/pkg/services/ngalert/provisioning/alert_rules.go @@ -260,53 +260,60 @@ func (service *AlertRuleService) ReplaceRuleGroup(ctx context.Context, orgID int } return service.xact.InTransaction(ctx, func(ctx context.Context) error { - uids, err := service.ruleStore.InsertAlertRules(ctx, withoutNilAlertRules(delta.New)) - if err != nil { - return fmt.Errorf("failed to insert alert rules: %w", err) - } - for uid := range uids { - if err := service.provenanceStore.SetProvenance(ctx, &models.AlertRule{UID: uid}, orgID, provenance); err != nil { + // Delete first as this could prevent future unique constraint violations. + if len(delta.Delete) > 0 { + for _, del := range delta.Delete { + // check that provenance is not changed in an invalid way + storedProvenance, err := service.provenanceStore.GetProvenance(ctx, del, orgID) + if err != nil { + return err + } + if canUpdate := canUpdateProvenanceInRuleGroup(storedProvenance, provenance); !canUpdate { + return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance) + } + } + if err := service.deleteRules(ctx, orgID, delta.Delete...); err != nil { return err } } - updates := make([]models.UpdateRule, 0, len(delta.Update)) - for _, update := range delta.Update { - // check that provenance is not changed in an invalid way - storedProvenance, err := service.provenanceStore.GetProvenance(ctx, update.New, orgID) - if err != nil { - return err + if len(delta.Update) > 0 { + updates := make([]models.UpdateRule, 0, len(delta.Update)) + for _, update := range delta.Update { + // check that provenance is not changed in an invalid way + storedProvenance, err := service.provenanceStore.GetProvenance(ctx, update.New, orgID) + if err != nil { + return err + } + if canUpdate := canUpdateProvenanceInRuleGroup(storedProvenance, provenance); !canUpdate { + return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance) + } + updates = append(updates, models.UpdateRule{ + Existing: update.Existing, + New: *update.New, + }) } - if canUpdate := canUpdateProvenanceInRuleGroup(storedProvenance, provenance); !canUpdate { - return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance) + if err = service.ruleStore.UpdateAlertRules(ctx, updates); err != nil { + return fmt.Errorf("failed to update alert rules: %w", err) } - updates = append(updates, models.UpdateRule{ - Existing: update.Existing, - New: *update.New, - }) - } - if err = service.ruleStore.UpdateAlertRules(ctx, updates); err != nil { - return fmt.Errorf("failed to update alert rules: %w", err) - } - for _, update := range delta.Update { - if err := service.provenanceStore.SetProvenance(ctx, update.New, orgID, provenance); err != nil { - return err + for _, update := range delta.Update { + if err := service.provenanceStore.SetProvenance(ctx, update.New, orgID, provenance); err != nil { + return err + } } } - for _, delete := range delta.Delete { - // check that provenance is not changed in an invalid way - storedProvenance, err := service.provenanceStore.GetProvenance(ctx, delete, orgID) + if len(delta.New) > 0 { + uids, err := service.ruleStore.InsertAlertRules(ctx, withoutNilAlertRules(delta.New)) if err != nil { - return err + return fmt.Errorf("failed to insert alert rules: %w", err) } - if canUpdate := canUpdateProvenanceInRuleGroup(storedProvenance, provenance); !canUpdate { - return fmt.Errorf("cannot update with provided provenance '%s', needs '%s'", provenance, storedProvenance) + for uid := range uids { + if err := service.provenanceStore.SetProvenance(ctx, &models.AlertRule{UID: uid}, orgID, provenance); err != nil { + return err + } } } - if err := service.deleteRules(ctx, orgID, delta.Delete...); err != nil { - return err - } if err = service.checkLimitsTransactionCtx(ctx, orgID, userID); err != nil { return err diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index c3ac5fd3480..67f8d731512 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -160,6 +160,210 @@ func TestAlertRuleService(t *testing.T) { require.Equal(t, int64(2), readGroup.Rules[0].Version) }) + t.Run("updating a group to temporarily overlap rule names should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "overlap-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("overlap-test-rule-1", orgID), + dummyRule("overlap-test-rule-2", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "overlap-test") + require.NoError(t, err) + + updatedGroup.Rules[0].Title = "overlap-test-rule-2" + updatedGroup.Rules[1].Title = "overlap-test-rule-3" + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "overlap-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 2) + require.Equal(t, "overlap-test-rule-2", readGroup.Rules[0].Title) + require.Equal(t, "overlap-test-rule-3", readGroup.Rules[1].Title) + require.Equal(t, int64(3), readGroup.Rules[0].Version) + require.Equal(t, int64(3), readGroup.Rules[1].Version) + }) + + t.Run("updating a group to swap the name of two rules should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "swap-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("swap-test-rule-1", orgID), + dummyRule("swap-test-rule-2", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "swap-test") + require.NoError(t, err) + + updatedGroup.Rules[0].Title = "swap-test-rule-2" + updatedGroup.Rules[1].Title = "swap-test-rule-1" + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "swap-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 2) + require.Equal(t, "swap-test-rule-2", readGroup.Rules[0].Title) + require.Equal(t, "swap-test-rule-1", readGroup.Rules[1].Title) + require.Equal(t, int64(3), readGroup.Rules[0].Version) // Needed an extra update to break the update cycle. + require.Equal(t, int64(3), readGroup.Rules[1].Version) + }) + + t.Run("updating a group that has a rule name cycle should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "cycle-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("cycle-test-rule-1", orgID), + dummyRule("cycle-test-rule-2", orgID), + dummyRule("cycle-test-rule-3", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "cycle-test") + require.NoError(t, err) + + updatedGroup.Rules[0].Title = "cycle-test-rule-2" + updatedGroup.Rules[1].Title = "cycle-test-rule-3" + updatedGroup.Rules[2].Title = "cycle-test-rule-1" + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "cycle-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 3) + require.Equal(t, "cycle-test-rule-2", readGroup.Rules[0].Title) + require.Equal(t, "cycle-test-rule-3", readGroup.Rules[1].Title) + require.Equal(t, "cycle-test-rule-1", readGroup.Rules[2].Title) + require.Equal(t, int64(3), readGroup.Rules[0].Version) // Needed an extra update to break the update cycle. + require.Equal(t, int64(3), readGroup.Rules[1].Version) + require.Equal(t, int64(3), readGroup.Rules[2].Version) + }) + + t.Run("updating a group that has multiple rule name cycles should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "multi-cycle-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("multi-cycle-test-rule-1", orgID), + dummyRule("multi-cycle-test-rule-2", orgID), + + dummyRule("multi-cycle-test-rule-3", orgID), + dummyRule("multi-cycle-test-rule-4", orgID), + dummyRule("multi-cycle-test-rule-5", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "multi-cycle-test") + require.NoError(t, err) + + updatedGroup.Rules[0].Title = "multi-cycle-test-rule-2" + updatedGroup.Rules[1].Title = "multi-cycle-test-rule-1" + + updatedGroup.Rules[2].Title = "multi-cycle-test-rule-4" + updatedGroup.Rules[3].Title = "multi-cycle-test-rule-5" + updatedGroup.Rules[4].Title = "multi-cycle-test-rule-3" + + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "multi-cycle-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 5) + require.Equal(t, "multi-cycle-test-rule-2", readGroup.Rules[0].Title) + require.Equal(t, "multi-cycle-test-rule-1", readGroup.Rules[1].Title) + require.Equal(t, "multi-cycle-test-rule-4", readGroup.Rules[2].Title) + require.Equal(t, "multi-cycle-test-rule-5", readGroup.Rules[3].Title) + require.Equal(t, "multi-cycle-test-rule-3", readGroup.Rules[4].Title) + require.Equal(t, int64(3), readGroup.Rules[0].Version) // Needed an extra update to break the update cycle. + require.Equal(t, int64(3), readGroup.Rules[1].Version) + require.Equal(t, int64(3), readGroup.Rules[2].Version) // Needed an extra update to break the update cycle. + require.Equal(t, int64(3), readGroup.Rules[3].Version) + require.Equal(t, int64(3), readGroup.Rules[4].Version) + }) + + t.Run("updating a group to recreate a rule using the same name should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "recreate-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("recreate-test-rule-1", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup := models.AlertRuleGroup{ + Title: "recreate-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("recreate-test-rule-1", orgID), + }, + } + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "recreate-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 1) + require.Equal(t, "recreate-test-rule-1", readGroup.Rules[0].Title) + require.Equal(t, int64(1), readGroup.Rules[0].Version) + }) + + t.Run("updating a group to create a rule that temporarily overlaps an existing should not throw unique constraint", func(t *testing.T) { + var orgID int64 = 1 + group := models.AlertRuleGroup{ + Title: "create-overlap-test", + Interval: 60, + FolderUID: "my-namespace", + Rules: []models.AlertRule{ + dummyRule("create-overlap-test-rule-1", orgID), + }, + } + err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) + require.NoError(t, err) + updatedGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "create-overlap-test") + require.NoError(t, err) + updatedGroup.Rules[0].Title = "create-overlap-test-rule-2" + updatedGroup.Rules = append(updatedGroup.Rules, dummyRule("create-overlap-test-rule-1", orgID)) + + err = ruleService.ReplaceRuleGroup(context.Background(), orgID, updatedGroup, 0, models.ProvenanceAPI) + require.NoError(t, err) + + readGroup, err := ruleService.GetRuleGroup(context.Background(), orgID, "my-namespace", "create-overlap-test") + require.NoError(t, err) + require.NotEmpty(t, readGroup.Rules) + require.Len(t, readGroup.Rules, 2) + require.Equal(t, "create-overlap-test-rule-2", readGroup.Rules[0].Title) + require.Equal(t, "create-overlap-test-rule-1", readGroup.Rules[1].Title) + require.Equal(t, int64(2), readGroup.Rules[0].Version) + require.Equal(t, int64(1), readGroup.Rules[1].Version) + }) + t.Run("updating a group by updating a rule should not remove dashboard and panel ids", func(t *testing.T) { dashboardUid := "huYnkl7H" panelId := int64(5678) diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 7e6f080aa42..78f468a1ca6 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -6,6 +6,8 @@ import ( "fmt" "strings" + "github.com/google/uuid" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" @@ -180,6 +182,11 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu // UpdateAlertRules is a handler for updating alert rules. func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateRule) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + err := st.preventIntermediateUniqueConstraintViolations(sess, rules) + if err != nil { + return fmt.Errorf("failed when preventing intermediate unique constraint violation: %w", err) + } + ruleVersions := make([]ngmodels.AlertRuleVersion, 0, len(rules)) for _, r := range rules { var parentVersion int64 @@ -231,6 +238,77 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateR }) } +// preventIntermediateUniqueConstraintViolations prevents unique constraint violations caused by an intermediate update. +// The uniqueness constraint for titles within an org+folder is enforced on every update within a transaction +// instead of on commit (deferred constraint). This means that there could be a set of updates that will throw +// a unique constraint violation in an intermediate step even though the final state is valid. +// For example, a chain of updates RuleA -> RuleB -> RuleC could fail if not executed in the correct order, or +// a swap of titles RuleA <-> RuleB cannot be executed in any order without violating the constraint. +func (st DBstore) preventIntermediateUniqueConstraintViolations(sess *db.Session, updates []ngmodels.UpdateRule) error { + // The exact solution to this is complex and requires determining directed paths and cycles in the update graph, + // adding in temporary updates to break cycles, and then executing the updates in reverse topological order. + // This is not implemented here. Instead, we choose a simpler solution that works in all cases but might perform + // more updates than necessary. This simpler solution makes a determination of whether an intermediate collision + // could occur and if so, adds a temporary title on all updated rules to break any cycles and remove the need for + // specific ordering. + + titleUpdates := make([]ngmodels.UpdateRule, 0) + for _, update := range updates { + if update.Existing.Title != update.New.Title { + titleUpdates = append(titleUpdates, update) + } + } + + // If there is no overlap then an intermediate unique constraint violation is not possible. If there is an overlap, + // then there is the possibility of intermediate unique constraint violation. + if !newTitlesOverlapExisting(titleUpdates) { + return nil + } + st.Logger.Debug("detected possible intermediate unique constraint violation, creating temporary title updates", "updates", len(titleUpdates)) + + for _, update := range titleUpdates { + r := update.Existing + u := uuid.New().String() + + // Some defensive programming in case the temporary title is somehow persisted it will still be recognizable. + uniqueTempTitle := r.Title + u + if len(uniqueTempTitle) > AlertRuleMaxTitleLength { + uniqueTempTitle = r.Title[:AlertRuleMaxTitleLength-len(u)] + uuid.New().String() + } + + if updated, err := sess.ID(r.ID).Cols("title").Update(&ngmodels.AlertRule{Title: uniqueTempTitle, Version: r.Version}); err != nil || updated == 0 { + if err != nil { + return fmt.Errorf("failed to set temporary rule title [%s] %s: %w", r.UID, r.Title, err) + } + return fmt.Errorf("%w: alert rule UID %s version %d", ErrOptimisticLock, r.UID, r.Version) + } + // Otherwise optimistic locking will conflict on the 2nd update. + r.Version++ + // For consistency. + r.Title = uniqueTempTitle + } + + return nil +} + +// newTitlesOverlapExisting returns true if any new titles overlap with existing titles. +// It does so in a case-insensitive manner as some supported databases perform case-insensitive comparisons. +func newTitlesOverlapExisting(rules []ngmodels.UpdateRule) bool { + existingTitles := make(map[string]struct{}, len(rules)) + for _, r := range rules { + existingTitles[strings.ToLower(r.Existing.Title)] = struct{}{} + } + + // Check if there is any overlap between lower case existing and new titles. + for _, r := range rules { + if _, ok := existingTitles[strings.ToLower(r.New.Title)]; ok { + return true + } + } + + return false +} + // CountInFolder is a handler for retrieving the number of alert rules of // specific organisation associated with a given namespace (parent folder). func (st DBstore) CountInFolder(ctx context.Context, orgID int64, folderUID string, u *user.SignedInUser) (int64, error) { diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 87e77bf731e..493d267cce6 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -4,11 +4,13 @@ import ( "context" "errors" "fmt" + "strings" "testing" "time" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/log/logtest" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/folderimpl" @@ -37,6 +39,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { SQLStore: sqlStore, Cfg: cfg.UnifiedAlerting, FolderService: setupFolderService(t, sqlStore, cfg), + Logger: &logtest.Fake{}, } generator := models.AlertRuleGen(withIntervalMatching(store.Cfg.BaseInterval), models.WithUniqueID()) @@ -79,6 +82,236 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { }) } +func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + cfg := setting.NewCfg() + cfg.UnifiedAlerting = setting.UnifiedAlertingSettings{BaseInterval: time.Duration(rand.Int63n(100)+1) * time.Second} + sqlStore := db.InitTestDB(t) + store := &DBstore{ + SQLStore: sqlStore, + Cfg: cfg.UnifiedAlerting, + FolderService: setupFolderService(t, sqlStore, cfg), + Logger: &logtest.Fake{}, + } + + idMutator := models.WithUniqueID() + createRuleInFolder := func(title string, orgID int64, namespaceUID string) *models.AlertRule { + generator := models.AlertRuleGen(withIntervalMatching(store.Cfg.BaseInterval), idMutator, models.WithNamespace(&folder.Folder{ + UID: namespaceUID, + Title: namespaceUID, + }), withOrgID(orgID), models.WithTitle(title)) + return createRule(t, store, generator) + } + + t.Run("should handle update chains without unique constraint violation", func(t *testing.T) { + rule1 := createRuleInFolder("chain-rule1", 1, "my-namespace") + rule2 := createRuleInFolder("chain-rule2", 1, "my-namespace") + + newRule1 := models.CopyRule(rule1) + newRule2 := models.CopyRule(rule2) + newRule1.Title = rule2.Title + newRule2.Title = util.GenerateShortUID() + + err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + Existing: rule1, + New: *newRule1, + }, { + Existing: rule2, + New: *newRule2, + }, + }) + require.NoError(t, err) + + dbrule1 := &models.AlertRule{} + dbrule2 := &models.AlertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(models.AlertRule{}).ID(rule1.ID).Get(dbrule1) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule2.ID).Get(dbrule2) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID)) + return nil + }) + + require.NoError(t, err) + require.Equal(t, newRule1.Title, dbrule1.Title) + require.Equal(t, newRule2.Title, dbrule2.Title) + }) + + t.Run("should handle update chains with cycle without unique constraint violation", func(t *testing.T) { + rule1 := createRuleInFolder("cycle-rule1", 1, "my-namespace") + rule2 := createRuleInFolder("cycle-rule2", 1, "my-namespace") + rule3 := createRuleInFolder("cycle-rule3", 1, "my-namespace") + + newRule1 := models.CopyRule(rule1) + newRule2 := models.CopyRule(rule2) + newRule3 := models.CopyRule(rule3) + newRule1.Title = rule2.Title + newRule2.Title = rule3.Title + newRule3.Title = rule1.Title + + err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + Existing: rule1, + New: *newRule1, + }, { + Existing: rule2, + New: *newRule2, + }, { + Existing: rule3, + New: *newRule3, + }, + }) + require.NoError(t, err) + + dbrule1 := &models.AlertRule{} + dbrule2 := &models.AlertRule{} + dbrule3 := &models.AlertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(models.AlertRule{}).ID(rule1.ID).Get(dbrule1) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule2.ID).Get(dbrule2) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule3.ID).Get(dbrule3) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule3.ID)) + return nil + }) + + require.NoError(t, err) + require.Equal(t, newRule1.Title, dbrule1.Title) + require.Equal(t, newRule2.Title, dbrule2.Title) + require.Equal(t, newRule3.Title, dbrule3.Title) + }) + + t.Run("should handle case-insensitive intermediate collision without unique constraint violation", func(t *testing.T) { + rule1 := createRuleInFolder("case-cycle-rule1", 1, "my-namespace") + rule2 := createRuleInFolder("case-cycle-rule2", 1, "my-namespace") + + newRule1 := models.CopyRule(rule1) + newRule2 := models.CopyRule(rule2) + newRule1.Title = strings.ToUpper(rule2.Title) + newRule2.Title = strings.ToUpper(rule1.Title) + + err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + Existing: rule1, + New: *newRule1, + }, { + Existing: rule2, + New: *newRule2, + }, + }) + require.NoError(t, err) + + dbrule1 := &models.AlertRule{} + dbrule2 := &models.AlertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(models.AlertRule{}).ID(rule1.ID).Get(dbrule1) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule2.ID).Get(dbrule2) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID)) + return nil + }) + + require.NoError(t, err) + require.Equal(t, newRule1.Title, dbrule1.Title) + require.Equal(t, newRule2.Title, dbrule2.Title) + }) + + t.Run("should handle update multiple chains in different folders without unique constraint violation", func(t *testing.T) { + rule1 := createRuleInFolder("multi-cycle-rule1", 1, "my-namespace") + rule2 := createRuleInFolder("multi-cycle-rule2", 1, "my-namespace") + rule3 := createRuleInFolder("multi-cycle-rule1", 1, "my-namespace2") + rule4 := createRuleInFolder("multi-cycle-rule2", 1, "my-namespace2") + + newRule1 := models.CopyRule(rule1) + newRule2 := models.CopyRule(rule2) + newRule3 := models.CopyRule(rule3) + newRule4 := models.CopyRule(rule4) + newRule1.Title = rule2.Title + newRule2.Title = rule1.Title + newRule3.Title = rule4.Title + newRule4.Title = rule3.Title + + err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + Existing: rule1, + New: *newRule1, + }, { + Existing: rule2, + New: *newRule2, + }, { + Existing: rule3, + New: *newRule3, + }, { + Existing: rule4, + New: *newRule4, + }, + }) + require.NoError(t, err) + + dbrule1 := &models.AlertRule{} + dbrule2 := &models.AlertRule{} + dbrule3 := &models.AlertRule{} + dbrule4 := &models.AlertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(models.AlertRule{}).ID(rule1.ID).Get(dbrule1) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule2.ID).Get(dbrule2) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule3.ID).Get(dbrule3) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule3.ID)) + + exist, err = sess.Table(models.AlertRule{}).ID(rule4.ID).Get(dbrule4) + if err != nil { + return err + } + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule4.ID)) + return nil + }) + + require.NoError(t, err) + require.Equal(t, newRule1.Title, dbrule1.Title) + require.Equal(t, newRule2.Title, dbrule2.Title) + require.Equal(t, newRule3.Title, dbrule3.Title) + require.Equal(t, newRule4.Title, dbrule4.Title) + }) +} + func TestIntegration_GetAlertRulesForScheduling(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") @@ -99,6 +332,8 @@ func TestIntegration_GetAlertRulesForScheduling(t *testing.T) { generator := models.AlertRuleGen(withIntervalMatching(store.Cfg.BaseInterval), models.WithUniqueID(), models.WithUniqueOrgID()) rule1 := createRule(t, store, generator) rule2 := createRule(t, store, generator) + createFolder(t, store, rule1.NamespaceUID, rule1.Title, rule1.OrgID) + createFolder(t, store, rule2.NamespaceUID, rule2.Title, rule2.OrgID) tc := []struct { name string @@ -251,7 +486,6 @@ func createRule(t *testing.T, store *DBstore, generate func() *models.AlertRule) generate = models.AlertRuleGen(withIntervalMatching(store.Cfg.BaseInterval), models.WithUniqueID()) } rule := generate() - createFolder(t, store, rule.NamespaceUID, rule.Title, rule.OrgID) err := store.SQLStore.WithDbSession(context.Background(), func(sess *db.Session) error { _, err := sess.Table(models.AlertRule{}).InsertOne(rule) if err != nil { diff --git a/pkg/tests/api/alerting/api_ruler_test.go b/pkg/tests/api/alerting/api_ruler_test.go index d3884a8810e..e774a08068d 100644 --- a/pkg/tests/api/alerting/api_ruler_test.go +++ b/pkg/tests/api/alerting/api_ruler_test.go @@ -358,9 +358,10 @@ func TestIntegrationAlertRuleConflictingTitle(t *testing.T) { require.Len(t, createdRuleGroup.Rules, 2) t.Run("trying to create alert with same title under same folder should fail", func(t *testing.T) { - rules := newTestingRuleConfig(t) + rulesWithUID := convertGettableRuleGroupToPostable(createdRuleGroup) + rulesWithUID.Rules = append(rulesWithUID.Rules, rules.Rules[0]) // Create new copy of first rule. - status, body := apiClient.PostRulesGroup(t, "folder1", &rules) + status, body := apiClient.PostRulesGroup(t, "folder1", &rulesWithUID) assert.Equal(t, http.StatusInternalServerError, status) var res map[string]interface{} @@ -369,12 +370,10 @@ func TestIntegrationAlertRuleConflictingTitle(t *testing.T) { }) t.Run("trying to update an alert to the title of an existing alert in the same folder should fail", func(t *testing.T) { - rules := newTestingRuleConfig(t) - rules.Rules[0].GrafanaManagedAlert.UID = createdRuleGroup.Rules[0].GrafanaManagedAlert.UID - rules.Rules[1].GrafanaManagedAlert.UID = createdRuleGroup.Rules[1].GrafanaManagedAlert.UID - rules.Rules[1].GrafanaManagedAlert.Title = "AlwaysFiring" + rulesWithUID := convertGettableRuleGroupToPostable(createdRuleGroup) + rulesWithUID.Rules[1].GrafanaManagedAlert.Title = "AlwaysFiring" - status, body := apiClient.PostRulesGroup(t, "folder1", &rules) + status, body := apiClient.PostRulesGroup(t, "folder1", &rulesWithUID) assert.Equal(t, http.StatusInternalServerError, status) var res map[string]interface{} @@ -388,6 +387,28 @@ func TestIntegrationAlertRuleConflictingTitle(t *testing.T) { assert.Equal(t, http.StatusAccepted, status) require.JSONEq(t, `{"message":"rule group updated successfully"}`, body) }) + + t.Run("trying to swap titles of existing alerts in the same folder should work", func(t *testing.T) { + rulesWithUID := convertGettableRuleGroupToPostable(createdRuleGroup) + title0 := rulesWithUID.Rules[0].GrafanaManagedAlert.Title + title1 := rulesWithUID.Rules[1].GrafanaManagedAlert.Title + rulesWithUID.Rules[0].GrafanaManagedAlert.Title = title1 + rulesWithUID.Rules[1].GrafanaManagedAlert.Title = title0 + + status, body := apiClient.PostRulesGroup(t, "folder1", &rulesWithUID) + assert.Equal(t, http.StatusAccepted, status) + require.JSONEq(t, `{"message":"rule group updated successfully"}`, body) + }) + + t.Run("trying to update titles of existing alerts in a chain in the same folder should work", func(t *testing.T) { + rulesWithUID := convertGettableRuleGroupToPostable(createdRuleGroup) + rulesWithUID.Rules[0].GrafanaManagedAlert.Title = rulesWithUID.Rules[1].GrafanaManagedAlert.Title + rulesWithUID.Rules[1].GrafanaManagedAlert.Title = "something new" + + status, body := apiClient.PostRulesGroup(t, "folder1", &rulesWithUID) + assert.Equal(t, http.StatusAccepted, status) + require.JSONEq(t, `{"message":"rule group updated successfully"}`, body) + }) } func TestIntegrationRulerRulesFilterByDashboard(t *testing.T) {