Merge pull request #57 from mattmanley/improve_response_logging

Improve exceptions and fix JSON decoding bug

Author: mattmanley

maproulette/api/errors.py

@@ -3,34 +3,65 @@
 
 class MapRouletteBaseException(Exception):
     """MapRoulette Base Exception"""
-    def __init__(self, message, status=None, payload=None):
+    def __init__(self, message, status, payload):
         self.message = message
         self.status = status
         self.payload = payload
 
     def __str__(self):
-        return repr({
+        error = {
                 "status": self.status,
                 "message": self.message,
                 "payload": self.payload
-                })
+            }
+        return repr({k: v for (k, v) in error.items() if v is not None})
 
 
 class NotFoundError(MapRouletteBaseException):
     """Resource cannot be found"""
+    def __init__(self, message=None, status=None, payload=None):
+        if message:
+            self.message = message
+        else:
+            self.message = "Resource cannot be found."
+        super().__init__(message=self.message, status=status, payload=payload)
 
 
 class ConnectionUnavailableError(MapRouletteBaseException):
     """A connection error occurred"""
+    def __init__(self, message=None, status=None, payload=None):
+        if message:
+            self.message = message
+        else:
+            self.message = "A connection error occurred."
+        super().__init__(message=self.message, status=status, payload=payload)
 
 
 class UnauthorizedError(MapRouletteBaseException):
     """The user is not authorized to make this request"""
+    def __init__(self, message=None, status=None, payload=None):
+        if message:
+            self.message = message
+        else:
+            self.message = "The user is not authorized to make this request."
+        super().__init__(message=self.message, status=status, payload=payload)
 
 
 class HttpError(MapRouletteBaseException):
     """An HTTP error occurred"""
+    def __init__(self, message=None, status=None, payload=None):
+        if message:
+            self.message = message
+        else:
+            self.message = "An HTTP error occurred."
+        super().__init__(message=self.message, status=status, payload=payload)
 
 
 class InvalidJsonError(MapRouletteBaseException):
     """Errors produced from an invalid JSON object"""
+    def __init__(self, message=None, status=None, payload=None):
+        if message:
+            self.message = message
+        else:
+            self.message = "Invalid JSON payload."
+        super().__init__(message=self.message, status=status, payload=payload)

maproulette/api/maproulette_server.py

@@ -64,28 +64,25 @@ def get(self, endpoint, params=None):
         except requests.exceptions.HTTPError as e:
             if e.response.status_code == 404:
                 raise NotFoundError(
-                    message='Resource not found',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             else:
                 raise HttpError(
-                    message='An HTTP error occurred',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
         except (requests.ConnectionError, requests.Timeout) as e:
             raise ConnectionUnavailableError(
-                message="Connection Unavailable",
-                status=e.response.status_code,
-                payload=response
+                message=self.parse_response_message(e.response),
+                status=e.response.status_code
             ) from None
         try:
             return {
                 "data": response.json(),
                 "status": response.status_code
             }
-        except json.decoder.JSONDecodeError:
+        except ValueError:
             return {
                 "status": response.status_code
             }
@@ -108,21 +105,18 @@ def post(self, endpoint, body=None, params=None):
         except requests.exceptions.HTTPError as e:
             if e.response.status_code == 400:
                 raise InvalidJsonError(
-                    message='Invalid JSON payload',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             elif e.response.status_code == 401:
                 raise UnauthorizedError(
-                    message='The user is not authorized to make this request',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             else:
                 raise HttpError(
-                    message='An HTTP error occurred',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
         except (requests.ConnectionError, requests.Timeout) as e:
             raise ConnectionUnavailableError(e) from None
@@ -131,7 +125,7 @@ def post(self, endpoint, body=None, params=None):
                 "data": response.json(),
                 "status": response.status_code
             }
-        except json.decoder.JSONDecodeError:
+        except ValueError:
             return {
                 "status": response.status_code
             }
@@ -154,21 +148,18 @@ def put(self, endpoint, body=None, params=None):
         except requests.exceptions.HTTPError as e:
             if e.response.status_code == 400:
                 raise InvalidJsonError(
-                    message='Invalid JSON payload',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             elif e.response.status_code == 401:
                 raise UnauthorizedError(
-                    message='The user is not authorized to make this request',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             else:
                 raise HttpError(
-                    message='An HTTP error occurred',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
         except (requests.ConnectionError, requests.Timeout) as e:
             raise ConnectionUnavailableError(e) from None
@@ -177,7 +168,7 @@ def put(self, endpoint, body=None, params=None):
                 "data": response.json(),
                 "status": response.status_code
             }
-        except json.decoder.JSONDecodeError:
+        except ValueError:
             return {
                 "status": response.status_code
             }
@@ -198,21 +189,18 @@ def delete(self, endpoint, params=None):
         except requests.exceptions.HTTPError as e:
             if e.response.status_code == 401:
                 raise UnauthorizedError(
-                    message='The user is not authorized to make this request',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             elif e.response.status_code == 404:
                 raise NotFoundError(
-                    message='Resource not found',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
             else:
                 raise HttpError(
-                    message='An HTTP error occurred',
-                    status=e.response.status_code,
-                    payload=e.response
+                    message=self.parse_response_message(e.response),
+                    status=e.response.status_code
                 ) from None
         except (requests.ConnectionError, requests.Timeout) as e:
             raise ConnectionUnavailableError(e) from None
@@ -221,7 +209,7 @@ def delete(self, endpoint, params=None):
                 "data": response.json(),
                 "status": response.status_code
             }
-        except json.decoder.JSONDecodeError:
+        except ValueError:
             return {
                 "status": response.status_code
             }
@@ -239,3 +227,15 @@ def is_json(input_object):
             return True
         except ValueError:
             return False
+
+    @staticmethod
+    def parse_response_message(response):
+        """Method to determine the message body from a response object. Will return None if message cannot be parsed.
+
+        :param response: the Requests response object
+        :returns: the response message if parsable, otherwise None
+        """
+        try:
+            return json.loads(response.text)['message']
+        except (ValueError, KeyError):
+            return None

tests/test_server.py

@@ -26,7 +26,6 @@ def test_get_not_found_error(self, mock_get, mock_server_get, server_instance=se
             server_instance.get(endpoint='')
         assert context.exception.message == 'Resource not found'
         assert context.exception.status == 404
-        assert context.exception.payload == 'error payload'
 
     @patch('maproulette.api.maproulette_server.MapRouletteServer.get')
     @patch('requests.Session.get')
@@ -265,3 +264,20 @@ def test_delete_connection_error(self, mock_delete, mock_server_delete, server_i
         assert context.exception.message == 'Connection Unavailable'
         assert context.exception.status == 500
         assert context.exception.payload == 'error payload'
+
+    @patch('json.loads')
+    def test_parse_response_message(self, mock_loads, server_instance=server):
+        mock_loads.return_value = {
+            'message': 'some message'
+        }
+        self.assertTrue(server_instance.parse_response_message(mock_loads))
+
+    @patch('json.loads')
+    def test_parse_response_message_value_error(self, mock_loads, server_instance=server):
+        mock_loads.side_effect = ValueError()
+        self.assertIsNone(server_instance.parse_response_message(mock_loads))
+
+    @patch('json.loads')
+    def test_parse_response_message_key_error(self, mock_loads, server_instance=server):
+        mock_loads.side_effect = KeyError()
+        self.assertIsNone(server_instance.parse_response_message(mock_loads))