From a4a28791b790692ed46ade1b3b26aaea76bce63c Mon Sep 17 00:00:00 2001 From: Moustafa Baiou Date: Tue, 7 Oct 2025 17:40:59 -0400 Subject: [PATCH] Alerting: Ensure case-sensitive ordering for alert rule group column The query which fetches alert rules in a paginated manner ordered by `rule_group` can result in strange and inconsistent ordering when the database uses a case-insensitive collation for the `rule_group` column. This can lead to scenarios where rules from different groups are interleaved in the results, making pagination unreliable and the returned number of rule_groups incorrect. Related to #88990 --- pkg/services/ngalert/store/alert_rule_test.go | 124 ++++++++++++++++++ .../sqlstore/migrations/migrations.go | 2 + .../ualert/alert_rule_group_collation.go | 9 ++ ...index_mig.go => alert_rule_group_index.go} | 0 4 files changed, 135 insertions(+) create mode 100644 pkg/services/sqlstore/migrations/ualert/alert_rule_group_collation.go rename pkg/services/sqlstore/migrations/ualert/{alert_rule_group_index_mig.go => alert_rule_group_index.go} (100%) diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index 90d557abf0d..0491f8e6dd2 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -1460,6 +1460,130 @@ func TestIntegrationRuleGroupsCaseSensitive(t *testing.T) { }) } +// To address issues arising from case-insensitive collations in some databases (e.g., MySQL/MariaDB), +func TestIntegrationListAlertRulesByGroupCaseSensitiveOrdering(t *testing.T) { + tutil.SkipIntegrationTestInShortMode(t) + + usr := models.UserUID("test") + + sqlStore := db.InitTestDB(t) + cfg := setting.NewCfg() + cfg.UnifiedAlerting.BaseInterval = 1 * time.Second + folderService := setupFolderService(t, sqlStore, cfg, featuremgmt.WithFeatures()) + b := &fakeBus{} + logger := log.New("test-dbstore") + store := createTestStore(sqlStore, folderService, logger, cfg.UnifiedAlerting, b) + store.FeatureToggles = featuremgmt.WithFeatures() + + gen := models.RuleGen.With(models.RuleMuts.WithOrgID(1)) + + // Create namespace and base group key + groupKey := models.GenerateGroupKey(1) + + // Create groups with case-sensitive names: "TEST", "Test", "test" + groupKeyUpper := groupKey + groupKeyUpper.RuleGroup = "TEST" + + groupKeyMixed := groupKey + groupKeyMixed.RuleGroup = "Test" + + groupKeyLower := groupKey + groupKeyLower.RuleGroup = "test" + + // Generate rules for each group + groupUpper := gen.With(gen.WithGroupKey(groupKeyUpper)).GenerateMany(2) + groupMixed := gen.With(gen.WithGroupKey(groupKeyMixed)).GenerateMany(2) + groupLower := gen.With(gen.WithGroupKey(groupKeyLower)).GenerateMany(2) + + // Insert all rules + allRules := append(append(groupUpper, groupMixed...), groupLower...) + _, err := store.InsertAlertRules(context.Background(), &usr, allRules) + require.NoError(t, err) + + t.Run("should order groups case-sensitively", func(t *testing.T) { + result, _, err := store.ListAlertRulesByGroup(context.Background(), &models.ListAlertRulesExtendedQuery{ + ListAlertRulesQuery: models.ListAlertRulesQuery{OrgID: 1}, + }) + require.NoError(t, err) + require.Len(t, result, 6, "should return all 6 rules") + + // Extract group names in order + var groupOrder []string + for _, rule := range result { + if len(groupOrder) == 0 || groupOrder[len(groupOrder)-1] != rule.RuleGroup { + groupOrder = append(groupOrder, rule.RuleGroup) + } + } + + // Verify case-sensitive alphabetical ordering + // Expected order: "TEST", "Test", "test" (uppercase comes before lowercase in ASCII) + expectedOrder := []string{"TEST", "Test", "test"} + require.Equal(t, expectedOrder, groupOrder, "groups should be ordered case-sensitively") + + // Verify each group contains the correct rules + groupRules := make(map[string][]*models.AlertRule) + for _, rule := range result { + groupRules[rule.RuleGroup] = append(groupRules[rule.RuleGroup], rule) + } + + require.Len(t, groupRules["TEST"], 2, "TEST group should have 2 rules") + require.Len(t, groupRules["Test"], 2, "Test group should have 2 rules") + require.Len(t, groupRules["test"], 2, "test group should have 2 rules") + }) + + t.Run("should respect group limit with case-sensitive ordering", func(t *testing.T) { + // Test with limit of 2 groups - should get first 2 groups in case-sensitive order + result, continueToken, err := store.ListAlertRulesByGroup(context.Background(), &models.ListAlertRulesExtendedQuery{ + ListAlertRulesQuery: models.ListAlertRulesQuery{OrgID: 1}, + Limit: 2, + }) + require.NoError(t, err) + require.Len(t, result, 4, "should return 4 rules (2 rules from first 2 groups)") + require.NotEmpty(t, continueToken, "should have continue token when limit is reached") + + // Extract group names from limited result + var limitedGroupOrder []string + for _, rule := range result { + if len(limitedGroupOrder) == 0 || limitedGroupOrder[len(limitedGroupOrder)-1] != rule.RuleGroup { + limitedGroupOrder = append(limitedGroupOrder, rule.RuleGroup) + } + } + + // Should get first 2 groups in case-sensitive order: "TEST", "Test" + expectedLimitedOrder := []string{"TEST", "Test"} + require.Equal(t, expectedLimitedOrder, limitedGroupOrder, "limited result should contain first 2 groups in case-sensitive order") + + // Continue from token to get remaining groups + remainingResult, nextToken, err := store.ListAlertRulesByGroup(context.Background(), &models.ListAlertRulesExtendedQuery{ + ListAlertRulesQuery: models.ListAlertRulesQuery{OrgID: 1}, + ContinueToken: continueToken, + }) + require.NoError(t, err) + require.Len(t, remainingResult, 2, "should return 2 rules from remaining group") + require.Empty(t, nextToken, "should not have continue token when all groups are fetched") + + // Verify the remaining group is "test" + for _, rule := range remainingResult { + require.Equal(t, "test", rule.RuleGroup, "remaining group should be 'test'") + } + }) + + t.Run("should handle group limit of 1 correctly", func(t *testing.T) { + result, continueToken, err := store.ListAlertRulesByGroup(context.Background(), &models.ListAlertRulesExtendedQuery{ + ListAlertRulesQuery: models.ListAlertRulesQuery{OrgID: 1}, + Limit: 1, + }) + require.NoError(t, err) + require.Len(t, result, 2, "should return 2 rules from first group") + require.NotEmpty(t, continueToken, "should have continue token") + + // Should only get the first group "TEST" + for _, rule := range result { + require.Equal(t, "TEST", rule.RuleGroup, "should only return rules from TEST group") + } + }) +} + func TestIntegrationIncreaseVersionForAllRulesInNamespaces(t *testing.T) { tutil.SkipIntegrationTestInShortMode(t) diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index afabfc8a359..e4f6ac50893 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -157,4 +157,6 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { ualert.AddStateFiredAtColumn(mg) ualert.AddAlertRuleGroupIndexMigration(mg) + + ualert.CollateAlertRuleGroup(mg) } diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule_group_collation.go b/pkg/services/sqlstore/migrations/ualert/alert_rule_group_collation.go new file mode 100644 index 00000000000..793fe1b8f78 --- /dev/null +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule_group_collation.go @@ -0,0 +1,9 @@ +package ualert + +import "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + +// CollateAlertRuleGroup ensures that rule_group column collates. +func CollateAlertRuleGroup(mg *migrator.Migrator) { + mg.AddMigration("ensure rule_group column is case sensitive in returned results", migrator.NewRawSQLMigration(""). + Mysql("ALTER TABLE alert_rule MODIFY rule_group VARCHAR(190) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_bin NOT NULL;")) +} diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule_group_index_mig.go b/pkg/services/sqlstore/migrations/ualert/alert_rule_group_index.go similarity index 100% rename from pkg/services/sqlstore/migrations/ualert/alert_rule_group_index_mig.go rename to pkg/services/sqlstore/migrations/ualert/alert_rule_group_index.go