Improve ApiError message formatting; add response field to ApiError and UnexpectedResponseError (linode#467)

* Improve ApiError and UnexpectedResponseError

* make format

* Fix again

* Remove comma from multiline string

Author: lgarber-akamai

linode_api4/errors.py

@@ -1,4 +1,11 @@
+# Necessary to maintain compatibility with Python < 3.11
+from __future__ import annotations
+
 from builtins import super
+from json import JSONDecodeError
+from typing import Any, Dict, Optional
+
+from requests import Response
 
 
 class ApiError(RuntimeError):
@@ -8,14 +15,90 @@ class ApiError(RuntimeError):
     often, this will be caused by invalid input to the API.
     """
 
-    def __init__(self, message, status=400, json=None):
+    def __init__(
+        self,
+        message: str,
+        status: int = 400,
+        json: Optional[Dict[str, Any]] = None,
+        response: Optional[Response] = None,
+    ):
         super().__init__(message)
+
         self.status = status
         self.json = json
+        self.response = response
+
         self.errors = []
+
         if json and "errors" in json and isinstance(json["errors"], list):
             self.errors = [e["reason"] for e in json["errors"]]
 
+    @classmethod
+    def from_response(
+        cls,
+        response: Response,
+        message: Optional[str] = None,
+        disable_formatting: bool = False,
+    ) -> Optional[ApiError]:
+        """
+        Creates an ApiError object from the given response,
+        or None if the response does not contain an error.
+
+        :arg response: The response to create an ApiError from.
+        :arg message: An optional message to prepend to the error's message.
+        :arg disable_formatting: If true, the error's message will not automatically be formatted
+                                 with details from the API response.
+
+        :returns: The new API error.
+        """
+
+        if response.status_code < 400 or response.status_code > 599:
+            # No error was found
+            return None
+
+        request = response.request
+
+        try:
+            response_json = response.json()
+        except JSONDecodeError:
+            response_json = None
+
+        # Use the user-defined message is formatting is disabled
+        if disable_formatting:
+            return cls(
+                message,
+                status=response.status_code,
+                json=response_json,
+                response=response,
+            )
+
+        # Build the error string
+        error_fmt = "N/A"
+
+        if response_json is not None and "errors" in response_json:
+            errors = []
+
+            for error in response_json["errors"]:
+                field = error.get("field")
+                reason = error.get("reason")
+                errors.append(f"{field + ': ' if field else ''}{reason}")
+
+            error_fmt = "; ".join(errors)
+
+        elif len(response.text or "") > 0:
+            error_fmt = response.text
+
+        return cls(
+            (
+                f"{message + ': ' if message is not None else ''}"
+                f"{f'{request.method} {request.path_url}: ' if request else ''}"
+                f"[{response.status_code}] {error_fmt}"
+            ),
+            status=response.status_code,
+            json=response_json,
+            response=response,
+        )
+
 
 class UnexpectedResponseError(RuntimeError):
     """
@@ -26,7 +109,41 @@ class UnexpectedResponseError(RuntimeError):
     library, and should be fixed with changes to this codebase.
     """
 
-    def __init__(self, message, status=200, json=None):
+    def __init__(
+        self,
+        message: str,
+        status: int = 200,
+        json: Optional[Dict[str, Any]] = None,
+        response: Optional[Response] = None,
+    ):
         super().__init__(message)
+
         self.status = status
         self.json = json
+        self.response = response
+
+    @classmethod
+    def from_response(
+        cls,
+        message: str,
+        response: Response,
+    ) -> Optional[UnexpectedResponseError]:
+        """
+        Creates an UnexpectedResponseError object from the given response and message.
+
+        :arg message: The message to create this error with.
+        :arg response: The response to create an UnexpectedResponseError from.
+        :returns: The new UnexpectedResponseError.
+        """
+
+        try:
+            response_json = response.json()
+        except JSONDecodeError:
+            response_json = None
+
+        return cls(
+            message,
+            status=response.status_code,
+            json=response_json,
+            response=response,
+        )

linode_api4/linode_client.py

@@ -287,23 +287,9 @@ def _api_call(
         if warning:
             logger.warning("Received warning from server: {}".format(warning))
 
-        if 399 < response.status_code < 600:
-            j = None
-            error_msg = "{}: ".format(response.status_code)
-            try:
-                j = response.json()
-                if "errors" in j.keys():
-                    for e in j["errors"]:
-                        msg = e.get("reason", "")
-                        field = e.get("field", None)
-
-                        error_msg += "{}{}; ".format(
-                            f"[{field}] " if field is not None else "",
-                            msg,
-                        )
-            except:
-                pass
-            raise ApiError(error_msg, status=response.status_code, json=j)
+        api_error = ApiError.from_response(response)
+        if api_error is not None:
+            raise api_error
 
         if response.status_code != 204:
             j = response.json()

linode_api4/login_client.py

@@ -434,10 +434,9 @@ def oauth_redirect():
         )
 
         if r.status_code != 200:
-            raise ApiError(
-                "OAuth token exchange failed",
-                status=r.status_code,
-                json=r.json(),
+            raise ApiError.from_response(
+                r,
+                message="OAuth token exchange failed",
             )
 
         token = r.json()["access_token"]
@@ -479,7 +478,7 @@ def refresh_oauth_token(self, refresh_token):
         )
 
         if r.status_code != 200:
-            raise ApiError("Refresh failed", r)
+            raise ApiError.from_response(r, message="Refresh failed")
 
         token = r.json()["access_token"]
         scopes = OAuthScopes.parse(r.json()["scopes"])
@@ -516,5 +515,5 @@ def expire_token(self, token):
         )
 
         if r.status_code != 200:
-            raise ApiError("Failed to expire token!", r)
+            raise ApiError.from_response(r, "Failed to expire token!")
         return True

linode_api4/objects/account.py

@@ -436,8 +436,10 @@ def thumbnail(self, dump_to=None):
         )
 
         if not result.status_code == 200:
-            raise ApiError(
-                "No thumbnail found for OAuthClient {}".format(self.id)
+            raise ApiError.from_response(
+                result,
+                "No thumbnail found for OAuthClient {}".format(self.id),
+                disable_formatting=True,
             )
 
         if dump_to:
@@ -472,12 +474,9 @@ def set_thumbnail(self, thumbnail):
             data=thumbnail,
         )
 
-        if not result.status_code == 200:
-            errors = []
-            j = result.json()
-            if "errors" in j:
-                errors = [e["reason"] for e in j["errors"]]
-            raise ApiError("{}: {}".format(result.status_code, errors), json=j)
+        api_exc = ApiError.from_response(result)
+        if api_exc is not None:
+            raise api_exc
 
         return True
 

linode_api4/objects/support.py

@@ -174,12 +174,9 @@ def upload_attachment(self, attachment: Union[Path, str]):
                 files={"file": f},
             )
 
-        if not result.status_code == 200:
-            errors = []
-            j = result.json()
-            if "errors" in j:
-                errors = [e["reason"] for e in j["errors"]]
-            raise ApiError("{}: {}".format(result.status_code, errors), json=j)
+        api_exc = ApiError.from_response(result)
+        if api_exc is not None:
+            raise api_exc
 
         return True
 

test/integration/linode_client/test_errors.py

@@ -0,0 +1,28 @@
+from linode_api4.errors import ApiError
+
+
+def test_error_404(test_linode_client):
+    api_exc = None
+
+    try:
+        test_linode_client.get("/invalid/endpoint")
+    except ApiError as exc:
+        api_exc = exc
+
+    assert str(api_exc) == "GET /v4beta/invalid/endpoint: [404] Not found"
+
+
+def test_error_400(test_linode_client):
+    api_exc = None
+
+    try:
+        test_linode_client.linode.instance_create(
+            "g6-fake-plan", "us-fakeregion"
+        )
+    except ApiError as exc:
+        api_exc = exc
+
+    assert str(api_exc) == (
+        "POST /v4beta/linode/instances: [400] type: A valid plan type by that ID was not found; "
+        "region: region is not valid"
+    )

test/unit/errors_test.py

@@ -0,0 +1,104 @@
+from types import SimpleNamespace
+from unittest import TestCase
+
+from linode_api4.errors import ApiError, UnexpectedResponseError
+
+
+class ApiErrorTest(TestCase):
+    def test_from_response(self):
+        mock_response = SimpleNamespace(
+            status_code=400,
+            json=lambda: {
+                "errors": [
+                    {"reason": "foo"},
+                    {"field": "bar", "reason": "oh no"},
+                ]
+            },
+            text='{"errors": [{"reason": "foo"}, {"field": "bar", "reason": "oh no"}]}',
+            request=SimpleNamespace(
+                method="POST",
+                path_url="foo/bar",
+            ),
+        )
+
+        exc = ApiError.from_response(mock_response)
+
+        assert str(exc) == "POST foo/bar: [400] foo; bar: oh no"
+        assert exc.status == 400
+        assert exc.json == {
+            "errors": [{"reason": "foo"}, {"field": "bar", "reason": "oh no"}]
+        }
+        assert exc.response.request.method == "POST"
+        assert exc.response.request.path_url == "foo/bar"
+
+    def test_from_response_non_json_body(self):
+        mock_response = SimpleNamespace(
+            status_code=500,
+            json=lambda: None,
+            text="foobar",
+            request=SimpleNamespace(
+                method="POST",
+                path_url="foo/bar",
+            ),
+        )
+
+        exc = ApiError.from_response(mock_response)
+
+        assert str(exc) == "POST foo/bar: [500] foobar"
+        assert exc.status == 500
+        assert exc.json is None
+        assert exc.response.request.method == "POST"
+        assert exc.response.request.path_url == "foo/bar"
+
+    def test_from_response_empty_body(self):
+        mock_response = SimpleNamespace(
+            status_code=500,
+            json=lambda: None,
+            text=None,
+            request=SimpleNamespace(
+                method="POST",
+                path_url="foo/bar",
+            ),
+        )
+
+        exc = ApiError.from_response(mock_response)
+
+        assert str(exc) == "POST foo/bar: [500] N/A"
+        assert exc.status == 500
+        assert exc.json is None
+        assert exc.response.request.method == "POST"
+        assert exc.response.request.path_url == "foo/bar"
+
+    def test_from_response_no_request(self):
+        mock_response = SimpleNamespace(
+            status_code=500, json=lambda: None, text="foobar", request=None
+        )
+
+        exc = ApiError.from_response(mock_response)
+
+        assert str(exc) == "[500] foobar"
+        assert exc.status == 500
+        assert exc.json is None
+        assert exc.response.request is None
+
+
+class UnexpectedResponseErrorTest(TestCase):
+    def test_from_response(self):
+        mock_response = SimpleNamespace(
+            status_code=400,
+            json=lambda: {
+                "foo": "bar",
+            },
+            request=SimpleNamespace(
+                method="POST",
+                path_url="foo/bar",
+            ),
+        )
+
+        exc = UnexpectedResponseError.from_response("foobar", mock_response)
+
+        assert str(exc) == "foobar"
+        assert exc.status == 400
+        assert exc.json == {"foo": "bar"}
+        assert exc.response.request.method == "POST"
+        assert exc.response.request.path_url == "foo/bar"