Fix FlaskApp exception handlers (#1788)

Fixes #1787 In the `FlaskApp`, the error handlers were still registered on the underlying Flask app instead of on the `ExceptionMiddleware`, which led to them not following the documented behavior. The documentation was also incorrect about ignoring error handlers registered on the flask application. We are only ignoring the default error handlers registered by Flask itself. This is a breaking change, however, since this functionality was not following the documented behavior, and 3.0.0 was only recently released, I propose to release this as a patch version.
spec-first · Nov 6, 2023 · 70084bc · 70084bc
1 parent 55bdfba
commit 70084bc
Show file tree

Hide file tree

Showing 7 changed files with 147 additions and 64 deletions.
diff --git a/connexion/apps/flask.py b/connexion/apps/flask.py
@@ -6,6 +6,8 @@
 import typing as t
 
 import flask
+import starlette.exceptions
+import werkzeug.exceptions
 from a2wsgi import WSGIMiddleware
 from flask import Response as FlaskResponse
 from starlette.types import Receive, Scope, Send
@@ -213,7 +215,7 @@ def __init__(
             :obj:`security.SECURITY_HANDLERS`
         """
         self._middleware_app = FlaskASGIApp(import_name, server_args or {})
-        self.app = self._middleware_app.app
+
         super().__init__(
             import_name,
             lifespan=lifespan,
@@ -233,6 +235,15 @@ def __init__(
             security_map=security_map,
         )
 
+        self.app = self._middleware_app.app
+        self.app.register_error_handler(
+            werkzeug.exceptions.HTTPException, self._http_exception
+        )
+
+    def _http_exception(self, exc: werkzeug.exceptions.HTTPException):
+        """Reraise werkzeug HTTPExceptions as starlette HTTPExceptions"""
+        raise starlette.exceptions.HTTPException(exc.code, detail=exc.description)
+
     def add_url_rule(
         self, rule, endpoint: str = None, view_func: t.Callable = None, **options
     ):
@@ -247,4 +258,4 @@ def add_error_handler(
             [ConnexionRequest, Exception], MaybeAwaitable[ConnexionResponse]
         ],
     ) -> None:
-        self.app.register_error_handler(code_or_exception, function)
+        self.middleware.add_error_handler(code_or_exception, function)
diff --git a/connexion/http_facts.py b/connexion/http_facts.py
@@ -5,3 +5,70 @@
 FORM_CONTENT_TYPES = ["application/x-www-form-urlencoded", "multipart/form-data"]
 
 METHODS = {"get", "put", "post", "delete", "options", "head", "patch", "trace"}
+
+HTTP_STATUS_CODES = {
+    100: "Continue",
+    101: "Switching Protocols",
+    102: "Processing",
+    103: "Early Hints",  # see RFC 8297
+    200: "OK",
+    201: "Created",
+    202: "Accepted",
+    203: "Non Authoritative Information",
+    204: "No Content",
+    205: "Reset Content",
+    206: "Partial Content",
+    207: "Multi Status",
+    208: "Already Reported",  # see RFC 5842
+    226: "IM Used",  # see RFC 3229
+    300: "Multiple Choices",
+    301: "Moved Permanently",
+    302: "Found",
+    303: "See Other",
+    304: "Not Modified",
+    305: "Use Proxy",
+    306: "Switch Proxy",  # unused
+    307: "Temporary Redirect",
+    308: "Permanent Redirect",
+    400: "Bad Request",
+    401: "Unauthorized",
+    402: "Payment Required",  # unused
+    403: "Forbidden",
+    404: "Not Found",
+    405: "Method Not Allowed",
+    406: "Not Acceptable",
+    407: "Proxy Authentication Required",
+    408: "Request Timeout",
+    409: "Conflict",
+    410: "Gone",
+    411: "Length Required",
+    412: "Precondition Failed",
+    413: "Request Entity Too Large",
+    414: "Request URI Too Long",
+    415: "Unsupported Media Type",
+    416: "Requested Range Not Satisfiable",
+    417: "Expectation Failed",
+    418: "I'm a teapot",  # see RFC 2324
+    421: "Misdirected Request",  # see RFC 7540
+    422: "Unprocessable Entity",
+    423: "Locked",
+    424: "Failed Dependency",
+    425: "Too Early",  # see RFC 8470
+    426: "Upgrade Required",
+    428: "Precondition Required",  # see RFC 6585
+    429: "Too Many Requests",
+    431: "Request Header Fields Too Large",
+    449: "Retry With",  # proprietary MS extension
+    451: "Unavailable For Legal Reasons",
+    500: "Internal Server Error",
+    501: "Not Implemented",
+    502: "Bad Gateway",
+    503: "Service Unavailable",
+    504: "Gateway Timeout",
+    505: "HTTP Version Not Supported",
+    506: "Variant Also Negotiates",  # see RFC 2295
+    507: "Insufficient Storage",
+    508: "Loop Detected",  # see RFC 5842
+    510: "Not Extended",
+    511: "Network Authentication Failed",
+}
diff --git a/connexion/lifecycle.py b/connexion/lifecycle.py
@@ -260,5 +260,6 @@ def __init__(
         self.content_type = content_type
         self.body = body
         self.headers = headers or {}
-        self.headers.update({"Content-Type": content_type})
+        if content_type:
+            self.headers.update({"Content-Type": content_type})
         self.is_streamed = is_streamed
diff --git a/connexion/middleware/exceptions.py b/connexion/middleware/exceptions.py
@@ -1,8 +1,8 @@
 import asyncio
+import functools
 import logging
 import typing as t
 
-import werkzeug.exceptions
 from starlette.concurrency import run_in_threadpool
 from starlette.exceptions import HTTPException
 from starlette.middleware.exceptions import (
@@ -12,6 +12,7 @@
 from starlette.responses import Response as StarletteResponse
 from starlette.types import ASGIApp, Receive, Scope, Send
 
+from connexion import http_facts
 from connexion.exceptions import InternalServerError, ProblemException, problem
 from connexion.lifecycle import ConnexionRequest, ConnexionResponse
 from connexion.types import MaybeAwaitable
@@ -28,6 +29,7 @@ def connexion_wrapper(
     them to the error handler, and translates the returned Connexion responses to
     Starlette responses."""
 
+    @functools.wraps(handler)
     async def wrapper(request: StarletteRequest, exc: Exception) -> StarletteResponse:
         request = ConnexionRequest.from_starlette_request(request)
 
@@ -36,6 +38,9 @@ async def wrapper(request: StarletteRequest, exc: Exception) -> StarletteRespons
         else:
             response = await run_in_threadpool(handler, request, exc)
 
+        while asyncio.iscoroutine(response):
+            response = await response
+
         return StarletteResponse(
             content=response.body,
             status_code=response.status_code,
@@ -53,9 +58,6 @@ class ExceptionMiddleware(StarletteExceptionMiddleware):
     def __init__(self, next_app: ASGIApp):
         super().__init__(next_app)
         self.add_exception_handler(ProblemException, self.problem_handler)  # type: ignore
-        self.add_exception_handler(
-            werkzeug.exceptions.HTTPException, self.flask_error_handler
-        )
         self.add_exception_handler(Exception, self.common_error_handler)
 
     def add_exception_handler(
@@ -81,7 +83,7 @@ def http_exception(
         """Default handler for Starlette HTTPException"""
         logger.error("%r", exc)
         return problem(
-            title=exc.detail,
+            title=http_facts.HTTP_STATUS_CODES.get(exc.status_code),
             detail=exc.detail,
             status=exc.status_code,
             headers=exc.headers,
@@ -95,21 +97,5 @@ def common_error_handler(
         logger.error("%r", exc, exc_info=exc)
         return InternalServerError().to_problem()
 
-    def flask_error_handler(
-        self, request: StarletteRequest, exc: werkzeug.exceptions.HTTPException
-    ) -> ConnexionResponse:
-        """Default handler for Flask / werkzeug HTTPException"""
-        # If a handler is registered for the received status_code, call it instead.
-        # This is only done automatically for Starlette HTTPExceptions
-        if handler := self._status_handlers.get(exc.code):
-            starlette_exception = HTTPException(exc.code, detail=exc.description)
-            return handler(request, starlette_exception)
-
-        return problem(
-            title=exc.name,
-            detail=exc.description,
-            status=exc.code,
-        )
-
     async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
         await super().__call__(scope, receive, send)
diff --git a/docs/exceptions.rst b/docs/exceptions.rst
@@ -4,21 +4,21 @@ Exception Handling
 Connexion allows you to register custom error handlers to convert Python ``Exceptions`` into HTTP
 problem responses.
 
+You can register error handlers on:
+
+- The exception class to handle
+  If this exception class is raised somewhere in your application or the middleware stack,
+  it will be passed to your handler.
+- The HTTP status code to handle
+  Connexion will raise ``starlette.HTTPException`` errors when it encounters any issues
+  with a request or response. You can intercept these exceptions with specific status codes
+  if you want to return custom responses.
+
 .. tab-set::
 
     .. tab-item:: AsyncApp
         :sync: AsyncApp
 
-        You can register error handlers on:
-
-        - The exception class to handle
-          If this exception class is raised somewhere in your application or the middleware stack,
-          it will be passed to your handler.
-        - The HTTP status code to handle
-          Connexion will raise ``starlette.HTTPException`` errors when it encounters any issues
-          with a request or response. You can intercept these exceptions with specific status codes
-          if you want to return custom responses.
-
         .. code-block:: python
 
             from connexion import AsyncApp
@@ -40,17 +40,6 @@ problem responses.
     .. tab-item:: FlaskApp
         :sync: FlaskApp
 
-        You can register error handlers on:
-
-        - The exception class to handle
-          If this exception class is raised somewhere in your application or the middleware stack,
-          it will be passed to your handler.
-        - The HTTP status code to handle
-          Connexion will raise ``starlette.HTTPException`` errors when it encounters any issues
-          with a request or response. The underlying Flask application will raise
-          ``werkzeug.HTTPException`` errors. You can intercept both of these exceptions with
-          specific status codes if you want to return custom responses.
-
         .. code-block:: python
 
             from connexion import FlaskApp
@@ -69,20 +58,34 @@ problem responses.
             .. automethod:: connexion.FlaskApp.add_error_handler
                 :noindex:
 
-    .. tab-item:: ConnexionMiddleware
-        :sync: ConnexionMiddleware
+        .. note::
+
+            .. warning::
+
+                ⚠️ **The following is not recommended as it complicates the exception handling logic,**
 
-        You can register error handlers on:
+            You can also register error handlers on the underlying flask application directly.
 
-        - The exception class to handle
-          If this exception class is raised somewhere in your application or the middleware stack,
-          it will be passed to your handler.
-        - The HTTP status code to handle
-          Connexion will raise ``starlette.HTTPException`` errors when it encounters any issues
-          with a request or response. You can intercept these exceptions with specific status codes
-          if you want to return custom responses.
-          Note that this might not catch ``HTTPExceptions`` with the same status code raised by
-          your wrapped ASGI/WSGI framework.
+            .. code-block:: python
+
+                flask_app = app.app
+                flask_app.register_error_handler(FileNotFoundError, not_found)
+                flask_app.register_error_handler(404, not_found)
+
+            `Flask documentation`_
+
+            Error handlers registered this way:
+
+            - Will only intercept exceptions thrown in the application, not in the Connexion
+              middleware.
+            - Can intercept exceptions before they reach the error handlers registered on the
+              connexion app.
+            - When registered on status code, will intercept only
+              ``werkzeug.exceptions.HTTPException`` thrown by werkzeug / Flask not
+              ``starlette.exceptions.HTTPException``.
+
+    .. tab-item:: ConnexionMiddleware
+        :sync: ConnexionMiddleware
 
         .. code-block:: python
 
@@ -105,10 +108,17 @@ problem responses.
             .. automethod:: connexion.ConnexionMiddleware.add_error_handler
                 :noindex:
 
+        .. note::
+
+            This might not catch ``HTTPExceptions`` with the same status code raised by
+            your wrapped ASGI/WSGI framework.
+
 .. note::
 
     Error handlers can be ``async`` coroutines as well.
 
+.. _Flask documentation: https://flask.palletsprojects.com/en/latest/errorhandling/#error-handlers
+
 Default Exception Handling
 --------------------------
 By default connexion exceptions are JSON serialized according to

diff --git a/docs/v3.rst b/docs/v3.rst
@@ -152,8 +152,9 @@ Smaller breaking changes
   has been added to work with Flask's ``MethodView`` specifically.
 * Built-in support for uWSGI has been removed. You can re-add this functionality using a custom middleware.
 * The request body is now passed through for ``GET``, ``HEAD``, ``DELETE``, ``CONNECT`` and ``OPTIONS`` methods as well.
-* Error handlers registered on the on the underlying Flask app directly will be ignored. You
-  should register them on the Connexion app directly.
+* The signature of error handlers has changed and default Flask error handlers are now replaced
+  with default Connexion error handlers which work the same for ``AsyncApp`` and
+  ``ConnexionMiddleware``.
 
 
 Non-breaking changes

diff --git a/tests/api/test_bootstrap.py b/tests/api/test_bootstrap.py
@@ -1,3 +1,4 @@
+import json
 from unittest import mock
 
 import jinja2
@@ -7,6 +8,7 @@
 from connexion.exceptions import InvalidSpecification
 from connexion.http_facts import METHODS
 from connexion.json_schema import ExtendedSafeLoader
+from connexion.lifecycle import ConnexionRequest, ConnexionResponse
 from connexion.middleware.abstract import AbstractRoutingAPI
 from connexion.options import SwaggerUIOptions
 
@@ -302,10 +304,15 @@ def test_add_error_handler(app_class, simple_api_spec_dir):
     app = app_class(__name__, specification_dir=simple_api_spec_dir)
     app.add_api("openapi.yaml")
 
-    def custom_error_handler(_request, _exception):
-        pass
+    def not_found(request: ConnexionRequest, exc: Exception) -> ConnexionResponse:
+        return ConnexionResponse(
+            status_code=404, body=json.dumps({"error": "NotFound"})
+        )
 
-    app.add_error_handler(Exception, custom_error_handler)
-    app.add_error_handler(500, custom_error_handler)
+    app.add_error_handler(404, not_found)
 
-    app.middleware._build_middleware_stack()
+    app_client = app.test_client()
+
+    response = app_client.get("/does_not_exist")
+    assert response.status_code == 404
+    assert response.json()["error"] == "NotFound"