diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 12d6b63c198..359b3c1a5b7 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -339,7 +339,7 @@ func (srv RulerSrv) RouteGetRuleByUID(c *contextmodel.ReqContext, ruleUID string func (srv RulerSrv) RouteGetRuleVersionsByUID(c *contextmodel.ReqContext, ruleUID string) response.Response { ctx := c.Req.Context() // make sure the user has access to the current version of the rule. Also, check if it exists - _, err := srv.getAuthorizedRuleByUid(ctx, c, ruleUID) + rule, err := srv.getAuthorizedRuleByUid(ctx, c, ruleUID) if err != nil { if errors.Is(err, ngmodels.ErrAlertRuleNotFound) { return response.Empty(http.StatusNotFound) @@ -347,7 +347,7 @@ func (srv RulerSrv) RouteGetRuleVersionsByUID(c *contextmodel.ReqContext, ruleUI return response.ErrOrFallback(http.StatusInternalServerError, "failed to get rule by UID", err) } - rules, err := srv.store.GetAlertRuleVersions(ctx, ngmodels.AlertRuleKey{OrgID: c.OrgID, UID: ruleUID}) + rules, err := srv.store.GetAlertRuleVersions(ctx, rule.OrgID, rule.GUID) if err != nil { return response.ErrOrFallback(http.StatusInternalServerError, "failed to get rule history", err) } diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index e61258a7110..17b0d8334ab 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -486,7 +487,7 @@ func TestRouteGetRuleHistoryByUID(t *testing.T) { } ruleStore.PutRule(context.Background(), rule) - ruleStore.History[rule.GetKey()] = append(ruleStore.History[rule.GetKey()], history...) + ruleStore.History[rule.GUID] = append(ruleStore.History[rule.GUID], history...) perms := createPermissionsForRules([]*models.AlertRule{rule}, orgID) req := createRequestContextWithPerms(orgID, perms, nil) @@ -516,8 +517,9 @@ func TestRouteGetRuleHistoryByUID(t *testing.T) { OrgID: orgID, UID: "test", } - history := gen.With(gen.WithKey(ruleKey)).GenerateManyRef(3) - ruleStore.History[ruleKey] = append(ruleStore.History[ruleKey], history...) // even if history is full of records + guid := uuid.NewString() + history := gen.With(gen.WithGUID(guid), gen.WithKey(ruleKey)).GenerateManyRef(3) + ruleStore.History[guid] = append(ruleStore.History[guid], history...) // even if history is full of records perms := createPermissionsForRules(history, orgID) req := createRequestContextWithPerms(orgID, perms, nil) @@ -533,9 +535,10 @@ func TestRouteGetRuleHistoryByUID(t *testing.T) { OrgID: orgID, UID: "test", } - rule := gen.With(gen.WithKey(ruleKey)).GenerateRef() + guid := uuid.NewString() + rule := gen.With(gen.WithKey(ruleKey), gen.WithGUID(guid)).GenerateRef() ruleStore.PutRule(context.Background(), rule) - ruleStore.History[ruleKey] = nil + ruleStore.History[guid] = nil perms := createPermissionsForRules([]*models.AlertRule{rule}, orgID) req := createRequestContextWithPerms(orgID, perms, nil) @@ -556,10 +559,11 @@ func TestRouteGetRuleHistoryByUID(t *testing.T) { OrgID: orgID, UID: "test", } - rule := gen.With(gen.WithKey(ruleKey), gen.WithNamespaceUID(anotherFolder.UID)).GenerateRef() + guid := uuid.NewString() + rule := gen.With(gen.WithGUID(guid), gen.WithKey(ruleKey), gen.WithNamespaceUID(anotherFolder.UID)).GenerateRef() ruleStore.PutRule(context.Background(), rule) - history := gen.With(gen.WithKey(ruleKey)).GenerateManyRef(3) - ruleStore.History[ruleKey] = history + history := gen.With(gen.WithGUID(guid), gen.WithKey(ruleKey)).GenerateManyRef(3) + ruleStore.History[guid] = history perms := createPermissionsForRules(history, orgID) // grant permissions to all records in history but not the rule itself req := createRequestContextWithPerms(orgID, perms, nil) diff --git a/pkg/services/ngalert/api/persist.go b/pkg/services/ngalert/api/persist.go index 57eda01631d..228f62cf24e 100644 --- a/pkg/services/ngalert/api/persist.go +++ b/pkg/services/ngalert/api/persist.go @@ -30,6 +30,6 @@ type RuleStore interface { // IncreaseVersionForAllRulesInNamespaces Increases version for all rules that have specified namespace uids IncreaseVersionForAllRulesInNamespaces(ctx context.Context, orgID int64, namespaceUIDs []string) ([]ngmodels.AlertRuleKeyWithVersion, error) - GetAlertRuleVersions(ctx context.Context, key ngmodels.AlertRuleKey) ([]*ngmodels.AlertRule, error) + GetAlertRuleVersions(ctx context.Context, orgID int64, guid string) ([]*ngmodels.AlertRule, error) accesscontrol.RuleUIDToNamespaceStore } diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index b56c660876b..7a97d7f02ca 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -125,10 +125,10 @@ func (st DBstore) GetAlertRuleByUID(ctx context.Context, query *ngmodels.GetAler return result, err } -func (st DBstore) GetAlertRuleVersions(ctx context.Context, key ngmodels.AlertRuleKey) ([]*ngmodels.AlertRule, error) { +func (st DBstore) GetAlertRuleVersions(ctx context.Context, orgID int64, guid string) ([]*ngmodels.AlertRule, error) { alertRules := make([]*ngmodels.AlertRule, 0) err := st.SQLStore.WithDbSession(ctx, func(sess *db.Session) error { - rows, err := sess.Table(new(alertRuleVersion)).Where("rule_org_id = ? AND rule_uid = ?", key.OrgID, key.UID).Asc("id").Rows(new(alertRuleVersion)) + rows, err := sess.Table(new(alertRuleVersion)).Where("rule_org_id = ? AND rule_guid = ?", orgID, guid).Asc("id").Rows(new(alertRuleVersion)) if err != nil { return err } @@ -330,7 +330,7 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, user *ngmodels.UserUID, return fmt.Errorf("failed to convert alert rule %s to storage model: %w", r.New.UID, err) } // no way to update multiple rules at once - if updated, err := sess.ID(r.Existing.ID).AllCols().Update(converted); err != nil || updated == 0 { + if updated, err := sess.ID(r.Existing.ID).AllCols().Omit("rule_guid").Update(converted); err != nil || updated == 0 { if err != nil { if st.SQLStore.GetDialect().IsUniqueConstraintViolation(err) { return ruleConstraintViolationToErr(sess, r.New, err, st.Logger) diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 1f833a3e10f..1e60dfc9249 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -1599,7 +1599,7 @@ func TestGetRuleVersions(t *testing.T) { require.NoError(t, err) t.Run("should return rule versions sorted in decreasing order", func(t *testing.T) { - versions, err := store.GetAlertRuleVersions(context.Background(), ruleV2.GetKey()) + versions, err := store.GetAlertRuleVersions(context.Background(), ruleV2.OrgID, ruleV2.GUID) require.NoError(t, err) assert.Len(t, versions, 2) assert.IsDecreasing(t, versions[0].ID, versions[1].ID) @@ -1630,7 +1630,7 @@ func TestGetRuleVersions(t *testing.T) { }, }) - versions, err := store.GetAlertRuleVersions(context.Background(), ruleV3.GetKey()) + versions, err := store.GetAlertRuleVersions(context.Background(), ruleV3.OrgID, ruleV3.GUID) require.NoError(t, err) assert.Len(t, versions, 3) diff := versions[0].Diff(versions[1], AlertRuleFieldsToIgnoreInDiff[:]...) diff --git a/pkg/services/ngalert/tests/fakes/rules.go b/pkg/services/ngalert/tests/fakes/rules.go index 728ae6bd76f..1a3e4ce9a63 100644 --- a/pkg/services/ngalert/tests/fakes/rules.go +++ b/pkg/services/ngalert/tests/fakes/rules.go @@ -23,7 +23,7 @@ type RuleStore struct { mtx sync.Mutex // OrgID -> RuleGroup -> Namespace -> Rules Rules map[int64][]*models.AlertRule - History map[models.AlertRuleKey][]*models.AlertRule + History map[string][]*models.AlertRule Hook func(cmd any) error // use Hook if you need to intercept some query and return an error RecordedOps []any Folders map[int64][]*folder.Folder @@ -42,7 +42,7 @@ func NewRuleStore(t *testing.T) *RuleStore { return nil }, Folders: map[int64][]*folder.Folder{}, - History: map[models.AlertRuleKey][]*models.AlertRule{}, + History: map[string][]*models.AlertRule{}, } } @@ -54,7 +54,7 @@ mainloop: for _, r := range rules { rgs := f.Rules[r.OrgID] cp := models.CopyRule(r) - f.History[r.GetKey()] = append(f.History[r.GetKey()], cp) + f.History[r.GUID] = append(f.History[r.GUID], cp) for idx, rulePtr := range rgs { if rulePtr.UID == r.UID { rgs[idx] = r @@ -426,21 +426,21 @@ func (f *RuleStore) GetNamespacesByRuleUID(ctx context.Context, orgID int64, uid return namespacesMap, nil } -func (f *RuleStore) GetAlertRuleVersions(_ context.Context, key models.AlertRuleKey) ([]*models.AlertRule, error) { +func (f *RuleStore) GetAlertRuleVersions(_ context.Context, orgID int64, guid string) ([]*models.AlertRule, error) { f.mtx.Lock() defer f.mtx.Unlock() q := GenericRecordedQuery{ Name: "GetAlertRuleVersions", - Params: []any{key}, + Params: []any{orgID, guid}, } defer func() { f.RecordedOps = append(f.RecordedOps, q) }() - if err := f.Hook(key); err != nil { + if err := f.Hook([]any{orgID, guid}); err != nil { return nil, err } - return f.History[key], nil + return f.History[guid], nil }