Avoid hiding error info in non-compliant error responses

Author: soxofaan

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   Although removed from openeo-processes 2.0.0-rc1, support for `load_result`, `predict_random_forest` and `load_ml_model`
   is preserved but deprecated. ([#424](https://github.com/Open-EO/openeo-python-client/issues/424))
 - Show more informative error message on `403 Forbidden` errors from CDSE firewall ([#512](https://github.com/Open-EO/openeo-python-client/issues/512))
+- Handle API error responses more strict and avoid hiding possibly important information in JSON-formatted but non-compliant error responses.
 
 ### Removed
 

openeo/rest/__init__.py

@@ -74,9 +74,10 @@ class OpenEoApiError(OpenEoApiPlainError):
 
     def __init__(
         self,
-        http_status_code: Optional[int] = None,
-        code: str = "unknown",
-        message: str = "unknown error",
+        *,
+        http_status_code: int,
+        code: str,
+        message: str,
         id: Optional[str] = None,
         url: Optional[str] = None,
     ):

openeo/rest/connection.py

@@ -176,28 +176,37 @@ def _raise_api_error(self, response: requests.Response):
         """Convert API error response to Python exception"""
         status_code = response.status_code
         try:
-            # Try parsing the error info according to spec and wrap it in an exception.
             info = response.json()
-            exception = OpenEoApiError(
-                http_status_code=status_code,
-                code=info.get("code", "unknown"),
-                message=info.get("message", f"unknown error ({repr_truncate(info, width=200)})"),
-                id=info.get("id"),
-                url=info.get("url"),
-            )
         except Exception:
-            # Parsing of error info went wrong: let's see if we can still extract some helpful information.
-            text = response.text
-            error_message = None
-            _log.warning(f"Failed to parse API error response: [{status_code}] {text!r} (headers: {response.headers})")
-            if status_code == 502 and "Proxy Error" in text:
-                error_message = (
-                    "Received 502 Proxy Error."
-                    " This typically happens when a synchronous openEO processing request takes too long and is aborted."
-                    " Consider using a batch job instead."
+            info = None
+
+        # Valid JSON object with "code" and "message" fields indicates a proper openEO API error.
+        if isinstance(info, dict):
+            error_code = info.get("code")
+            error_message = info.get("message")
+            if error_code and isinstance(error_code, str) and error_message and isinstance(error_message, str):
+                raise OpenEoApiError(
+                    http_status_code=status_code,
+                    code=error_code,
+                    message=error_message,
+                    id=info.get("id"),
+                    url=info.get("url"),
                 )
-            exception = OpenEoApiPlainError(message=text, http_status_code=status_code, error_message=error_message)
-        raise exception
+
+        # Failed to parse it as a compliant openEO API error: show body as-is in the exception.
+        text = response.text
+        error_message = None
+        _log.warning(f"Failed to parse API error response: [{status_code}] {text!r} (headers: {response.headers})")
+
+        # TODO: eliminate this VITO-backend specific error massaging?
+        if status_code == 502 and "Proxy Error" in text:
+            error_message = (
+                "Received 502 Proxy Error."
+                " This typically happens when a synchronous openEO processing request takes too long and is aborted."
+                " Consider using a batch job instead."
+            )
+
+        raise OpenEoApiPlainError(message=text, http_status_code=status_code, error_message=error_message)
 
     def get(self, path: str, stream: bool = False, auth: Optional[AuthBase] = None, **kwargs) -> Response:
         """

tests/rest/test_connection.py

@@ -130,6 +130,28 @@ def test_rest_api_expected_status_with_error(requests_mock):
         conn.get("/bar", check_error=False, expected_status=[302, 303])
 
 
+@pytest.mark.parametrize(
+    ["status_code", "text", "expected"],
+    [
+        (400, "Nope, no JSON here", "[400] Nope, no JSON here"),
+        (400, '{"code": "InvalidJSON", "message": "oops}}}}}}', '[400] {"code": "InvalidJSON", "message": "oops}}}}}}'),
+        (400, '{"status": "error", "msg": "Nope"}', '[400] {"status": "error", "msg": "Nope"}'),
+        (500, '{"status": "error", "msg": "Nope"}', '[500] {"status": "error", "msg": "Nope"}'),
+        (400, '{"status": "error", "code": "WrongColor"}', '[400] {"status": "error", "code": "WrongColor"}'),
+        (400, '{"code": "WrongColor", "message": "Nope"}', "[400] WrongColor: Nope"),
+        (400, '{"code": "WrongColor", "message": "Nope", "id": "r-123"}', "[400] WrongColor: Nope (ref: r-123)"),
+    ],
+)
+def test_error_response_format(requests_mock, status_code, text, expected):
+    requests_mock.get("https://oeo.test/bar", status_code=status_code, text=text)
+    conn = RestApiConnection(API_URL)
+    with pytest.raises(
+        OpenEoApiPlainError,
+        match=re.escape(expected) if isinstance(expected, str) else expected,
+    ):
+        conn.get("/bar")
+
+
 def test_502_proxy_error(requests_mock):
     """EP-3387"""
     requests_mock.get("https://oeo.test/bar", status_code=502, text="""<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
@@ -157,7 +179,7 @@ def test_cdse_firewall_error(requests_mock):
     conn = RestApiConnection(API_URL)
     with pytest.raises(
         OpenEoApiPlainError,
-        match=r"\[403\] unknown: unknown error.*This request was rejected.*provide reference ID 128968",
+        match=r"\[403\].*This request was rejected.*provide reference ID 128968",
     ):
         conn.get("/bar")