fix: detect JSON unmarshal errors in responses (#157)

* fix: detect JSON unmarshal errors in responses

This commit fixes BaseService.send() so that it
will now  detect a JSON parsing error when processing
a success (2xx) response.
If, in fact, a JSON parsing error occurs while
trying to process a JSON response body,
then a requests.exceptions.JSONDecodeError is
wrapped in an ApiException and propagated back to the caller.

Signed-off-by: Phil Adams <phil_adams@us.ibm.com>

ibm_cloud_sdk_core/base_service.py

@@ -25,13 +25,15 @@
 
 import requests
 from requests.structures import CaseInsensitiveDict
+from requests.exceptions import JSONDecodeError
 
 from ibm_cloud_sdk_core.authenticators import Authenticator
 from .api_exception import ApiException
 from .detailed_response import DetailedResponse
 from .token_managers.token_manager import TokenManager
 from .utils import (
     has_bad_first_or_last_char,
+    is_json_mimetype,
     remove_null_values,
     cleanup_values,
     read_external_sources,
@@ -310,21 +312,32 @@ def send(self, request: requests.Request, **kwargs) -> DetailedResponse:
         try:
             response = self.http_client.request(**request, cookies=self.jar, **kwargs)
 
+            # Process a "success" response.
             if 200 <= response.status_code <= 299:
                 if response.status_code == 204 or request['method'] == 'HEAD':
-                    # There is no body content for a HEAD request or a 204 response
+                    # There is no body content for a HEAD response or a 204 response.
                     result = None
                 elif stream_response:
                     result = response
                 elif not response.text:
                     result = None
-                else:
+                elif is_json_mimetype(response.headers.get('Content-Type')):
+                    # If this is a JSON response, then try to unmarshal it.
                     try:
                         result = response.json()
-                    except:
-                        result = response
+                    except JSONDecodeError as err:
+                        raise ApiException(
+                            code=response.status_code,
+                            http_response=response,
+                            message='Error processing the HTTP response',
+                        ) from err
+                else:
+                    # Non-JSON response, just use response body as-is.
+                    result = response
+
                 return DetailedResponse(response=result, headers=response.headers, status_code=response.status_code)
 
+            # Received error status code from server, raise an APIException.
             raise ApiException(response.status_code, http_response=response)
         except requests.exceptions.SSLError:
             logger.exception(self.ERROR_MSG_DISABLE_SSL)

ibm_cloud_sdk_core/detailed_response.py

@@ -26,7 +26,7 @@ class DetailedResponse:
     Keyword Args:
         response: The response to the service request, defaults to None.
         headers: The headers of the response, defaults to None.
-        status_code: The status code of there response, defaults to None.
+        status_code: The status code of the response, defaults to None.
 
     Attributes:
         result (dict, requests.Response, None): The response to the service request.

ibm_cloud_sdk_core/utils.py

@@ -16,6 +16,7 @@
 # from ibm_cloud_sdk_core.authenticators import Authenticator
 import datetime
 import json as json_import
+import re
 import ssl
 from os import getenv, environ, getcwd
 from os.path import isfile, join, expanduser
@@ -395,3 +396,19 @@ def __read_from_vcap_services(service_name: str) -> dict:
                 new_vcap_creds['APIKEY'] = vcap_service_credentials.get('apikey')
                 vcap_service_credentials = new_vcap_creds
     return vcap_service_credentials
+
+
+# A regex that matches an "application/json" mimetype.
+json_mimetype_pattern = re.compile('^application/json(\\s*;.*)?$')
+
+
+def is_json_mimetype(mimetype: str) -> bool:
+    """Returns true if 'mimetype' is a JSON-like mimetype, false otherwise.
+
+    Args:
+        mimetype: The mimetype to check.
+
+    Returns:
+        true if mimetype is a JSON-line mimetype, false otherwise.
+    """
+    return mimetype is not None and json_mimetype_pattern.match(mimetype) is not None

test/test_base_service.py

@@ -345,19 +345,20 @@ def test_has_bad_first_or_last_char():
 
 @responses.activate
 def test_request_server_error():
-    responses.add(
-        responses.GET,
-        'https://gateway.watsonplatform.net/test/api',
-        status=500,
-        body=json.dumps({'error': 'internal server error'}),
-        content_type='application/json',
-    )
-    service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
-    try:
+    with pytest.raises(ApiException, match=r'internal server error') as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=500,
+            body=json.dumps({'error': 'internal server error'}),
+            content_type='application/json',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
         prepped = service.prepare_request('GET', url='')
         service.send(prepped)
-    except ApiException as err:
-        assert err.message == 'internal server error'
+    assert err.value.code == 500
+    assert err.value.http_response.headers['Content-Type'] == 'application/json'
+    assert err.value.message == 'internal server error'
 
 
 @responses.activate
@@ -382,6 +383,27 @@ def test_request_success_json():
     assert detailed_response.get_result() == {'foo': 'bar'}
 
 
+@responses.activate
+def test_request_success_invalid_json():
+    # expect an ApiException with JSONDecodeError as the cause when a "success"
+    # response contains invalid JSON in response body.
+    with pytest.raises(ApiException, match=r'Error processing the HTTP response') as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=200,
+            body='{ "invalid": "json", "response"',
+            content_type='application/json; charset=utf8',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
+        prepped = service.prepare_request('GET', url='')
+        service.send(prepped)
+    assert err.value.code == 200
+    assert err.value.http_response.headers['Content-Type'] == 'application/json; charset=utf8'
+    assert isinstance(err.value.__cause__, requests.exceptions.JSONDecodeError)
+    assert "Expecting ':' delimiter: line 1" in str(err.value.__cause__)
+
+
 @responses.activate
 def test_request_success_response():
     responses.add(
@@ -398,20 +420,101 @@ def test_request_success_response():
 
 
 @responses.activate
-def test_request_fail_401():
+def test_request_success_nonjson():
     responses.add(
         responses.GET,
         'https://gateway.watsonplatform.net/test/api',
-        status=401,
-        body=json.dumps({'foo': 'bar'}),
-        content_type='application/json',
+        status=200,
+        body='<h1>Hola, amigo!</h1>',
+        content_type='text/html',
     )
     service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
-    try:
+    prepped = service.prepare_request('GET', url='')
+    detailed_response = service.send(prepped)
+    # It's odd that we have to call ".text" to get the string value
+    # (see issue 3557)
+    assert detailed_response.get_result().text == '<h1>Hola, amigo!</h1>'
+
+
+@responses.activate
+def test_request_fail_401_nonerror_json():
+    # response body not an error object, so we expect the default error message.
+    error_msg = 'Unauthorized: Access is denied due to invalid credentials'
+    with pytest.raises(ApiException, match=error_msg) as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=401,
+            body=json.dumps({'foo': 'bar'}),
+            content_type='application/json',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
+        prepped = service.prepare_request('GET', url='')
+        service.send(prepped)
+    assert err.value.code == 401
+    assert err.value.http_response.headers['Content-Type'] == 'application/json'
+    assert err.value.message == error_msg
+
+
+@responses.activate
+def test_request_fail_401_error_json():
+    # response body is an error object, so we expect to get the message from there.
+    error_msg = 'You dont need to know...'
+    with pytest.raises(ApiException, match=error_msg) as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=401,
+            body=json.dumps({'message': error_msg}),
+            content_type='application/json',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
+        prepped = service.prepare_request('GET', url='')
+        service.send(prepped)
+    assert err.value.code == 401
+    assert err.value.http_response.headers['Content-Type'] == 'application/json'
+    assert err.value.message == error_msg
+
+
+@responses.activate
+def test_request_fail_401_nonjson():
+    response_body = 'You dont have a need to know...'
+    with pytest.raises(ApiException, match=response_body) as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=401,
+            body=response_body,
+            content_type='text/plain',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
+        prepped = service.prepare_request('GET', url='')
+        service.send(prepped)
+    assert err.value.code == 401
+    assert err.value.http_response.headers['Content-Type'] == 'text/plain'
+    assert err.value.message == response_body
+
+
+@responses.activate
+def test_request_fail_401_badjson():
+    # if an error response contains invalid JSON, then we should
+    # end up with 'Unknown error' as the message since we couldn't get
+    # the actual error message from the response body.
+    response_body = 'This is not a JSON object'
+    with pytest.raises(ApiException, match=response_body) as err:
+        responses.add(
+            responses.GET,
+            'https://gateway.watsonplatform.net/test/api',
+            status=401,
+            body=response_body,
+            content_type='application/json',
+        )
+        service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
         prepped = service.prepare_request('GET', url='')
         service.send(prepped)
-    except ApiException as err:
-        assert err.message == 'Unauthorized: Access is denied due to invalid credentials'
+    assert err.value.code == 401
+    assert err.value.http_response.headers['Content-Type'] == 'application/json'
+    assert err.value.message == response_body
 
 
 def test_misc_methods():

test/test_utils.py

@@ -28,7 +28,7 @@
 from ibm_cloud_sdk_core import get_query_param
 from ibm_cloud_sdk_core import read_external_sources
 from ibm_cloud_sdk_core.authenticators import Authenticator, BasicAuthenticator, IAMAuthenticator
-from ibm_cloud_sdk_core.utils import strip_extra_slashes
+from ibm_cloud_sdk_core.utils import strip_extra_slashes, is_json_mimetype
 
 
 def datetime_test(datestr: str, expected: str):
@@ -605,3 +605,15 @@ def test_strip_extra_slashes():
     assert strip_extra_slashes('https://host/path//') == 'https://host/path/'
     assert strip_extra_slashes('https://host//path//') == 'https://host//path/'
     assert strip_extra_slashes('https://host//path//////////') == 'https://host//path/'
+
+
+def test_is_json_mimetype():
+    assert is_json_mimetype(None) is False
+    assert is_json_mimetype('') is False
+    assert is_json_mimetype('application/octet-stream') is False
+    assert is_json_mimetype('ApPlIcAtION/JsoN') is False
+    assert is_json_mimetype('applicaiton/json; charset=utf8') is False
+    assert is_json_mimetype('fooapplication/json; charset=utf8; foo=bar') is False
+
+    assert is_json_mimetype('application/json') is True
+    assert is_json_mimetype('application/json; charset=utf8') is True