Alerting: Fix xorm serialization of Record field struct, add tests for storing and reading (#87857)

Fix sub struct ser and deser, add tests

pkg/services/ngalert/models/alert_rule.go

@@ -243,7 +243,7 @@ type AlertRule struct {
 	PanelID         *int64  `xorm:"panel_id"`
 	RuleGroup       string
 	RuleGroupIndex  int     `xorm:"rule_group_idx"`
-	Record          *Record `xorm:"text null 'record'"`
+	Record          *Record `xorm:"json"`
 	NoDataState     NoDataState
 	ExecErrState    ExecutionErrorState
 	// ideally this field should have been apimodels.ApiDuration
@@ -518,10 +518,6 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting
 		}
 	}
 
-	if alertRule.Record != nil {
-		return fmt.Errorf("%w: storing recording rules is not yet allowed", ErrAlertRuleFailedValidation)
-	}
-
 	if len(alertRule.NotificationSettings) > 0 {
 		if len(alertRule.NotificationSettings) != 1 {
 			return fmt.Errorf("%w: only one notification settings entry is allowed", ErrAlertRuleFailedValidation)
@@ -552,6 +548,10 @@ func (alertRule *AlertRule) GetFolderKey() FolderKey {
 	}
 }
 
+func (alertRule *AlertRule) IsRecordingRule() bool {
+	return alertRule.Record != nil
+}
+
 // AlertRuleVersion is the model for alert rule versions in unified alerting.
 type AlertRuleVersion struct {
 	ID               int64  `xorm:"pk autoincr 'id'"`
@@ -569,7 +569,7 @@ type AlertRuleVersion struct {
 	Condition       string
 	Data            []AlertQuery
 	IntervalSeconds int64
-	Record          *Record `xorm:"text null 'record'"`
+	Record          *Record `xorm:"json"`
 	NoDataState     NoDataState
 	ExecErrState    ExecutionErrorState
 	// ideally this field should have been apimodels.ApiDuration

pkg/services/ngalert/store/alert_rule_test.go

@@ -610,18 +610,32 @@ func TestIntegrationInsertAlertRules(t *testing.T) {
 		Cfg:           cfg.UnifiedAlerting,
 	}
 
-	gen := models.RuleGen
+	gen := models.RuleGen.With(
-	rules := gen.With(
+		models.RuleGen.WithOrgID(orgID),
-		gen.WithOrgID(orgID),
+		models.RuleGen.WithIntervalMatching(store.Cfg.BaseInterval),
-		gen.WithIntervalMatching(store.Cfg.BaseInterval),
+	)
-	).GenerateManyRef(5)
 
-	deref := make([]models.AlertRule, 0, len(rules))
+	generateRecordingRules := func(n int) []models.AlertRule {
-	for _, rule := range rules {
+		rrs := gen.GenerateMany(n)
-		deref = append(deref, *rule)
+		for i := range rrs {
+			rrs[i].Condition = ""
+			// TODO: These fields do not apply to recording rules - for now, we just use the default values of them. This is validated at the storage level.
+			// TODO: Consider making it so recording rules allow for empty string here, or use some other sentinel value in the future.
+			rrs[i].NoDataState = models.NoData
+			rrs[i].ExecErrState = models.ErrorErrState
+			rrs[i].For = 0
+			rrs[i].NotificationSettings = nil
+			rrs[i].Record = &models.Record{
+				Metric: "my_metric",
+				From:   "A",
+			}
+		}
+		return rrs
 	}
 
-	ids, err := store.InsertAlertRules(context.Background(), deref)
+	rules := append(gen.GenerateMany(5), generateRecordingRules(5)...)
+
+	ids, err := store.InsertAlertRules(context.Background(), rules)
 	require.NoError(t, err)
 	require.Len(t, ids, len(rules))
 
@@ -645,29 +659,53 @@ func TestIntegrationInsertAlertRules(t *testing.T) {
 
 	t.Run("inserted alerting rules should have nil recording rule fields on model", func(t *testing.T) {
 		for _, rule := range dbRules {
-			require.Nil(t, rule.Record)
+			if !rule.IsRecordingRule() {
+				require.Nil(t, rule.Record)
+			}
+		}
+	})
+
+	t.Run("inserted recording rules map identical fields when listed", func(t *testing.T) {
+		for _, rule := range dbRules {
+			if rule.IsRecordingRule() {
+				require.NotNil(t, rule.Record)
+				require.Equal(t, "my_metric", rule.Record.Metric)
+				require.Equal(t, "A", rule.Record.From)
+			}
+		}
+	})
+
+	t.Run("inserted recording rules have empty or default alert-specific settings", func(t *testing.T) {
+		for _, rule := range dbRules {
+			if rule.IsRecordingRule() {
+				require.Empty(t, rule.Condition)
+				require.Equal(t, models.NoData, rule.NoDataState)
+				require.Equal(t, models.ErrorErrState, rule.ExecErrState)
+				require.Zero(t, rule.For)
+				require.Nil(t, rule.NotificationSettings)
+			}
 		}
 	})
 
 	t.Run("fail to insert rules with same ID", func(t *testing.T) {
-		_, err = store.InsertAlertRules(context.Background(), []models.AlertRule{deref[0]})
+		_, err = store.InsertAlertRules(context.Background(), []models.AlertRule{rules[0]})
 		require.ErrorIs(t, err, models.ErrAlertRuleConflictBase)
 	})
 	t.Run("fail insert rules with the same title in a folder", func(t *testing.T) {
-		cp := models.CopyRule(&deref[0])
+		cp := models.CopyRule(&rules[0])
 		cp.UID = cp.UID + "-new"
 		_, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp})
 		require.ErrorIs(t, err, models.ErrAlertRuleConflictBase)
 		require.ErrorIs(t, err, models.ErrAlertRuleUniqueConstraintViolation)
-		require.NotEqual(t, deref[0].UID, "")
+		require.NotEqual(t, rules[0].UID, "")
-		require.NotEqual(t, deref[0].Title, "")
+		require.NotEqual(t, rules[0].Title, "")
-		require.NotEqual(t, deref[0].NamespaceUID, "")
+		require.NotEqual(t, rules[0].NamespaceUID, "")
-		require.ErrorContains(t, err, deref[0].UID)
+		require.ErrorContains(t, err, rules[0].UID)
-		require.ErrorContains(t, err, deref[0].Title)
+		require.ErrorContains(t, err, rules[0].Title)
-		require.ErrorContains(t, err, deref[0].NamespaceUID)
+		require.ErrorContains(t, err, rules[0].NamespaceUID)
 	})
 	t.Run("should not let insert rules with the same UID", func(t *testing.T) {
-		cp := models.CopyRule(&deref[0])
+		cp := models.CopyRule(&rules[0])
 		cp.Title = "unique-test-title"
 		_, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp})
 		require.ErrorIs(t, err, models.ErrAlertRuleConflictBase)