Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving error handlers to satisfy both template and API server use cases #640

Open
TGoddessana opened this issue Apr 25, 2024 · 6 comments

Comments

@TGoddessana
Copy link
Contributor

TGoddessana commented Apr 25, 2024

So I'm using the flask server for two things.

  1. a template-based server-side rendering server. It returns a web page.
  2. Building an API server using flask-smorest.

Here I ran into a problem with handling errors. If I wanted the server to handle errors like 404 in the template use case, I could register an error handler function to return the appropriate template. like this:

def _register_error_handlers(app):
    for errcode in [401, 403, 404, 500]:
        @app.errorhandler(errcode)
        def render_error(error):
            error_code = getattr(error, "code", 500)
            return render_template(f"error_{error_code}.html"), error_code

But the problem is that this overrides the error handler defined by flask-smorest. If I throw a 404 error in the API use case (flask_smorest.abort()), it will be caught by the error handler I defined "for templates" and will result in an HTML error, as opposed to the intended JSON response.

It wasn't easy to find a way to handle both of these methods.
Here's what I think is the ideal error handling provided by flask-smoret.

from flask import abort as flask_abort # errors in the default flask
from flask_smorest import abort as api_abort # api errors

# pseudocode...
@blp.response(UserSchema)
def users_api():
    # omit
    if not user:
        api_abort(404) # this returns a 404 JSON response (caught by the error handler currently present in flask-smorest)

@blp.response
def users_view():
    # similarly omit
    if not user:
        flask_abort() # this raises an HttpException from werkzeug.

I was thinking that flask-smorest could arbitrarily subclass HTTPException and register it in ErrorHandlerMixin, but I'd like to know what you think about this.

@lafrech
Copy link
Member

lafrech commented Apr 29, 2024

Related to #51 and #174.

As I wrote in #51, the use case of API and website seems like a corner case to me. I don't mind supporting it if it doesn't make things more complicated for the typical use case.

The way it works currently, any HttpException is treated by flask-smorest handler. Per your suggestion, the user would have to use flask_smorest.import abort explicitly. Doesn't seem like a huge constraint, I think I already do that in my code. Could be an issue for third-party code such as e.g. authentication libs. The user would have to wrap the code to catch the error and raise it with the proper abort.

I don't know. Would you like to elaborate more about "flask-smorest could arbitrarily subclass HTTPException and register it in ErrorHandlerMixin"?

@TGoddessana
Copy link
Contributor Author

How are you?
Thank you very much for maintaining the project.

I don't know. Would you like to elaborate more about "flask-smorest could arbitrarily subclass HTTPException and register it in ErrorHandlerMixin"?

Yes, I tried to create a minimal example to reproduce this.
I think this satisfies my use case.

from flask import Flask
from flask import abort
from flask_smorest import Api
from flask_smorest import abort as api_abort

app = Flask(__name__)

app.config['API_TITLE'] = 'My API'
app.config['API_VERSION'] = 'v1'
app.config['OPENAPI_VERSION'] = '3.0.2'

api = Api(app)


@app.route("/api")
def helloapi():
    api_abort(404)


@app.route("/website")
def hellowebsite():
    abort(404)


if __name__ == '__main__':
    app.run(debug=True)

This is the example application code. As mentioned, you can see that I'm importing abort from flask_smorest under the name api_abort.

# flask_smorest.exceptions.py

class ApiException(wexc.HTTPException):
    """
    Base class for all API exceptions,
    Flask-Smoest will catch this exception and return the response in json format
    """


class ApiNotFound(ApiException, wexc.NotFound):
    """404 Not Found"""


class ApiMethodNotAllowed(ApiException, wexc.MethodNotAllowed):
    """405 Method Not Allowed"""


class ApiConflict(ApiException, wexc.Conflict):
    """409 Conflict"""


def find_exception_by_code(code: int) -> ApiException:
    """Find an apiexception, by code"""

    for exception in ApiException.__subclasses__():
        if exception.code == code:
            return exception

And like above, we define a class called ApiException in flask-smorest.exception and subclass it and several exception classes provided by WerkZeug to throw Api{exception} .
I also created a helper function called find_exception_by_code to make it easier to find exceptions that subclass ApiException with a status code.

def abort(
    http_status_code: int, exc: Exception | None = None, **kwargs: typing.Any
) -> typing.NoReturn:
    """Raise a HTTPException for the given http_status_code. Attach any keyword
    arguments to the exception for later processing.

    From Flask-Restful. See NOTICE file for license information.
    """
    try:
        raise find_exception_by_code(http_status_code)(**kwargs)
    except ApiException as err:
        err.data = kwargs
        err.exc = exc
        raise err


    # try:
    #     flask.abort(http_status_code)
    # except HTTPException as err:
    #     err.data = kwargs  # type: ignore
    #     err.exc = exc  # type: ignore
    #     raise err

Now modify abort() in webargs.flaskparser like this. This is for simplicity's sake and may break any use cases or backwards compatibility, but I think it's a good example to show what I'm trying to accomplish. Find the classes that subclass ApiException, and throw exceptions on them. Even if it's not a generic code like 404, 403, 503, but something you define yourself, you can still throw an exception.

class ErrorHandlerMixin:
    # ...

    def _register_error_handlers(self):
        """Register error handlers in Flask app

        This method registers a default error handler for ``HTTPException``.
        """

        # set scope
        self._app.register_error_handler(ApiException, self.handle_http_exception)

Now, in the ErrorHandlerMixin, we explicitly specify the scope for handling exceptions as above. This way, flask-smoerst can only handle ApiExceptions...

(in /api route)
image

(in /website route)
image

I think there is still a lot to consider (how do we maintain backwards compatibility? If flask-smorest.abort() is given an exception other than ApiException as a parameter, how should it be handled? etc...), but I think this can be a big help in running API servers and template-based servers simultaneously by clearly separating the responsibility for handling API-related and non-API-related errors.

@lafrech
Copy link
Member

lafrech commented Apr 30, 2024

Thanks for the details. This makes sense.

There might be a way to create all the exceptions dynamically from the HttpException subclasses. And we could make find_exception_by_code a classmethod.

I haven't been giving too much thought to backward compatibility, but it might even be a no breaker. When given a non Api exception, re-raise.

This looks like it is worth going further.

@TGoddessana
Copy link
Contributor Author

TGoddessana commented May 1, 2024

thanks :) and i'm happy to start a PR.

There might be a way to create all the exceptions dynamically from the HttpException subclasses. And we could make find_exception_by_code a classmethod.

wolud you explain about this more? what i thought was like this... (Above, we have defined a new class called ApiException, but looking at the original source code, it seems that FlaskSmorestError inherits from werkzeug's HttpException to create an API error. I think it would be more consistent to inherit from FlaskSmorestError.)

class FlaskSmorestError(Exception):
    """Generic flask-smorest exception"""


class MissingAPIParameterError(FlaskSmorestError):
    """Missing API parameter"""


class BadRequest(wexc.BadRequest, FlaskSmorestError):
    ...


class Unauthorized(wexc.Unauthorized, FlaskSmorestError):
    ...


class Forbidden(wexc.Forbidden, FlaskSmorestError):
    ...


class NotFound(wexc.NotFound, FlaskSmorestError):
    ...


class MethodNotAllowed(wexc.MethodNotAllowed, FlaskSmorestError):
    ...


class NotAcceptable(wexc.NotAcceptable, FlaskSmorestError):
    ...

but this have too much duplicated code, i'm wondering is there any good solution for this..

@lafrech
Copy link
Member

lafrech commented May 1, 2024

I was thinking of something along the lines of

exceptions.py

import sys
module = sys.modules[__name__]
for exc in ...:  # Iterate over Werkzeug exceptions
    class Exc(exc, FlaskSmorestError):
        ...
    setattr(module, exc.__name__, SmorestExc)

@TGoddessana
Copy link
Contributor Author

TGoddessana commented May 4, 2024

Sorry for the delay.. I just submitted a PR #644, but the test I arbitrarily overridden is not passing. I thought that when a user arbitrarily throws an ApiException(), a JSON 500 error should be thrown, but I'm having trouble finding an elegant way to implement it (the current implementation returns a 500 Internal Error html).
I would appreciate it if you could take a look at it when you have time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants