Skip to content

Commit 9593038

Browse files
authored
ref(rules): Pre-save rules w/o condition names (#55091)
Add a `pre_save` signal to `Rule` to hopefully actually prevent any `name` data being save in the conditions part of rule.data. Everything should have been covered in https://github.com/getsentry/sentry/pull/54746/files but looking at production data since that was deployed shows an edge case where sometimes it's still being saved that I can't replicate. Closes #55112
1 parent 1015c81 commit 9593038

File tree

5 files changed

+48
-4
lines changed

5 files changed

+48
-4
lines changed

src/sentry/api/endpoints/project_rules.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
from django.conf import settings
2+
from django.db.models.signals import pre_save
3+
from django.dispatch import receiver
24
from rest_framework import status
35
from rest_framework.request import Request
46
from rest_framework.response import Response
@@ -20,6 +22,18 @@
2022
from sentry.web.decorators import transaction_start
2123

2224

25+
def clean_rule_data(data):
26+
for datum in data:
27+
if datum.get("name"):
28+
del datum["name"]
29+
30+
31+
@receiver(pre_save, sender=Rule)
32+
def pre_save_rule(instance, sender, *args, **kwargs):
33+
clean_rule_data(instance.data.get("conditions", []))
34+
clean_rule_data(instance.data.get("actions", []))
35+
36+
2337
@region_silo_endpoint
2438
class ProjectRulesEndpoint(ProjectEndpoint):
2539
owner = ApiOwner.ISSUES

tests/sentry/api/endpoints/test_project_rules.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,40 @@ def test_with_name(self):
159159

160160
self.run_test(actions=actions, conditions=conditions)
161161

162+
def test_pre_save(self):
163+
"""Test that a rule with name data in the conditions and actions is saved without it"""
164+
conditions = [
165+
{
166+
"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition",
167+
"name": "A new issue is created",
168+
}
169+
]
170+
actions = [
171+
{
172+
"id": "sentry.rules.actions.notify_event.NotifyEventAction",
173+
"name": "Send a notification to IssueOwners and if none can be found then send a notification to ActiveMembers",
174+
}
175+
]
176+
response = self.get_success_response(
177+
self.organization.slug,
178+
self.project.slug,
179+
name="hello world",
180+
owner=f"user:{self.user.id}",
181+
environment=None,
182+
actionMatch="any",
183+
frequency=5,
184+
actions=actions,
185+
conditions=conditions,
186+
status_code=status.HTTP_200_OK,
187+
)
188+
rule = Rule.objects.get(id=response.data.get("id"))
189+
assert rule.data["actions"][0] == {
190+
"id": "sentry.rules.actions.notify_event.NotifyEventAction"
191+
}
192+
assert rule.data["conditions"][0] == {
193+
"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"
194+
}
195+
162196
def test_with_environment(self):
163197
Environment.get_or_create(self.project, "production")
164198
conditions = [{"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"}]

tests/sentry/integrations/slack/test_tasks.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ def test_task_new_rule(self, mock_set_value):
116116
"channel": "#my-channel",
117117
"channel_id": "chan-id",
118118
"id": "sentry.integrations.slack.notify_action.SlackNotifyServiceAction",
119-
"name": "Send a notification to the funinthesun Slack workspace to #secrets and show tags [] in notification",
120119
"tags": "",
121120
"workspace": self.integration.id,
122121
}
@@ -165,7 +164,6 @@ def test_task_existing_rule(self, mock_set_value):
165164
"channel": "#my-channel",
166165
"channel_id": "chan-id",
167166
"id": "sentry.integrations.slack.notify_action.SlackNotifyServiceAction",
168-
"name": "Send a notification to the funinthesun Slack workspace to #secrets and show tags [] in notification",
169167
"tags": "",
170168
"workspace": self.integration.id,
171169
}

tests/sentry/mediators/project_rules/test_creator.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ def test_creates_rule(self):
4848
"actions": [
4949
{
5050
"id": "sentry.rules.actions.notify_event.NotifyEventAction",
51-
"name": "Send a notification (for all legacy integrations)",
5251
}
5352
],
5453
"conditions": [

tests/sentry/mediators/project_rules/test_updater.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ def test_update_actions(self):
6363
assert self.rule.data["actions"] == [
6464
{
6565
"id": "sentry.rules.actions.notify_event.NotifyEventAction",
66-
"name": "Send a notification (for all legacy integrations)",
6766
}
6867
]
6968

0 commit comments

Comments
 (0)