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 @@
 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,8 +63,9 @@ def __init__(self, import_name, api_cls, port=None, specification_dir='',
 
         logger.debug('Specification directory: %s', self.specification_dir)
 
-        logger.debug('Setting error handlers')
-        self.set_errors_handlers()
+        if not skip_error_handlers:
+            logger.debug('Setting error handlers')
+            self.set_errors_handlers()
 
     @abc.abstractmethod
     def create_app(self):

connexion/apps/flask_app.py

@@ -40,7 +40,10 @@ def common_error_handler(exception):
         :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 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 @@ def wrapper(*args, **kwargs):
             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 ..exceptions import (NonConformingResponseBody,
                           NonConformingResponseHeaders)
-from ..problem import problem
 from ..utils import all_json, has_coroutine
 from .decorator import BaseDecorator
 from .validation import ResponseBodyValidator
@@ -86,16 +85,11 @@ def __call__(self, function):
         """
 
         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)
+            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)
 
             return response
 

connexion/decorators/validation.py

@@ -10,10 +10,9 @@
 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 @@ def wrapper(request):
 
                     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 @@ def wrapper(request):
                                 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 @@ def validate_schema(self, data, url):
             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 @@ def wrapper(request):
             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 @@ def __init__(self, status=400, title=None, detail=None, type=None,
         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 @@ def __repr__(self):  # pragma: no cover
         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 @@ def handle(self, *args, **kwargs):
         """
         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 @@ def function(self):
         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'