Converting response to raise a ProblemException (#955)

* Converting response to raise a ProblemException

* Centralizing around ProblemException for errors in the app.

* Adding the ability to skip error handlers, allow for defining exception payload.

* Fixing flake8

* Fixed some bugs found through unit testing.

* Unit tests are now passing.

* Added problem back to __init__

* Updating based on the feedback from the PR.

connexion/apis/aiohttp_api.py

@@ -55,7 +55,8 @@ def problems_middleware(request, handler):
     try:
         response = yield from handler(request)
     except ProblemException as exc:
-        response = exc.to_problem()
+        response = problem(status=exc.status, detail=exc.detail, title=exc.title,
+                           type=exc.type, instance=exc.instance, headers=exc.headers, ext=exc.ext)
     except (werkzeug_HTTPException, _HttpNotFoundError) as exc:
         response = problem(status=exc.code, title=exc.name, detail=exc.description)
     except web.HTTPError as exc:

connexion/apps/abstract.py

@@ -14,7 +14,7 @@ logger = logging.getLogger('connexion.app')
 class AbstractApp(object):
     def __init__(self, import_name, api_cls, port=None, specification_dir='',
                  host=None, server=None, arguments=None, auth_all_paths=False, debug=False,
-                 resolver=None, options=None):
+                 resolver=None, options=None, skip_error_handlers=False):
         """
         :param import_name: the name of the application package
         :type import_name: str
@@ -63,6 +63,7 @@ class AbstractApp(object):
 
         logger.debug('Specification directory: %s', self.specification_dir)
 
+        if not skip_error_handlers:
             logger.debug('Setting error handlers')
             self.set_errors_handlers()
 

connexion/apps/flask_app.py

@@ -40,7 +40,10 @@ class FlaskApp(AbstractApp):
         :type exception: Exception
         """
         if isinstance(exception, ProblemException):
-            response = exception.to_problem()
+            response = problem(
+                status=exception.status, title=exception.title, detail=exception.detail,
+                type=exception.type, instance=exception.instance, headers=exception.headers,
+                ext=exception.ext)
         else:
             if not isinstance(exception, werkzeug.exceptions.HTTPException):
                 exception = werkzeug.exceptions.InternalServerError()

connexion/decorators/metrics.py

@@ -3,7 +3,7 @@ import os
 import time
 
 from werkzeug.exceptions import HTTPException
-
+from connexion.exceptions import ProblemException
 try:
     import uwsgi_metrics
     HAS_UWSGI_METRICS = True  # pragma: no cover
@@ -40,6 +40,9 @@ class UWSGIMetricsCollector(object):
             except HTTPException as http_e:
                 status = http_e.code
                 raise http_e
+            except ProblemException as prob_e:
+                status = prob_e.status
+                raise prob_e
             finally:
                 end_time_s = time.time()
                 delta_s = end_time_s - start_time_s

connexion/decorators/response.py

@@ -6,7 +6,6 @@ from jsonschema import ValidationError
 
 from ..exceptions import (NonConformingResponseBody,
                           NonConformingResponseHeaders)
-from ..problem import problem
 from ..utils import all_json, has_coroutine
 from .decorator import BaseDecorator
 from .validation import ResponseBodyValidator
@@ -86,17 +85,12 @@ class ResponseValidator(BaseDecorator):
         """
 
         def _wrapper(request, response):
-            try:
             connexion_response = \
                 self.operation.api.get_connexion_response(response, self.mimetype)
             self.validate_response(
                 connexion_response.body, connexion_response.status_code,
                 connexion_response.headers, request.url)
 
-            except (NonConformingResponseBody, NonConformingResponseHeaders) as e:
-                response = problem(500, e.reason, e.message)
-                return self.operation.api.get_response(response)
-
             return response
 
         if has_coroutine(function):  # pragma: 2.7 no cover

connexion/decorators/validation.py

@@ -10,10 +10,9 @@ from jsonschema import Draft4Validator, ValidationError, draft4_format_checker
 from jsonschema.validators import extend
 from werkzeug.datastructures import FileStorage
 
-from ..exceptions import ExtraParameterProblem
+from ..exceptions import ExtraParameterProblem, BadRequestProblem, UnsupportedMediaTypeProblem
 from ..http_facts import FORM_CONTENT_TYPES
 from ..json_schema import Draft4RequestValidator, Draft4ResponseValidator
-from ..problem import problem
 from ..utils import all_json, boolean, is_json_mimetype, is_null, is_nullable
 
 _jsonschema_3_or_newer = pkg_resources.parse_version(
@@ -148,22 +147,17 @@ class RequestBodyValidator(object):
 
                     if ctype_is_json:
                         # Content-Type is json but actual body was not parsed
-                        return problem(400,
-                                       "Bad Request",
-                                       "Request body is not valid JSON"
-                                       )
+                        raise BadRequestProblem(detail="Request body is not valid JSON")
                     else:
                         # the body has contents that were not parsed as JSON
-                        return problem(415,
-                                       "Unsupported Media Type",
+                        raise UnsupportedMediaTypeProblem(
                                        "Invalid Content-type ({content_type}), expected JSON data".format(
                                            content_type=request.headers.get("Content-Type", "")
                                        ))
 
                 logger.debug("%s validating schema...", request.url)
-                error = self.validate_schema(data, request.url)
-                if error and not self.has_default:
-                    return error
+                if data is not None or not self.has_default:
+                    self.validate_schema(data, request.url)
             elif self.consumes[0] in FORM_CONTENT_TYPES:
                 data = dict(request.form.items()) or (request.body if len(request.body) > 0 else {})
                 data.update(dict.fromkeys(request.files, ''))  # validator expects string..
@@ -185,11 +179,9 @@ class RequestBodyValidator(object):
                                 errs += [str(e)]
                                 print(errs)
                     if errs:
-                        return problem(400, 'Bad Request', errs)
+                        raise BadRequestProblem(detail=errs)
 
-                error = self.validate_schema(data, request.url)
-                if error:
-                    return error
+                self.validate_schema(data, request.url)
 
             response = function(request)
             return response
@@ -207,7 +199,7 @@ class RequestBodyValidator(object):
             logger.error("{url} validation error: {error}".format(url=url,
                                                                   error=exception.message),
                          extra={'validator': 'body'})
-            return problem(400, 'Bad Request', str(exception.message))
+            raise BadRequestProblem(detail=str(exception.message))
 
         return None
 
@@ -358,32 +350,27 @@ class ParameterValidator(object):
             for param in self.parameters.get('query', []):
                 error = self.validate_query_parameter(param, request)
                 if error:
-                    response = problem(400, 'Bad Request', error)
-                    return self.api.get_response(response)
+                    raise BadRequestProblem(detail=error)
 
             for param in self.parameters.get('path', []):
                 error = self.validate_path_parameter(param, request)
                 if error:
-                    response = problem(400, 'Bad Request', error)
-                    return self.api.get_response(response)
+                    raise BadRequestProblem(detail=error)
 
             for param in self.parameters.get('header', []):
                 error = self.validate_header_parameter(param, request)
                 if error:
-                    response = problem(400, 'Bad Request', error)
-                    return self.api.get_response(response)
+                    raise BadRequestProblem(detail=error)
 
             for param in self.parameters.get('cookie', []):
                 error = self.validate_cookie_parameter(param, request)
                 if error:
-                    response = problem(400, 'Bad Request', error)
-                    return self.api.get_response(response)
+                    raise BadRequestProblem(detail=error)
 
             for param in self.parameters.get('formData', []):
                 error = self.validate_formdata_parameter(param["name"], param, request)
                 if error:
-                    response = problem(400, 'Bad Request', error)
-                    return self.api.get_response(response)
+                    raise BadRequestProblem(detail=error)
 
             return function(request)
 

connexion/exceptions.py

@@ -1,3 +1,4 @@
+import warnings
 from jsonschema.exceptions import ValidationError
 from werkzeug.exceptions import Forbidden, Unauthorized
 
@@ -24,6 +25,9 @@ class ProblemException(ConnexionException):
         self.ext = ext
 
     def to_problem(self):
+        warnings.warn(
+            "'to_problem' is planned to be removed in a future release. "
+            "Call connexion.problem.problem(..) instead to maintain the existing error response.", DeprecationWarning)
         return problem(status=self.status, title=self.title, detail=self.detail,
                        type=self.type, instance=self.instance, headers=self.headers,
                        ext=self.ext)
@@ -52,12 +56,13 @@ class InvalidSpecification(ConnexionException, ValidationError):
     pass
 
 
-class NonConformingResponse(ConnexionException):
+class NonConformingResponse(ProblemException):
     def __init__(self, reason='Unknown Reason', message=None):
         """
         :param reason: Reason why the response did not conform to the specification
         :type reason: str
         """
+        super(NonConformingResponse, self).__init__(status=500, title=reason, detail=message)
         self.reason = reason
         self.message = message
 
@@ -68,6 +73,30 @@ class NonConformingResponse(ConnexionException):
         return '<NonConformingResponse: {}>'.format(self.reason)
 
 
+class AuthenticationProblem(ProblemException):
+
+    def __init__(self, status, title, detail):
+        super(AuthenticationProblem, self).__init__(status=status, title=title, detail=detail)
+
+
+class ResolverProblem(ProblemException):
+
+    def __init__(self, status, title, detail):
+        super(ResolverProblem, self).__init__(status=status, title=title, detail=detail)
+
+
+class BadRequestProblem(ProblemException):
+
+    def __init__(self, title='Bad Request', detail=None):
+        super(BadRequestProblem, self).__init__(status=400, title=title, detail=detail)
+
+
+class UnsupportedMediaTypeProblem(ProblemException):
+
+    def __init__(self, title="Unsupported Media Type", detail=None):
+        super(UnsupportedMediaTypeProblem, self).__init__(status=415, title=title, detail=detail)
+
+
 class NonConformingResponseBody(NonConformingResponse):
     def __init__(self, message, reason="Response body does not conform to specification"):
         super(NonConformingResponseBody, self).__init__(reason=reason, message=message)

connexion/handlers.py

@@ -1,7 +1,7 @@
 import logging
 
 from .operations.secure import SecureOperation
-from .problem import problem
+from .exceptions import AuthenticationProblem, ResolverProblem
 
 logger = logging.getLogger('connexion.handlers')
 
@@ -45,12 +45,11 @@ class AuthErrorHandler(SecureOperation):
         """
         Actual handler for the execution after authentication.
         """
-        response = problem(
+        raise AuthenticationProblem(
             title=self.exception.name,
             detail=self.exception.description,
             status=self.exception.code
         )
-        return self.api.get_response(response)
 
 
 class ResolverErrorHandler(SecureOperation):
@@ -68,12 +67,11 @@ class ResolverErrorHandler(SecureOperation):
         return self.handle
 
     def handle(self, *args, **kwargs):
-        response = problem(
+        raise ResolverProblem(
             title='Not Implemented',
             detail=self.exception.reason,
             status=self.status_code
         )
-        return self.api.get_response(response)
 
     @property
     def operation_id(self):

connexion/utils.py

@@ -1,6 +1,5 @@
 import functools
 import importlib
-import re
 
 import six
 import yaml

requirements-all.txt

@@ -0,0 +1,4 @@
+-r requirements-aiohttp.txt
+-r requirements-devel.txt
+openapi_spec_validator
+pytest-aiohttp

tests/aiohttp/test_aiohttp_errors.py

@@ -22,7 +22,7 @@ def aiohttp_app(problem_api_spec_dir):
 
 
 @asyncio.coroutine
-def test_aiohttp_problems(aiohttp_app, aiohttp_client):
+def test_aiohttp_problems_404(aiohttp_app, aiohttp_client):
     # TODO: This is a based on test_errors.test_errors(). That should be refactored
     #       so that it is parameterized for all web frameworks.
     app_client = yield from aiohttp_client(aiohttp_app.app)  # type: aiohttp.test_utils.TestClient
@@ -38,6 +38,12 @@ def test_aiohttp_problems(aiohttp_app, aiohttp_client):
     assert error404['status'] == 404
     assert 'instance' not in error404
 
+@asyncio.coroutine
+def test_aiohttp_problems_405(aiohttp_app, aiohttp_client):
+    # TODO: This is a based on test_errors.test_errors(). That should be refactored
+    #       so that it is parameterized for all web frameworks.
+    app_client = yield from aiohttp_client(aiohttp_app.app)  # type: aiohttp.test_utils.TestClient
+
     get_greeting = yield from app_client.get('/v1.0/greeting/jsantos')  # type: aiohttp.ClientResponse
     assert get_greeting.content_type == 'application/problem+json'
     assert get_greeting.status == 405
@@ -49,6 +55,12 @@ def test_aiohttp_problems(aiohttp_app, aiohttp_client):
     assert error405['status'] == 405
     assert 'instance' not in error405
 
+@asyncio.coroutine
+def test_aiohttp_problems_500(aiohttp_app, aiohttp_client):
+    # TODO: This is a based on test_errors.test_errors(). That should be refactored
+    #       so that it is parameterized for all web frameworks.
+    app_client = yield from aiohttp_client(aiohttp_app.app)  # type: aiohttp.test_utils.TestClient
+
     get500 = yield from app_client.get('/v1.0/except')  # type: aiohttp.ClientResponse
     assert get500.content_type == 'application/problem+json'
     assert get500.status == 500
@@ -60,6 +72,12 @@ def test_aiohttp_problems(aiohttp_app, aiohttp_client):
     assert error500['status'] == 500
     assert 'instance' not in error500
 
+@asyncio.coroutine
+def test_aiohttp_problems_418(aiohttp_app, aiohttp_client):
+    # TODO: This is a based on test_errors.test_errors(). That should be refactored
+    #       so that it is parameterized for all web frameworks.
+    app_client = yield from aiohttp_client(aiohttp_app.app)  # type: aiohttp.test_utils.TestClient
+
     get_problem = yield from app_client.get('/v1.0/problem')  # type: aiohttp.ClientResponse
     assert get_problem.content_type == 'application/problem+json'
     assert get_problem.status == 418
@@ -72,6 +90,12 @@ def test_aiohttp_problems(aiohttp_app, aiohttp_client):
     assert error_problem['status'] == 418
     assert error_problem['instance'] == 'instance1'
 
+@asyncio.coroutine
+def test_aiohttp_problems_misc(aiohttp_app, aiohttp_client):
+    # TODO: This is a based on test_errors.test_errors(). That should be refactored
+    #       so that it is parameterized for all web frameworks.
+    app_client = yield from aiohttp_client(aiohttp_app.app)  # type: aiohttp.test_utils.TestClient
+
     problematic_json = yield from app_client.get(
         '/v1.0/json_response_with_undefined_value_to_serialize')  # type: aiohttp.ClientResponse
     assert problematic_json.content_type == 'application/problem+json'

tests/fakeapi/hello.py

@@ -2,7 +2,7 @@
 import flask
 from flask import jsonify, redirect
 
-from connexion import NoContent, ProblemException, context, problem
+from connexion import NoContent, ProblemException, context
 
 
 class DummyClass(object):
@@ -92,7 +92,7 @@ def get_bye_secure_jwt(name, user, token_info):
     return 'Goodbye {name} (Secure: {user})'.format(name=name, user=user)
 
 def with_problem():
-    return problem(type='http://www.example.com/error',
+    raise ProblemException(type='http://www.example.com/error',
                            title='Some Error',
                            detail='Something went wrong somewhere',
                            status=418,
@@ -101,7 +101,7 @@ def with_problem():
 
 
 def with_problem_txt():
-    return problem(title='Some Error',
+    raise ProblemException(title='Some Error',
                            detail='Something went wrong somewhere',
                            status=418,
                            instance='instance1')
@@ -416,7 +416,7 @@ def get_empty_dict():
 
 
 def get_custom_problem_response():
-    return problem(403, "You need to pay", "Missing amount",
+    raise ProblemException(403, "You need to pay", "Missing amount",
                            ext={'amount': 23.0})
 
 

tests/test_metrics.py

@@ -1,9 +1,11 @@
 import json
 
 import flask
+import pytest
 from mock import MagicMock
 
 import connexion
+from connexion.exceptions import ProblemException
 from connexion.decorators.metrics import UWSGIMetricsCollector
 
 
@@ -11,13 +13,14 @@ def test_timer(monkeypatch):
     wrapper = UWSGIMetricsCollector('/foo/bar/<param>', 'get')
 
     def operation(req):
-        return connexion.problem(418, '', '')
+        raise ProblemException(418, '', '')
 
     op = wrapper(operation)
     metrics = MagicMock()
     monkeypatch.setattr('flask.request', MagicMock())
     monkeypatch.setattr('flask.current_app', MagicMock(response_class=flask.Response))
     monkeypatch.setattr('connexion.decorators.metrics.uwsgi_metrics', metrics)
+    with pytest.raises(ProblemException) as exc:
         op(MagicMock())
     assert metrics.timer.call_args[0][:2] == ('connexion.response',
                                               '418.GET.foo.bar.{param}')

tests/test_validation.py

@@ -1,10 +1,12 @@
 import json
 
 import flask
+import pytest
 # we are using "mock" module here for Py 2.7 support
 from mock import MagicMock
 
 from connexion.apis.flask_api import FlaskApi
+from connexion.exceptions import BadRequestProblem
 from connexion.decorators.validation import ParameterValidator
 
 
@@ -34,40 +36,61 @@ def test_parameter_validator(monkeypatch):
 
     kwargs = {'query': {}, 'headers': {}, 'cookies': {}}
     request = MagicMock(path_params={}, **kwargs)
-    assert json.loads(handler(request).data.decode())['detail'] == "Missing path parameter 'p1'"
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail == "Missing path parameter 'p1'"
     request = MagicMock(path_params={'p1': '123'}, **kwargs)
     assert handler(request) == 'OK'
     request = MagicMock(path_params={'p1': ''}, **kwargs)
-    assert json.loads(handler(request).data.decode())['detail'] == "Wrong type, expected 'integer' for path parameter 'p1'"
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail == "Wrong type, expected 'integer' for path parameter 'p1'"
     request = MagicMock(path_params={'p1': 'foo'}, **kwargs)
-    assert json.loads(handler(request).data.decode())['detail'] == "Wrong type, expected 'integer' for path parameter 'p1'"
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail == "Wrong type, expected 'integer' for path parameter 'p1'"
     request = MagicMock(path_params={'p1': '1.2'}, **kwargs)
-    assert json.loads(handler(request).data.decode())['detail'] == "Wrong type, expected 'integer' for path parameter 'p1'"
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail == "Wrong type, expected 'integer' for path parameter 'p1'"
 
-    request = MagicMock(path_params={'p1': 1}, query={'q1': '4'}, headers={}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith('4 is greater than the maximum of 3')
+    request = MagicMock(path_params={'p1': 1}, query={'q1': '4'}, headers={})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith('4 is greater than the maximum of 3')
     request = MagicMock(path_params={'p1': 1}, query={'q1': '3'}, headers={}, cookies={})
     assert handler(request) == 'OK'
 
     request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', '2']}, headers={}, cookies={})
     assert handler(request) == "OK"
-    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', 'a']}, headers={}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("'a' is not of type 'integer'")
-    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', '-1']}, headers={}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("-1 is less than the minimum of 0")
-    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1']}, headers={}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("[1] is too short")
-    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', '2', '3', '4']}, headers={}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("[1, 2, 3, 4] is too long")
-
-    request = MagicMock(path_params={'p1': '123'}, query={}, headers={'h1': 'a'}, cookies={})
-    assert handler(request) == 'OK'
-
-    request = MagicMock(path_params={'p1': '123'}, query={}, headers={'h1': 'x'}, cookies={})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("'x' is not one of ['a', 'b']")
-
+    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', 'a']}, headers={})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith("'a' is not of type 'integer'")
     request = MagicMock(path_params={'p1': '123'}, query={}, headers={}, cookies={'c1': 'b'})
     assert handler(request) == 'OK'
 
     request = MagicMock(path_params={'p1': '123'}, query={}, headers={}, cookies={'c1': 'x'})
-    assert json.loads(handler(request).data.decode())['detail'].startswith("'x' is not one of ['a', 'b']")
+    with pytest.raises(BadRequestProblem) as exc:
+        assert handler(request)
+    assert exc.value.detail.startswith("'x' is not one of ['a', 'b']")
+    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', '-1']}, headers={})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith("-1 is less than the minimum of 0")
+    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1']}, headers={})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith("[1] is too short")
+    request = MagicMock(path_params={'p1': 1}, query={'a1': ['1', '2', '3', '4']}, headers={})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith("[1, 2, 3, 4] is too long")
+
+    request = MagicMock(path_params={'p1': '123'}, query={}, headers={'h1': 'a'}, cookies={})
+    assert handler(request) == 'OK'
+
+    request = MagicMock(path_params={'p1': '123'}, query={}, headers={'h1': 'x'})
+    with pytest.raises(BadRequestProblem) as exc:
+        handler(request)
+    assert exc.value.detail.startswith("'x' is not one of ['a', 'b']")