From 0da7d945b8c3ee3c453d3ff80ae0f45ab166fe4b Mon Sep 17 00:00:00 2001
From: Kohsuke Kawaguchi <kk@kohsuke.org>
Date: Tue, 5 Nov 2024 14:54:11 -0800
Subject: [PATCH] Make sure to report server side error message

---
 launchable/commands/subset.py   |  2 +-
 launchable/utils/http_client.py | 12 ++++++++++++
 tests/utils/test_http_client.py | 18 ++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/launchable/commands/subset.py b/launchable/commands/subset.py
index 7a1c1524d..9eaa01cee 100644
--- a/launchable/commands/subset.py
+++ b/launchable/commands/subset.py
@@ -485,7 +485,7 @@ def run(self):
 
                     # The status code 422 is returned when validation error of the test mapping file occurs.
                     if res.status_code == 422:
-                        msg = "Error: {}".format(res.json().get("reason"))
+                        msg = "Error: {}".format(res.reason)
                         tracking_client.send_error_event(
                             event_name=Tracking.ErrorEvent.USER_ERROR,
                             stack_trace=msg,
diff --git a/launchable/utils/http_client.py b/launchable/utils/http_client.py
index 9cdb403ed..c5d3eeca9 100644
--- a/launchable/utils/http_client.py
+++ b/launchable/utils/http_client.py
@@ -104,6 +104,18 @@ def request(
             "received response status:{} message:{} headers:{}".format(response.status_code, response.reason,
                                                                        response.headers)
         )
+
+        # because (I believe, though I could be wrong) HTTP/2 got rid of status message, our server side HTTP stack
+        # doesn't let us forward the status message (=response.reason), which would have been otherwise a very handy
+        # mechanism to reliably forward error messages. So instead, we forward JSON error response of the form
+        # {"reason": ...}. Backfill response.reason with this JSON error message if it exists, so that the exceptions
+        # thrown from response.raise_for_status() will have a meaningful message.
+        if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"):
+            try:
+                response.reason = response.json().get("reason", response.reason)
+            except json.JSONDecodeError:
+                pass
+
         return response
 
     def _headers(self, compress):
diff --git a/tests/utils/test_http_client.py b/tests/utils/test_http_client.py
index aadb886ce..a565e2286 100644
--- a/tests/utils/test_http_client.py
+++ b/tests/utils/test_http_client.py
@@ -2,6 +2,8 @@
 import platform
 from unittest import TestCase, mock
 
+from requests import Session
+
 from launchable.utils.http_client import _HttpClient
 from launchable.version import __version__
 
@@ -43,3 +45,19 @@ def test_header(self):
                 "dummy",
             ),
         })
+
+    def test_reason(self):
+        '''make sure we correctly propagate error message from the server'''
+
+        # use new session to disable retry
+        cli = _HttpClient(session=Session())
+        # /error is an actual endpoint that exists on our service to test the behavior
+        res = cli.request("GET", "intake/error")
+        self.assertEqual(res.status_code, 500)
+        self.assertEqual(res.reason, "Welp")
+
+        try:
+            res.raise_for_status()
+            self.fail("should have raised")
+        except Exception as e:
+            self.assertIn("Welp", str(e))