diff --git a/README.md b/README.md index 9ca4aff..4239914 100644 --- a/README.md +++ b/README.md @@ -223,7 +223,7 @@ To store data, send a POST request to `/measure` with the values in one of the f curl -X POST http://localhost:8080/measure -H "Content-Type: application/xml" -d '21.540' ``` -A malformed XML body or a JSON body that is not an object is rejected with a `400` response. With `format='xml'`, column names must be valid XML element names: letters, digits, `_`, `-` or `.`, starting with a letter or `_`, without spaces, and not starting with `xml`. +A malformed XML body or a JSON body that is not an object (or not valid JSON) is rejected with a `400` response. All errors are returned as JSON with a `message` field, e.g. `{"message": "Invalid JSON: the request body could not be parsed."}`. With `format='xml'`, column names must be valid XML element names: letters, digits, `_`, `-` or `.`, starting with a letter or `_`, without spaces, and not starting with `xml`. To restrict access to the collected data, define the following setting in the `configuration.yml` file in the root directory: ```yml diff --git a/docs/getting_started.rst b/docs/getting_started.rst index 7e63a9a..a2e23f5 100644 --- a/docs/getting_started.rst +++ b/docs/getting_started.rst @@ -96,7 +96,7 @@ To store data, send a POST request to ``/measure`` with the values in one of the curl -X POST http://localhost:8080/measure -H "Content-Type: application/xml" -d '21.540' -A malformed XML body or a JSON body that is not an object is rejected with a ``400`` response. With ``format='xml'``, column names must be valid XML element names: letters, digits, ``_``, ``-`` or ``.``, starting with a letter or ``_``, without spaces, and not starting with ``xml``. +A malformed XML body or a JSON body that is not an object (or not valid JSON) is rejected with a ``400`` response. All errors are returned as JSON with a ``message`` field, e.g. ``{"message": "Invalid JSON: the request body could not be parsed."}``. With ``format='xml'``, column names must be valid XML element names: letters, digits, ``_``, ``-`` or ``.``, starting with a letter or ``_``, without spaces, and not starting with ``xml``. To restrict access to the collected data, define the following setting in the ``configuration.yml`` file in the root directory: diff --git a/succulent/api.py b/succulent/api.py index f84c622..9c20631 100644 --- a/succulent/api.py +++ b/succulent/api.py @@ -1,10 +1,27 @@ +import json import numpy as np import pandas as pd -from flask import Flask, jsonify, request, url_for, make_response +from flask import Flask, Request, jsonify, request, url_for, make_response +from werkzeug.exceptions import BadRequest, HTTPException from succulent.configuration import Configuration from succulent.processing import Processing +class SucculentRequest(Request): + """Request class with a clear error message for invalid JSON bodies.""" + + def on_json_loading_failed(self, e): + """Raise a 400 error that says the JSON body could not be parsed. + + Args: + e (ValueError or None): The error raised while parsing. + + Raises: + BadRequest: Always. + """ + raise BadRequest('Invalid JSON: the request body could not be parsed.') + + class SucculentAPI: """Succulent API server. @@ -48,6 +65,7 @@ def __init__(self, config, host='0.0.0.0', port=8080, format='csv'): # Initialise Flask self.app = Flask(__name__, static_url_path='/succulent/static') + self.app.request_class = SucculentRequest # Limit the maximum accepted size of incoming request bodies to # protect the server against oversized or flooding POST requests. @@ -66,6 +84,11 @@ def __init__(self, config, host='0.0.0.0', port=8080, format='csv'): self.app.register_error_handler( 413, self.payload_too_large) + # Return every other error as JSON ({"message": ...}) instead of + # Flask's default HTML error page, like the API's own errors + self.app.register_error_handler(HTTPException, self.http_error) + self.app.register_error_handler(Exception, self.internal_error) + def index(self): """Generate index HTML page with information about the API. @@ -168,6 +191,44 @@ def payload_too_large(self, error): """ return jsonify({'message': 'Payload too large.'}), 413 + def http_error(self, error): + """Return an HTTP error (e.g. 400, 404, 405) as JSON. + + Headers of the error are kept, e.g. ``Allow`` for 405 responses. + + Args: + error (HTTPException): The raised HTTP error. + + Returns: + Response: JSON response with an error message. + """ + # Short message (e.g. "Not Found.") instead of Werkzeug's generic + # description; a specific description (e.g. invalid JSON) is kept + if error.description and error.description != type(error).description: + message = error.description + else: + message = f'{error.name}.' + + response = error.get_response() + response.data = json.dumps({'message': message}) + response.content_type = 'application/json' + return response + + def internal_error(self, error): + """Return an unexpected error as a JSON 500 response. + + The error is logged with its traceback. Its details are not sent to + the client. + + Args: + error (Exception): The unexpected error. + + Returns: + Response: JSON response with an error message. + """ + self.app.logger.exception('Unexpected error on %s %s', request.method, request.path) + return jsonify({'message': 'Internal server error.'}), 500 + def start(self): """Start the Flask application on the specified host and port. diff --git a/tests/test_succulent.py b/tests/test_succulent.py index 47f8dc7..dc37a15 100644 --- a/tests/test_succulent.py +++ b/tests/test_succulent.py @@ -671,6 +671,63 @@ def test_invalid_allowed_extensions(self): format='image', unittest=True) +class TestJsonErrors(unittest.TestCase): + """ + Test that errors are returned as JSON instead of HTML pages. + """ + + def setUp(self): + config_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'configuration.yml') + self.api = SucculentAPI(host='0.0.0.0', port=8080, config=config_path, format='csv') + self.client = self.api.app.test_client() + + def tearDown(self): + shutil.rmtree(self.api.processing.directory, ignore_errors=True) + + def assertJsonError(self, response, status, message): + self.assertEqual(response.status_code, status) + self.assertEqual(response.content_type, 'application/json') + self.assertEqual(response.get_json(), {'message': message}) + + def test_invalid_json_body(self): + for body in ['{bad', '']: + with self.subTest(body=body): + response = self.client.post('/measure', data=body, + content_type='application/json') + self.assertJsonError(response, 400, + 'Invalid JSON: the request body could not be parsed.') + + def test_unknown_path(self): + self.assertJsonError(self.client.get('/nope'), 404, 'Not Found.') + + def test_method_not_allowed_keeps_allow_header(self): + response = self.client.delete('/measure') + self.assertJsonError(response, 405, 'Method Not Allowed.') + self.assertIn('POST', response.headers['Allow']) + + def test_unexpected_error_is_logged_and_hidden(self): + def crash(req): + raise RuntimeError('secret internal detail') + self.api.processing.process = crash + + with self.assertLogs(self.api.app.logger, level='ERROR') as logs: + response = self.client.post('/measure', json={'temperature': 20}) + + self.assertJsonError(response, 500, 'Internal server error.') + self.assertNotIn('secret', response.get_data(as_text=True)) + self.assertIn('secret internal detail', '\n'.join(logs.output)) + + def test_existing_json_errors_unchanged(self): + self.api.app.config['MAX_CONTENT_LENGTH'] = 10 + response = self.client.post('/measure', data='x' * 100, content_type='application/json') + self.assertJsonError(response, 413, 'Payload too large.') + + def test_index_is_still_html(self): + response = self.client.get('/') + self.assertEqual(response.status_code, 200) + self.assertTrue(response.content_type.startswith('text/html')) + + class TestStaleData(unittest.TestCase): """ Test that /data and /export do not return stale in-memory data.