From 0753f8d0d15bf1df68c2152848e9d34ef4bfde1f Mon Sep 17 00:00:00 2001
From: Will Kahn-Greene <willkg@users.noreply.github.com>
Date: Sat, 2 Nov 2024 14:05:18 -0400
Subject: [PATCH] bug-1892787: handle case where Bugzilla returns bad payload
 (#6785)

Sometimes Bugzilla returns an HTTP 200 with JSON content, but it doesn't
have "bugs" in it.

Rather than throw an HTTP 500 because of an unhandled KeyError, this
changes the code to raise a BugzillaRestHTTPUnexpectedError with the
payload it did get. This will give us a better idea of what's happening
so we can figure out what to do about it if anything.
---
 webapp/crashstats/crashstats/models.py         | 17 +++++++++++++++--
 .../crashstats/crashstats/tests/test_models.py | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/webapp/crashstats/crashstats/models.py b/webapp/crashstats/crashstats/models.py
index a2f0267970..c428280a02 100644
--- a/webapp/crashstats/crashstats/models.py
+++ b/webapp/crashstats/crashstats/models.py
@@ -1033,12 +1033,25 @@ def get(self, bugs, **kwargs):
             )
             response = session.get(url, headers=headers)
             if response.status_code != 200:
-                raise BugzillaRestHTTPUnexpectedError(response.status_code)
+                raise BugzillaRestHTTPUnexpectedError(
+                    f"status code: {response.status_code}"
+                )
+
+            data = response.json()
+            if "bugs" not in data:
+                # We know the payload is JSON, but we don't know what shape it is--could
+                # be an array or an object. We know it doesn't have sensitive data in
+                # it, so let's dump to a string and truncate that to 100 characters.
+                payload_data = response.content[:100]
+                raise BugzillaRestHTTPUnexpectedError(
+                    f"status code: {response.status_code}, payload: {payload_data!r}"
+                )
 
-            for each in response.json()["bugs"]:
+            for each in data["bugs"]:
                 cache_key = self.make_cache_key(each["id"])
                 cache.set(cache_key, each, self.BUG_CACHE_SECONDS)
                 results.append(each)
+
         return {"bugs": results}
 
     post = None
diff --git a/webapp/crashstats/crashstats/tests/test_models.py b/webapp/crashstats/crashstats/tests/test_models.py
index 633e14a419..0385b99d01 100644
--- a/webapp/crashstats/crashstats/tests/test_models.py
+++ b/webapp/crashstats/crashstats/tests/test_models.py
@@ -368,6 +368,24 @@ def mocked_get(url, **options):
         with pytest.raises(models.BugzillaRestHTTPUnexpectedError):
             api.get("123456789")
 
+    @mock.patch("requests.Session")
+    def test_bugzilla_api_no_bugs(self, rsession):
+        model = models.BugzillaBugInfo
+
+        api = model()
+
+        def mocked_get(url, **options):
+            # NOTE(willkg): Bugzilla sometimes returns an HTTP 200 with a JSON payload
+            # that has no "bugs" key. We don't know what is in it, but we do know it has
+            # no "bugs".
+            return Response({"otherkey": "othervalue"})
+
+        rsession().get.side_effect = mocked_get
+        with pytest.raises(models.BugzillaRestHTTPUnexpectedError) as excinfo:
+            api.get("123456789")
+
+        assert 'payload: \'{"otherkey": "othervalue"}\'' in str(excinfo.value)
+
     @mock.patch("requests.Session")
     def test_massive_querystring_caching(self, rsession):
         # doesn't actually matter so much what API model we use