Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions pkg/query-service/rules/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -816,3 +816,155 @@ func TestEditRule(t *testing.T) {
})
}
}

func TestDeleteRule(t *testing.T) {
claims := &authtypes.Claims{
Email: "[email protected]",
}
manager, mockSQLRuleStore, mockRouteStore, _, orgId := setupTestManager(t)
claims.OrgID = orgId

testCases := []struct {
name string
ruleData string
}{
{
name: "delete rule with planned maintenance",
ruleData: `{
"alert": "cpu usage to delete",
"ruleType": "threshold_rule",
"evalWindow": "5m",
"frequency": "1m",
"condition": {
"compositeQuery": {
"queryType": "builder",
"builderQueries": {
"A": {
"expression": "A",
"disabled": false,
"dataSource": "metrics",
"aggregateOperator": "avg",
"aggregateAttribute": {
"key": "cpu_usage",
"type": "Gauge"
}
}
}
},
"op": "1",
"target": 80,
"matchType": "1"
},
"labels": {
"severity": "warning"
},
"disabled": false,
"preferredChannels": ["test-alerts"]
}`,
},
{
name: "delete v2 rule with thresholds",
ruleData: `{
"schemaVersion":"v2",
"state": "firing",
"alert": "test-multi-threshold-delete",
"alertType": "METRIC_BASED_ALERT",
"ruleType": "threshold_rule",
"evalWindow": "5m0s",
"condition": {
"thresholds": {
"kind": "basic",
"spec": [
{
"name": "CRITICAL",
"target": 10,
"matchType": "1",
"op": "1",
"selectedQuery": "A",
"channels": ["test-alerts"]
},
{
"name": "WARNING",
"target": 5,
"matchType": "1",
"op": "1",
"selectedQuery": "A",
"channels": ["test-alerts"]
}
]
},
"compositeQuery": {
"queryType": "builder",
"panelType": "graph",
"queries": [
{
"type": "builder_query",
"spec": {
"name": "A",
"signal": "metrics",
"disabled": false,
"aggregations": [
{
"metricName": "container.memory.usage",
"timeAggregation": "avg",
"spaceAggregation": "sum"
}
]
}
}
]
}
},
"evaluation": {
"kind": "rolling",
"spec": {
"evalWindow": "8m",
"frequency": "2m"
}
},
"labels": {
"severity": "critical"
},
"annotations": {
"description": "This alert is fired when memory usage crosses the threshold",
"summary": "Memory usage threshold exceeded"
},
"disabled": false,
"preferredChannels": ["#critical-alerts-v2"],
"version": "v5"
}`,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ruleID := valuer.GenerateUUID()

existingRule := &ruletypes.Rule{
Identifiable: types.Identifiable{
ID: ruleID,
},
TimeAuditable: types.TimeAuditable{
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
},
UserAuditable: types.UserAuditable{
CreatedBy: "[email protected]",
UpdatedBy: "[email protected]",
},
Data: tc.ruleData,
OrgID: claims.OrgID,
}

mockSQLRuleStore.ExpectGetStoredRule(ruleID, existingRule)
mockRouteStore.ExpectDeleteRouteByName(existingRule.OrgID, ruleID.String())
mockSQLRuleStore.ExpectDeleteRule(ruleID)

ctx := authtypes.NewContextWithClaims(context.Background(), *claims)
err := manager.DeleteRule(ctx, ruleID.StringValue())

assert.NoError(t, err)
assert.NoError(t, mockSQLRuleStore.AssertExpectations())
})
}
}
4 changes: 4 additions & 0 deletions pkg/ruler/rulestore/rulestoretest/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ func (m *MockSQLRuleStore) ExpectEditRule(rule *ruletypes.Rule) {

// ExpectDeleteRule sets up SQL expectations for DeleteRule operation
func (m *MockSQLRuleStore) ExpectDeleteRule(ruleID valuer.UUID) {
plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE \(rule_id = '` + ruleID.StringValue() + `'\)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExpectDeleteRule now expects deletion from the 'planned_maintenance_rule' table. Please add an inline comment explaining that this is done to work around the lack of cascade delete for foreign key constraints.

Suggested change
plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE \(rule_id = '` + ruleID.StringValue() + `'\)`
plannedMaintenancePattern := `DELETE FROM "planned_maintenance_rule".+WHERE (rule_id = '` + ruleID.StringValue() + `')` // Workaround: no cascade delete for FK constraints, so must delete explicitly

m.mock.ExpectExec(plannedMaintenancePattern).
WillReturnResult(sqlmock.NewResult(0, 1))

expectedPattern := `DELETE FROM "rule".+WHERE \(id = '` + ruleID.StringValue() + `'\)`
m.mock.ExpectExec(expectedPattern).
WillReturnResult(sqlmock.NewResult(1, 1))
Expand Down
10 changes: 10 additions & 0 deletions pkg/ruler/rulestore/sqlrulestore/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ func (r *rule) EditRule(ctx context.Context, storedRule *ruletypes.Rule, cb func
func (r *rule) DeleteRule(ctx context.Context, id valuer.UUID, cb func(context.Context) error) error {
if err := r.sqlstore.RunInTxCtx(ctx, nil, func(ctx context.Context) error {
_, err := r.sqlstore.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In DeleteRule, add a comment explaining that the deletion of planned maintenance records is performed first to avoid foreign key constraint errors due to lack of cascade delete.

BunDBCtx(ctx).
NewDelete().
Model(new(ruletypes.StorablePlannedMaintenanceRule)).
Where("rule_id = ?", id.StringValue()).
Exec(ctx)
Comment on lines +59 to +63
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deletion from planned_maintenance_rule table should handle the case where no rows are affected, as this is expected when a rule has no planned maintenance associations. Consider storing the result and checking if it's acceptable to have 0 affected rows.

Copilot uses AI. Check for mistakes.

if err != nil {
return err
}

_, err = r.sqlstore.
BunDBCtx(ctx).
NewDelete().
Model(new(ruletypes.Rule)).
Expand Down
Loading