ref(toolbar): check queryReferrer from query param and refactor tests for readability

src/sentry/api/analytics.py

@@ -39,15 +39,15 @@ class DevToolbarApiRequestEvent(analytics.Event):
     attributes = (
         analytics.Attribute("view_name"),
         analytics.Attribute("route"),
-        analytics.Attribute("query_string"),
-        analytics.Attribute("origin"),
+        analytics.Attribute("query_string", required=False),
+        analytics.Attribute("origin", required=False),
         analytics.Attribute("method"),
         analytics.Attribute("status_code", type=int),
         analytics.Attribute("organization_id", type=int, required=False),
         analytics.Attribute("organization_slug", required=False),
         analytics.Attribute("project_id", type=int, required=False),
         analytics.Attribute("project_slug", required=False),
-        analytics.Attribute("user_id", type=int),
+        analytics.Attribute("user_id", type=int, required=False),
     )
 
 

src/sentry/middleware/devtoolbar.py

@@ -17,8 +17,8 @@ def __init__(self, get_response):
     def __call__(self, request):
         response = self.get_response(request)
         try:
-            # note ordering of conditions to avoid extra option queries.
-            if request.headers.get("queryReferrer") == "devtoolbar" and options.get(
+            # Note ordering of conditions to reduce option queries. GET contains the query params, regardless of method.
+            if request.GET.get("queryReferrer") == "devtoolbar" and options.get(
                 "devtoolbar.analytics.enabled"
             ):
                 _record_api_request(request, response)
@@ -45,7 +45,8 @@ def _record_api_request(request: HttpRequest, response: HttpResponse) -> None:
     project_id, project_slug = parse_id_or_slug_param(project_id_or_slug)
 
     origin = origin_from_request(request)
-    query_string: str = get_query_string(request)  # starts with '?'
+    query_string: str = get_query_string(request)  # starts with ? if non-empty
+
     analytics.record(
         "devtoolbar.api_request",
         view_name=view_name,

tests/sentry/middleware/test_devtoolbar.py

@@ -21,50 +21,35 @@ def factory(self):
 
     def setUp(self):
         # Allows changing the get_response mock for each test.
-        self.get_response = MagicMock(return_value=HttpResponse(status=200))
-        self.middleware.get_response = self.get_response
-
-    def make_toolbar_request(
-        self,
-        path="/",
-        method="GET",
-        headers=None,
-        incl_toolbar_header=True,
-        resolver_match="mock",
-    ):
-        headers = headers or {}
-        if incl_toolbar_header:
-            headers["queryReferrer"] = "devtoolbar"
-        request = getattr(self.factory, method.lower())(path, headers=headers)
-        request.resolver_match = MagicMock() if resolver_match == "mock" else resolver_match
-        return request
+        self.middleware.get_response = MagicMock(return_value=HttpResponse(status=200))
 
     @override_options({"devtoolbar.analytics.enabled": True})
     @patch("sentry.analytics.record")
     def test_basic(self, mock_record: MagicMock):
-        request = self.make_toolbar_request()
+        request = self.factory.get("/?queryReferrer=devtoolbar")
+        request.resolver_match = MagicMock()
         self.middleware(request)
         mock_record.assert_called()
         assert mock_record.call_args[0][0] == self.analytics_event_name
 
     @override_options({"devtoolbar.analytics.enabled": True})
     @patch("sentry.analytics.record")
     def test_no_devtoolbar_header(self, mock_record: MagicMock):
-        request = self.make_toolbar_request(incl_toolbar_header=False)
+        request = self.factory.get("/")
+        request.resolver_match = MagicMock()
         self.middleware(request)
         mock_record.assert_not_called()
 
-        request = self.make_toolbar_request(
-            headers={"queryReferrer": "not-toolbar"}, incl_toolbar_header=False
-        )
+        request = self.factory.get("/?queryReferrer=not-toolbar")
+        request.resolver_match = MagicMock()
         self.middleware(request)
         mock_record.assert_not_called()
 
     @override_options({"devtoolbar.analytics.enabled": True})
     @patch("sentry.middleware.devtoolbar.logger.exception")
     @patch("sentry.analytics.record")
     def test_request_not_resolved(self, mock_record: MagicMock, mock_logger: MagicMock):
-        request = self.make_toolbar_request()
+        request = self.factory.get("/?queryReferrer=devtoolbar")
         request.resolver_match = None
         self.middleware(request)
 
@@ -81,9 +66,8 @@ def test_view_name_and_route(self, mock_record: MagicMock):
         # Integration tests do a better job of testing these fields, since they involve route resolver.
         view_name = "my-endpoint"
         route = "/issues/(?P<issue_id>)/"
-        request = self.make_toolbar_request(
-            resolver_match=MagicMock(view_name=view_name, route=route),
-        )
+        request = self.factory.get("/?queryReferrer=devtoolbar")
+        request.resolver_match = MagicMock(view_name=view_name, route=route)
         self.middleware(request)
 
         mock_record.assert_called()
@@ -94,10 +78,9 @@ def test_view_name_and_route(self, mock_record: MagicMock):
     @override_options({"devtoolbar.analytics.enabled": True})
     @patch("sentry.analytics.record")
     def test_query_string(self, mock_record: MagicMock):
-        query = "?a=b&statsPeriod=14d"
-        request = self.make_toolbar_request(
-            path="https://sentry.io/replays/" + query,
-        )
+        query = "?a=b&statsPeriod=14d&queryReferrer=devtoolbar"
+        request = self.factory.get("/" + query)
+        request.resolver_match = MagicMock()
         self.middleware(request)
 
         mock_record.assert_called()
@@ -108,7 +91,10 @@ def test_query_string(self, mock_record: MagicMock):
     @patch("sentry.analytics.record")
     def test_origin(self, mock_record: MagicMock):
         origin = "https://potato.com"
-        request = self.make_toolbar_request(headers={"Origin": origin})
+        request = self.factory.get(
+            f"{origin}/?queryReferrer=devtoolbar", headers={"Origin": origin}
+        )
+        request.resolver_match = MagicMock()
         self.middleware(request)
 
         mock_record.assert_called()
@@ -119,7 +105,9 @@ def test_origin(self, mock_record: MagicMock):
     @patch("sentry.analytics.record")
     def test_origin_from_referrer(self, mock_record: MagicMock):
         origin = "https://potato.com"
-        request = self.make_toolbar_request(headers={"Referer": origin + "/issues/?a=b"})
+        url = origin + "/issues/?a=b&queryReferrer=devtoolbar"
+        request = self.factory.get(url, headers={"Referer": url})
+        request.resolver_match = MagicMock()
         self.middleware(request)
 
         mock_record.assert_called()
@@ -129,8 +117,9 @@ def test_origin_from_referrer(self, mock_record: MagicMock):
     @override_options({"devtoolbar.analytics.enabled": True})
     @patch("sentry.analytics.record")
     def test_response_status_code(self, mock_record: MagicMock):
-        request = self.make_toolbar_request()
-        self.get_response.return_value = HttpResponse(status=420)
+        request = self.factory.get("/?queryReferrer=devtoolbar")
+        request.resolver_match = MagicMock()
+        self.middleware.get_response.return_value = HttpResponse(status=420)
         self.middleware(request)
 
         mock_record.assert_called()
@@ -141,7 +130,8 @@ def test_response_status_code(self, mock_record: MagicMock):
     @patch("sentry.analytics.record")
     def test_methods(self, mock_record: MagicMock):
         for method in ["GET", "POST", "PUT", "DELETE"]:
-            request = self.make_toolbar_request(method=method)
+            request = getattr(self.factory, method.lower())("/?queryReferrer=devtoolbar")
+            request.resolver_match = MagicMock()
             self.middleware(request)
 
             mock_record.assert_called()
@@ -206,15 +196,15 @@ def _test_endpoint(
     def test_organization_replays(self):
         self._test_endpoint(
             f"/api/0/organizations/{self.organization.slug}/replays/",
-            "?field=id",
+            "?field=id&queryReferrer=devtoolbar",
             "GET",
             "sentry-api-0-organization-replay-index",
             "^api/0/organizations/(?P<organization_id_or_slug>[^\\/]+)/replays/$",
             expected_org_slug=self.organization.slug,
         )
         self._test_endpoint(
             f"/api/0/organizations/{self.organization.id}/replays/",
-            "?field=id",
+            "?queryReferrer=devtoolbar&field=id",
             "GET",
             "sentry-api-0-organization-replay-index",
             "^api/0/organizations/(?P<organization_id_or_slug>[^\\/]+)/replays/$",
@@ -225,7 +215,7 @@ def test_group_details(self):
         group = self.create_group(substatus=GroupSubStatus.NEW)
         self._test_endpoint(
             f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/",
-            "",
+            "?queryReferrer=devtoolbar",
             "GET",
             "sentry-api-0-organization-group-group-details",
             "^api/0/organizations/(?P<organization_id_or_slug>[^\\/]+)/(?:issues|groups)/(?P<issue_id>[^\\/]+)/$",
@@ -236,7 +226,7 @@ def test_project_user_feedback(self):
         # Should return 400 (no POST data)
         self._test_endpoint(
             f"/api/0/projects/{self.organization.slug}/{self.project.id}/user-feedback/",
-            "",
+            "?queryReferrer=devtoolbar",
             "POST",
             "sentry-api-0-project-user-reports",
             r"^api/0/projects/(?P<organization_id_or_slug>[^\/]+)/(?P<project_id_or_slug>[^\/]+)/(?:user-feedback|user-reports)/$",