From 70084bcc4c21713cbf861841ebda977263d3ab8c Mon Sep 17 00:00:00 2001 From: Robbe Sneyders Date: Mon, 6 Nov 2023 19:00:46 +0100 Subject: [PATCH] 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. --- connexion/apps/flask.py | 15 +++++- connexion/http_facts.py | 67 ++++++++++++++++++++++++++ connexion/lifecycle.py | 3 +- connexion/middleware/exceptions.py | 28 +++-------- docs/exceptions.rst | 76 +++++++++++++++++------------- docs/v3.rst | 5 +- tests/api/test_bootstrap.py | 17 +++++-- 7 files changed, 147 insertions(+), 64 deletions(-) diff --git a/connexion/apps/flask.py b/connexion/apps/flask.py index 98345ff8d..c93ab1f76 100644 --- 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 index 21c9c7f29..a715ed394 100644 --- 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 index b7be1eae6..838294a6e 100644 --- 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 index 737c200bd..33688b840 100644 --- 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 index e12048ce0..dd77f68f1 100644 --- 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 index ad9700936..db8e87e11 100644 --- 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 index c0a4d44df..266ac36c4 100644 --- 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"