Browse Source

refs(metric_alerts): Remove duplicate query validation when creating metric alerts (#36099)

Follow up from https://github.com/getsentry/sentry/pull/35796. There's not much point performing
this validation here, since we already perform more thorough validation that invovles running the
query in the serializers. Since we need to convert this over to use `build_query_builder` to keep
it, it seems simpler to just remove.
Dan Fuller 2 years ago
parent
commit
a0ffe86741
2 changed files with 1 additions and 42 deletions
  1. 1 24
      src/sentry/incidents/logic.py
  2. 0 18
      tests/sentry/incidents/test_logic.py

+ 1 - 24
src/sentry/incidents/logic.py

@@ -37,13 +37,8 @@ from sentry.incidents.models import (
 from sentry.models import Integration, PagerDutyService, Project, SentryApp
 from sentry.search.events.builder import QueryBuilder
 from sentry.search.events.fields import resolve_field
-from sentry.search.events.filter import get_filter
 from sentry.shared_integrations.exceptions import DuplicateDisplayNameError
-from sentry.snuba.entity_subscription import (
-    ALERT_BLOCKED_FIELDS,
-    EntitySubscription,
-    get_entity_subscription_for_dataset,
-)
+from sentry.snuba.entity_subscription import EntitySubscription, get_entity_subscription_for_dataset
 from sentry.snuba.models import QueryDatasets
 from sentry.snuba.subscriptions import (
     bulk_create_snuba_subscriptions,
@@ -561,7 +556,6 @@ def create_alert_rule(
         # Since comparison alerts make twice as many queries, run the queries less frequently.
         resolution = DEFAULT_CMP_ALERT_RULE_RESOLUTION
         comparison_delta = int(timedelta(minutes=comparison_delta).total_seconds())
-    validate_alert_rule_query(query, organization, projects)
     if dataset == QueryDatasets.SESSIONS and features.has(
         "organizations:alert-crash-free-metrics", organization, actor=user
     ):
@@ -712,9 +706,6 @@ def update_alert_rule(
     if name:
         updated_fields["name"] = name
     if query is not None:
-        validate_alert_rule_query(
-            query, alert_rule.organization, projects if projects is not None else []
-        )
         updated_query_fields["query"] = query
     if aggregate is not None:
         updated_query_fields["aggregate"] = aggregate
@@ -911,20 +902,6 @@ def delete_alert_rule(alert_rule, user=None):
         tasks.auto_resolve_snapshot_incidents.apply_async(kwargs={"alert_rule_id": alert_rule.id})
 
 
-def validate_alert_rule_query(query, organization, projects):
-    # TODO: We should add more validation here to reject queries that include
-    # fields that are invalid in alert rules. For now this will just make sure
-    # the query parses correctly.
-    get_filter(
-        query,
-        params={
-            "organization_id": organization.id,
-            "project_id": [p.id for p in projects],
-        },
-        parser_config_overrides={"blocked_keys": ALERT_BLOCKED_FIELDS},
-    )
-
-
 def get_excluded_projects_for_alert_rule(alert_rule):
     return AlertRuleExcludedProjects.objects.filter(alert_rule=alert_rule)
 

+ 0 - 18
tests/sentry/incidents/test_logic.py

@@ -10,7 +10,6 @@ from exam import fixture, patcher
 from freezegun import freeze_time
 
 from sentry.constants import ObjectStatus
-from sentry.exceptions import InvalidSearchQuery
 from sentry.incidents.events import (
     IncidentCommentCreatedEvent,
     IncidentCreatedEvent,
@@ -496,19 +495,6 @@ class CreateAlertRuleTest(TestCase, BaseIncidentsTest):
         assert alert_rule.snuba_query.subscriptions.get().project == new_project
         assert alert_rule.include_all_projects == include_all_projects
 
-    def test_invalid_query(self):
-        with pytest.raises(InvalidSearchQuery):
-            create_alert_rule(
-                self.organization,
-                [self.project],
-                "hi",
-                "has:",
-                "count()",
-                1,
-                AlertRuleThresholdType.ABOVE,
-                1,
-            )
-
     # This test will fail unless real migrations are run. Refer to migration 0061.
     @pytest.mark.skipif(
         not settings.MIGRATIONS_TEST_MIGRATE, reason="requires custom migration 0061"
@@ -671,10 +657,6 @@ class UpdateAlertRuleTest(TestCase, BaseIncidentsTest):
         alert_rule = update_alert_rule(self.alert_rule, query="")
         assert alert_rule.snuba_query.query == ""
 
-    def test_invalid_query(self):
-        with pytest.raises(InvalidSearchQuery):
-            update_alert_rule(self.alert_rule, query="has:")
-
     def test_delete_projects(self):
         alert_rule = self.create_alert_rule(
             projects=[self.project, self.create_project(fire_project_created=True)]