Browse Source

fix(discover): Has issue filter for transactions was incorrect (#23952)

The has:issue filter for transactions was incorrect. The issue column on
transactions is inconsistent. It returns NULL on the events table and 0 on the
transactions table. This change updates the condition to account for this.
Tony 4 years ago
parent
commit
69839faaa5

+ 11 - 3
src/sentry/api/event_search.py

@@ -844,10 +844,18 @@ def convert_search_filter_to_snuba_query(search_filter, key=None, params=None):
         # Handle "has" queries
         if search_filter.value.raw_value == "":
             if search_filter.operator == "=":
-                # Use isNull to get events with no issue (transactions)
-                return [["isNull", [name]], search_filter.operator, 1]
+                # The state of having no issues is represented differently on transactions vs
+                # other events. On the transactions table, it is represented by 0 whereas it is
+                # represented by NULL everywhere else. This means we have to check for both 0
+                # or NULL.
+                return [
+                    [["isNull", [name]], search_filter.operator, 1],
+                    [name, search_filter.operator, 0],
+                ]
             else:
-                # Compare to 0 as group_id is not nullable on issues.
+                # Based the reasoning above, we should be checking that it is not 0 and not NULL.
+                # However, because NULL != 0 evaluates to NULL in Clickhouse, we can simplify it
+                # to check just not 0.
                 return [name, search_filter.operator, 0]
 
         # Skip isNull check on group_id value as we want to

+ 16 - 2
tests/sentry/api/test_event_search.py

@@ -1739,6 +1739,18 @@ class GetSnubaQueryArgsTest(TestCase):
     def test_not_has(self):
         assert get_filter("!has:release").conditions == [[["isNull", ["release"]], "=", 1]]
 
+    def test_has_issue(self):
+        has_issue_filter = get_filter("has:issue")
+        assert has_issue_filter.group_ids == []
+        assert has_issue_filter.conditions == [["issue.id", "!=", 0]]
+
+    def test_not_has_issue(self):
+        has_issue_filter = get_filter("!has:issue")
+        assert has_issue_filter.group_ids == []
+        assert has_issue_filter.conditions == [
+            [[["isNull", ["issue.id"]], "=", 1], ["issue.id", "=", 0]]
+        ]
+
     def test_has_issue_id(self):
         has_issue_filter = get_filter("has:issue.id")
         assert has_issue_filter.group_ids == []
@@ -1747,7 +1759,9 @@ class GetSnubaQueryArgsTest(TestCase):
     def test_not_has_issue_id(self):
         has_issue_filter = get_filter("!has:issue.id")
         assert has_issue_filter.group_ids == []
-        assert has_issue_filter.conditions == [[["isNull", ["issue.id"]], "=", 1]]
+        assert has_issue_filter.conditions == [
+            [[["isNull", ["issue.id"]], "=", 1], ["issue.id", "=", 0]]
+        ]
 
     def test_message_empty(self):
         assert get_filter("has:message").conditions == [[["equals", ["message", ""]], "!=", 1]]
@@ -1818,7 +1832,7 @@ class GetSnubaQueryArgsTest(TestCase):
 
     def test_unknown_issue_filter(self):
         _filter = get_filter("issue:unknown", {"organization_id": self.organization.id})
-        assert _filter.conditions == [[["isNull", ["issue.id"]], "=", 1]]
+        assert _filter.conditions == [[[["isNull", ["issue.id"]], "=", 1], ["issue.id", "=", 0]]]
         assert _filter.filter_keys == {}
         assert _filter.group_ids == []
 

+ 44 - 1
tests/snuba/api/endpoints/test_organization_events_v2.py

@@ -402,16 +402,59 @@ class OrganizationEventsV2EndpointTest(APITestCase, SnubaTestCase):
         self.store_event(data, project_id=project.id)
 
         features = {"organizations:discover-basic": True, "organizations:global-views": True}
+
+        # should only show 1 event of type default
         query = {"field": ["project", "issue"], "query": "has:issue", "statsPeriod": "14d"}
         response = self.do_request(query, features=features)
-
         assert response.status_code == 200, response.content
         assert len(response.data["data"]) == 1
         assert response.data["data"][0]["issue"] == event.group.qualified_short_id
 
+        # should only show 1 event of type transaction since they dont have issues
         query = {"field": ["project", "issue"], "query": "!has:issue", "statsPeriod": "14d"}
         response = self.do_request(query, features=features)
+        assert response.status_code == 200, response.content
+        assert len(response.data["data"]) == 1
+        assert response.data["data"][0]["issue"] == "unknown"
+
+        # should only show 1 event of type default
+        query = {
+            "field": ["project", "issue"],
+            "query": "event.type:default has:issue",
+            "statsPeriod": "14d",
+        }
+        response = self.do_request(query, features=features)
+        assert response.status_code == 200, response.content
+        assert len(response.data["data"]) == 1
+        assert response.data["data"][0]["issue"] == event.group.qualified_short_id
 
+        # should show no results because no the default event has an issue
+        query = {
+            "field": ["project", "issue"],
+            "query": "event.type:default !has:issue",
+            "statsPeriod": "14d",
+        }
+        response = self.do_request(query, features=features)
+        assert response.status_code == 200, response.content
+        assert len(response.data["data"]) == 0
+
+        # should show no results because no transactions have issues
+        query = {
+            "field": ["project", "issue"],
+            "query": "event.type:transaction has:issue",
+            "statsPeriod": "14d",
+        }
+        response = self.do_request(query, features=features)
+        assert response.status_code == 200, response.content
+        assert len(response.data["data"]) == 0
+
+        # should only show 1 event of type transaction since they dont have issues
+        query = {
+            "field": ["project", "issue"],
+            "query": "event.type:transaction !has:issue",
+            "statsPeriod": "14d",
+        }
+        response = self.do_request(query, features=features)
         assert response.status_code == 200, response.content
         assert len(response.data["data"]) == 1
         assert response.data["data"][0]["issue"] == "unknown"