Merge PR #422 into 16.0

Signed-off-by lmignon

fastapi/README.rst

@@ -1268,66 +1268,25 @@ you be consistent when writing a route handler for a search route.
     dependency between the **'odoo-addon-fastapi'** module and the **'odoo-addon-extendable'**
 
 
-Customization of the error handling
-===================================
-
-The error handling a very important topic in the design of the fastapi integration
-with odoo. It must ensure that the error messages are properly return to the client
-and that the transaction is properly roll backed. The **'fastapi'** module provides
-a way to register custom error handlers. The **'odoo.addons.fastapi.error_handlers'**
-module provides the default error handlers that are registered by default when
-a new instance of the **'FastAPI'** class is created. When an app is initialized in
-'fastapi.endpoint' model, the method `_get_app_exception_handlers` is called to
-get a dictionary of error handlers. This method is designed to be overridden
-in a custom module to provide custom error handlers. You can override the handler
-for a specific exception class or you can add a new handler for a new exception
-or even replace all the handlers by your own handlers. Whatever you do, you must
-ensure that the transaction is properly roll backed.
-
-Some could argue that the error handling can't be extended since the error handlers
-are global method not defined in an odoo model. Since the method providing the
-the error handlers definitions is defined on the 'fastapi.endpoint' model, it's
-not a problem at all, you just need to think another way to do it that by inheritance.
-
-A solution could be to develop you own error handler to be able to process the
-error and chain the call to the default error handler.
+Error handling
+==============
+
+The error handling is a very important topic in the design of the fastapi integration
+with odoo. By default, when instantiating the fastapi app, the fastapi library
+declare a default exception handler that will catch any exception raised by the
+route handlers and return a proper error response. This is done to ensure that
+the serving of the app is not interrupted by an unhandled exception. If this
+implementation makes sense for a native fastapi app, it's not the case for the
+fastapi integration with odoo. The transactional nature of the calls to
+odoo's api is implemented at the root of the request processing by odoo. To ensure
+that the transaction is properly managed, the integration with odoo must ensure
+that the exceptions raised by the route handlers properly bubble up to the
+handling of the request by odoo. This is done by the monkey patching of the
+registered exception handler of the fastapi app in the
+**'odoo.addons.fastapi.models.error_handlers'** module. As a result, it's no
+longer possible to define a custom exception handler in your fastapi app. If you
+add a custom exception handler in your app, it will be ignored.
 
-.. code-block:: python
-
-    class MyCustomErrorHandler():
-        def __init__(self, next_handler):
-            self.next_handler = next_handler
-
-        def __call__(self, request: Request, exc: Exception) -> JSONResponse:
-            # do something with the error
-            response = self.next_handler(request, exc)
-            # do something with the response
-            return response
-
-
-With this solution, you can now register your custom error handler by overriding
-the method `_get_app_exception_handlers` in your custom module.
-
-.. code-block:: python
-
-    class FastapiEndpoint(models.Model):
-        _inherit = "fastapi.endpoint"
-
-        def _get_app_exception_handlers(
-            self,
-        ) -> Dict[
-            Union[int, Type[Exception]],
-            Callable[[Request, Exception], Union[Response, Awaitable[Response]]],
-        ]:
-            handlers = super()._get_app_exception_handlers()
-            access_error_handler = handlers.get(odoo.exceptions.AccessError)
-            handlers[odoo.exceptions.AccessError] = MyCustomErrorHandler(access_error_handler)
-            return handlers
-
-In the previous example, we extend the error handler for the 'AccessError' exception
-for all the endpoints. You can do the same for a specific app by checking the
-'app' field of the 'fastapi.endpoint' record before registering your custom error
-handler.
 
 FastAPI addons directory structure
 ==================================

fastapi/__init__.py

@@ -1,2 +1,3 @@
 from . import models
 from . import fastapi_dispatcher
+from . import error_handlers

fastapi/error_handlers.py

@@ -1,80 +1,36 @@
 # Copyright 2022 ACSONE SA/NV
 # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
 
-import logging
 
+from starlette.exceptions import WebSocketException
+from starlette.middleware.errors import ServerErrorMiddleware
+from starlette.middleware.exceptions import ExceptionMiddleware
 from starlette.responses import JSONResponse
-from starlette.status import (
-    HTTP_400_BAD_REQUEST,
-    HTTP_403_FORBIDDEN,
-    HTTP_404_NOT_FOUND,
-    HTTP_500_INTERNAL_SERVER_ERROR,
-)
-
-import odoo
+from starlette.websockets import WebSocket
 
 from fastapi import Request
-from fastapi.exception_handlers import http_exception_handler
-from fastapi.exceptions import HTTPException
-
-from .context import odoo_env_ctx
-
-_logger = logging.getLogger(__name__)
-
-
-def _rollback(request: Request, reason: str) -> None:
-    cr = odoo_env_ctx.get().cr
-    if cr is not None:
-        _logger.debug("rollback on %s", reason)
-        cr.rollback()
-
-
-async def _odoo_user_error_handler(
-    request: Request, exc: odoo.exceptions.UserError
-) -> JSONResponse:
-    _rollback(request, "UserError")
-    return await http_exception_handler(
-        request, HTTPException(HTTP_400_BAD_REQUEST, exc.args[0])
-    )
-
 
-async def _odoo_access_error_handler(
-    request: Request, _exc: odoo.exceptions.AccessError
-) -> JSONResponse:
-    _rollback(request, "AccessError")
-    return await http_exception_handler(
-        request, HTTPException(HTTP_403_FORBIDDEN, "AccessError")
-    )
+# we need to monkey patch the ServerErrorMiddleware and ExceptionMiddleware classes
+# to ensure that all the exceptions that are handled by these specific
+# middlewares are let to bubble up to the retrying mechanism and the
+# dispatcher error handler to ensure that appropriate action are taken
+# regarding the transaction, environment, and registry. These middlewares
+# are added by default by FastAPI when creating an application and it's not
+# possible to remove them. So we need to monkey patch them.
 
 
-async def _odoo_missing_error_handler(
-    request: Request, _exc: odoo.exceptions.MissingError
+def pass_through_exception_handler(
+    self, request: Request, exc: Exception
 ) -> JSONResponse:
-    _rollback(request, "MissingError")
-    return await http_exception_handler(
-        request, HTTPException(HTTP_404_NOT_FOUND, "MissingError")
-    )
+    raise exc
 
 
-async def _odoo_validation_error_handler(
-    request: Request, exc: odoo.exceptions.ValidationError
-) -> JSONResponse:
-    _rollback(request, "ValidationError")
-    return await http_exception_handler(
-        request, HTTPException(HTTP_400_BAD_REQUEST, exc.args[0])
-    )
-
-
-async def _odoo_http_exception_handler(
-    request: Request, exc: HTTPException
-) -> JSONResponse:
-    _rollback(request, "HTTPException")
-    return await http_exception_handler(request, exc)
+def pass_through_websocket_exception_handler(
+    self, websocket: WebSocket, exc: WebSocketException
+) -> None:
+    raise exc
 
 
-async def _odoo_exception_handler(request: Request, exc: Exception) -> JSONResponse:
-    _rollback(request, "Exception")
-    _logger.exception("Unhandled exception", exc_info=exc)
-    return await http_exception_handler(
-        request, HTTPException(HTTP_500_INTERNAL_SERVER_ERROR, str(exc))
-    )
+ServerErrorMiddleware.error_response = pass_through_exception_handler
+ExceptionMiddleware.http_exception = pass_through_exception_handler
+ExceptionMiddleware.websocket_exception = pass_through_websocket_exception_handler

fastapi/fastapi_dispatcher.py

@@ -4,10 +4,17 @@
 from contextlib import contextmanager
 from io import BytesIO
 
-from werkzeug.exceptions import InternalServerError
+from starlette import status
+from starlette.exceptions import HTTPException
+from werkzeug.exceptions import HTTPException as WerkzeugHTTPException
 
+from odoo.exceptions import AccessDenied, AccessError, MissingError, UserError
 from odoo.http import Dispatcher, request
 
+from fastapi.encoders import jsonable_encoder
+from fastapi.exceptions import RequestValidationError, WebSocketRequestValidationError
+from fastapi.utils import is_body_allowed_for_status_code
+
 from .context import odoo_env_ctx
 
 
@@ -32,19 +39,65 @@ def dispatch(self, endpoint, args):
         with self._manage_odoo_env(uid):
             for r in app(environ, self._make_response):
                 data.write(r)
+            if self.inner_exception:
+                raise self.inner_exception
             return self.request.make_response(
                 data.getvalue(), headers=self.headers, status=self.status
             )
 
     def handle_error(self, exc):
-        # At this stage all the normal exceptions are handled by FastAPI
-        # and we should only have InternalServerError occurring after the
-        # FastAPI app has been called.
-        return InternalServerError()  # pragma: no cover
+        headers = getattr(exc, "headers", None)
+        status_code = status.HTTP_500_INTERNAL_SERVER_ERROR
+        details = "Internal Server Error"
+        if isinstance(exc, WerkzeugHTTPException):
+            status_code = exc.code
+            details = exc.description
+        elif isinstance(exc, HTTPException):
+            status_code = exc.status_code
+            details = exc.detail
+        elif isinstance(exc, RequestValidationError):
+            status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
+            details = jsonable_encoder(exc.errors())
+        elif isinstance(exc, WebSocketRequestValidationError):
+            status_code = status.WS_1008_POLICY_VIOLATION
+            details = jsonable_encoder(exc.errors())
+        elif isinstance(exc, (AccessDenied, AccessError)):
+            status_code = status.HTTP_403_FORBIDDEN
+            details = "AccessError"
+        elif isinstance(exc, MissingError):
+            status_code = status.HTTP_404_NOT_FOUND
+            details = "MissingError"
+        elif isinstance(exc, UserError):
+            status_code = status.HTTP_400_BAD_REQUEST
+            details = exc.args[0]
+        body = {}
+        if is_body_allowed_for_status_code(status_code):
+            # use the same format as in
+            # fastapi.exception_handlers.http_exception_handler
+            body = {"detail": details}
+        return self.request.make_json_response(
+            body, status=status_code, headers=headers
+        )
 
     def _make_response(self, status_mapping, headers_tuple, content):
         self.status = status_mapping[:3]
         self.headers = dict(headers_tuple)
+        self.inner_exception = None
+        # in case of exception, the method asgi_done_callback of the
+        # ASGIResponder will trigger an "a2wsgi.error" event with the exception
+        # instance stored in a tuple with the type of the exception and the traceback.
+        # The event loop will then be notified and then call the `error_response`
+        # method of the ASGIResponder. This method will then call the
+        # `_make_response` method provided as callback to the app with the tuple
+        # of the exception as content. In this case, we store the exception
+        # instance in the `inner_exception` attribute to be able to raise it
+        # in the `dispatch` method.
+        if (
+            isinstance(content, tuple)
+            and len(content) == 3
+            and isinstance(content[1], Exception)
+        ):
+            self.inner_exception = content[1]
 
     def _get_environ(self):
         try:

fastapi/models/fastapi_endpoint.py

@@ -4,17 +4,17 @@
 import logging
 from functools import partial
 from itertools import chain
-from typing import Any, Awaitable, Callable, Dict, List, Tuple, Type, Union
+from typing import Any, Callable, Dict, List, Tuple
 
 from a2wsgi import ASGIMiddleware
 from starlette.middleware import Middleware
+from starlette.routing import Mount
 
-import odoo
 from odoo import _, api, exceptions, fields, models, tools
 
-from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response
+from fastapi import APIRouter, Depends, FastAPI
 
-from .. import dependencies, error_handlers
+from .. import dependencies
 
 _logger = logging.getLogger(__name__)
 
@@ -201,8 +201,24 @@ def get_app(self, root_path):
             return None
         app = FastAPI()
         app.mount(record.root_path, record._get_app())
+        self._clear_fastapi_exception_handlers(app)
         return ASGIMiddleware(app)
 
+    def _clear_fastapi_exception_handlers(self, app: FastAPI) -> None:
+        """
+        Clear the exception handlers of the given fastapi app.
+
+        This method is used to ensure that the exception handlers are handled
+        by odoo and not by fastapi. We therefore need to remove all the handlers
+        added by default when instantiating a FastAPI app. Since apps can be
+        mounted recursively, we need to apply this method to all the apps in the
+        mounted tree.
+        """
+        app.exception_handlers = {}
+        for route in app.routes:
+            if isinstance(route, Mount):
+                self._clear_fastapi_exception_handlers(route.app)
+
     @api.model
     @tools.ormcache("root_path")
     def get_uid(self, root_path):
@@ -216,8 +232,6 @@ def _get_app(self) -> FastAPI:
         for router in self._get_fastapi_routers():
             app.include_router(router=router)
         app.dependency_overrides.update(self._get_app_dependencies_overrides())
-        for exception, handler in self._get_app_exception_handlers().items():
-            app.add_exception_handler(exception, handler)
         return app
 
     def _get_app_dependencies_overrides(self) -> Dict[Callable, Callable]:
@@ -226,38 +240,6 @@ def _get_app_dependencies_overrides(self) -> Dict[Callable, Callable]:
             dependencies.company_id: partial(lambda a: a, self.company_id.id),
         }
 
-    def _get_app_exception_handlers(
-        self,
-    ) -> Dict[
-        Union[int, Type[Exception]],
-        Callable[[Request, Exception], Union[Response, Awaitable[Response]]],
-    ]:
-        """Return a dict of exception handlers to register on the app
-
-        The key is the exception class or status code to handle.
-        The value is the handler function.
-
-        If you need to register your own handler, you can do it by overriding
-        this method and calling super(). Changes done in this way will be applied
-        to all the endpoints. If you need to register a handler only for a specific
-        endpoint, you can do it by overriding the _get_app_exception_handlers method
-        and conditionally returning your specific handlers only for the endpoint
-        you want according to the self.app value.
-
-        Be careful to not forget to roll back the transaction when you implement
-        your own error handler. If you don't, the transaction will be committed
-        and the changes will be applied to the database.
-        """
-        self.ensure_one()
-        return {
-            Exception: error_handlers._odoo_exception_handler,
-            HTTPException: error_handlers._odoo_http_exception_handler,
-            odoo.exceptions.UserError: error_handlers._odoo_user_error_handler,
-            odoo.exceptions.AccessError: error_handlers._odoo_access_error_handler,
-            odoo.exceptions.MissingError: error_handlers._odoo_missing_error_handler,
-            odoo.exceptions.ValidationError: error_handlers._odoo_validation_error_handler,
-        }
-
     def _prepare_fastapi_app_params(self) -> Dict[str, Any]:
         """Return the params to pass to the Fast API app constructor"""
         return {