ref(alerts): Prevent name data from being saved to db (#54746)

Backend piece to #54739 to make
sure we aren't saving the `name` field to the `Rule` table's `data`
column.

Author: ceorourke

src/sentry/api/endpoints/project_rule_details.py

@@ -31,7 +31,6 @@
 from sentry.services.hybrid_cloud.user.service import user_service
 from sentry.signals import alert_rule_edited
 from sentry.tasks.integrations.slack import find_channel_id_for_rule
-from sentry.utils import metrics
 from sentry.web.decorators import transaction_start
 
 
@@ -171,8 +170,6 @@ def put(self, request: Request, project, rule) -> Response:
 
             trigger_sentry_app_action_creators_for_issues(actions=kwargs.get("actions"))
 
-            if rule.data["conditions"] != kwargs["conditions"]:
-                metrics.incr("sentry.issue_alert.conditions.edited", sample_rate=1.0)
             updated_rule = project_rules.Updater.run(rule=rule, request=request, **kwargs)
 
             RuleActivity.objects.create(

src/sentry/api/serializers/rest_framework/rule.py

@@ -148,6 +148,13 @@ def validate_environment(self, environment):
 
         return environment
 
+    def validate_conditions(self, conditions):
+        for condition in conditions:
+            if condition.get("name"):
+                del condition["name"]
+
+        return conditions
+
     def validate(self, attrs):
         return super().validate(validate_actions(attrs))
 
@@ -181,6 +188,8 @@ def validate_actions(attrs):
     # project_rule(_details) endpoints by setting it on attrs
     actions = attrs.get("actions", tuple())
     for action in actions:
+        if action.get("name"):
+            del action["name"]
         # XXX(colleen): For ticket rules we need to ensure the user has
         # at least done minimal configuration
         if action["id"] in TICKET_ACTIONS:

src/sentry/monitors/utils.py

@@ -228,11 +228,9 @@ def create_alert_rule_data(project: Project, user: User, monitor: Monitor, alert
         "conditions": [
             {
                 "id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition",
-                "name": "A new issue is created",
             },
             {
                 "id": "sentry.rules.conditions.regression_event.RegressionEventCondition",
-                "name": "The issue changes state from resolved to unresolved",
             },
         ],
         "createdBy": {
@@ -248,7 +246,6 @@ def create_alert_rule_data(project: Project, user: User, monitor: Monitor, alert
                 "id": "sentry.rules.filters.tagged_event.TaggedEventFilter",
                 "key": "monitor.slug",
                 "match": "eq",
-                "name": f"The event's tags match monitor.slug contains {monitor.slug}",
                 "value": monitor.slug,
             }
         ],
@@ -265,7 +262,6 @@ def create_alert_rule_data(project: Project, user: User, monitor: Monitor, alert
 
         action = {
             "id": "sentry.mail.actions.NotifyEmailAction",
-            "name": f"Send a notification to {target_type}",
             "targetIdentifier": target_identifier,
             "targetType": target_type,
         }
@@ -282,7 +278,6 @@ def update_alert_rule(request: Request, project: Project, alert_rule: Rule, aler
 
         action = {
             "id": "sentry.mail.actions.NotifyEmailAction",
-            "name": f"Send a notification to {target_type}",
             "targetIdentifier": target_identifier,
             "targetType": target_type,
         }

tests/sentry/api/endpoints/test_project_rule_details.py

@@ -2,10 +2,8 @@
 
 from datetime import datetime, timezone
 from typing import Any, Mapping
-from unittest import mock
-from unittest.mock import call, patch
+from unittest.mock import patch
 
-import pytest
 import responses
 from freezegun import freeze_time
 
@@ -49,6 +47,8 @@ def assert_rule_from_payload(rule: Rule, payload: Mapping[str, Any]) -> None:
     # any(a.items() <= b.items()) to check if the payload dict is a subset of the rule.data dict
     # E.g. payload["actions"] = [{"name": "Test1"}], rule.data["actions"] = [{"name": "Test1", "id": 1}]
     for payload_action in payload.get("actions", []):
+        if payload_action.get("name"):
+            del payload_action["name"]
         # The Slack payload will contain '#channel' or '@user', but we save 'channel' or 'user' on the Rule
         if (
             payload_action["id"]
@@ -60,6 +60,8 @@ def assert_rule_from_payload(rule: Rule, payload: Mapping[str, Any]) -> None:
         )
     payload_conditions = payload.get("conditions", []) + payload.get("filters", [])
     for payload_condition in payload_conditions:
+        if payload_condition.get("name"):
+            del payload_condition["name"]
         assert any(
             payload_condition.items() <= rule_condition.items()
             for rule_condition in rule.data["conditions"]
@@ -102,6 +104,7 @@ def test_simple(self):
         )
         assert response.data["id"] == str(self.rule.id)
         assert response.data["environment"] is None
+        assert response.data["conditions"][0]["name"]
 
     def test_non_existing_rule(self):
         self.get_error_response(self.organization.slug, self.project.slug, 12345, status_code=404)
@@ -313,11 +316,6 @@ def test_with_jira_action_error(self):
 
 @region_silo_test(stable=True)
 class UpdateProjectRuleTest(ProjectRuleDetailsBaseTestCase):
-    @pytest.fixture(autouse=True)
-    def _setup_metric_patch(self):
-        with mock.patch("sentry.api.endpoints.project_rule_details.metrics") as self.metrics:
-            yield
-
     method = "PUT"
 
     @patch("sentry.signals.alert_rule_edited.send_robust")
@@ -638,7 +636,12 @@ def test_rule_form_missing_action(self):
         )
 
     def test_update_filters(self):
-        conditions = [{"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"}]
+        conditions = [
+            {
+                "id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition",
+                "name": "A new issue is created",
+            }
+        ]
         filters = [
             {"id": "sentry.rules.filters.issue_occurrences.IssueOccurrencesFilter", "value": 10}
         ]
@@ -735,10 +738,6 @@ def test_edit_condition_metric(self):
         self.get_success_response(
             self.organization.slug, self.project.slug, self.rule.id, status_code=200, **payload
         )
-        assert (
-            call("sentry.issue_alert.conditions.edited", sample_rate=1.0)
-            in self.metrics.incr.call_args_list
-        )
 
     def test_edit_non_condition_metric(self):
         payload = {
@@ -752,10 +751,6 @@ def test_edit_non_condition_metric(self):
         self.get_success_response(
             self.organization.slug, self.project.slug, self.rule.id, status_code=200, **payload
         )
-        assert (
-            call("sentry.issue_alert.conditions.edited", sample_rate=1.0)
-            not in self.metrics.incr.call_args_list
-        )
 
 
 @region_silo_test(stable=True)

tests/sentry/api/endpoints/test_project_rules.py

@@ -54,6 +54,14 @@ def test_simple(self):
 class CreateProjectRuleTest(ProjectRuleBaseTestCase):
     method = "post"
 
+    def clean_data(self, data):
+        cleaned_data = []
+        for datum in data:
+            if datum.get("name"):
+                del datum["name"]
+            cleaned_data.append(datum)
+        return cleaned_data
+
     def run_test(
         self,
         actions: Sequence[Mapping[str, Any]] | None = None,
@@ -101,9 +109,15 @@ def run_test(
         assert rule.owner == get_actor_for_user(self.user)
         assert rule.data["action_match"] == action_match
         assert rule.data["filter_match"] == filter_match
-        assert rule.data["actions"] == actions
+
+        updated_actions = self.clean_data(actions)
+        assert rule.data["actions"] == updated_actions
+
+        if conditions:
+            updated_conditions = self.clean_data(conditions)
+
         assert rule.data["conditions"] == (
-            expected_conditions if expected_conditions is not None else conditions
+            expected_conditions if expected_conditions is not None else updated_conditions
         )
         assert rule.data["frequency"] == frequency
         assert rule.created_by_id == self.user.id
@@ -127,6 +141,22 @@ def test_simple(self):
 
         self.run_test(actions=actions, conditions=conditions)
 
+    def test_with_name(self):
+        conditions = [
+            {
+                "id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition",
+                "name": "A new issue is created",
+            }
+        ]
+        actions = [
+            {
+                "id": "sentry.rules.actions.notify_event.NotifyEventAction",
+                "name": "Send a notification to IssueOwners and if none can be found then send a notification to ActiveMembers",
+            }
+        ]
+
+        self.run_test(actions=actions, conditions=conditions)
+
     def test_with_environment(self):
         Environment.get_or_create(self.project, "production")
         conditions = [{"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"}]
@@ -352,7 +382,10 @@ def test_match_values(self):
 
     def test_with_filters(self):
         conditions: list[dict[str, Any]] = [
-            {"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"}
+            {
+                "id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition",
+                "name": "A new issue is created",
+            }
         ]
         filters: list[dict[str, Any]] = [
             {"id": "sentry.rules.filters.issue_occurrences.IssueOccurrencesFilter", "value": 10}
@@ -459,6 +492,7 @@ def test_kicks_off_slack_async_job(
 
         self.user = User.objects.get(id=self.user.id)  # reload user to get actor
         assert not Rule.objects.filter(label=payload["name"]).exists()
+        payload["actions"][0].pop("name")
         kwargs = {
             "name": payload["name"],
             "owner": get_actor_id_for_user(self.user),