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

Improve errorhandler #644

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TGoddessana
Copy link
Contributor

There are a few issues... (shouldn't be merged yet)

  1. The abort(code) works, but raise ApiException() doesn't work with the json response, in my opinion, raising an ApiException should automatically result in a 500 json handling, but it's not.
    The flask_abort() and api_abort() use cases work anyway... I just want to handle them elegantly. (What should be the behavior if the user raises ApiException() instead of flask_smorest.abort()?)

  2. I looked at the method you mentioned in the original issue(re-define all werkzeug exceptions), and my guess is that since we are inheriting and defining some exceptions from Flask-Smorest, it would be better to explicitly inherit them all, even if it makes the code a bit longer (I'd love to hear your opinion).

Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort on this.

I'm not really a fan of the idea of redefining all exceptions manually, although I reckon they shouldn't change much upstream. I'd find it nice if we could create those exceptions automatically, then add the already existing changes.

Regarding raising ApiException, ideally it should be treated like an api_abort. We could decide the exception itself is private API and users must use api_abort, but I'd rather not.

.gitignore Outdated Show resolved Hide resolved
for exception in cls.__subclasses__():
if exception.code == code:
return exception
return InternalServerError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd return a ValueError here as code is wrong (it will result in an internal error anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# API exceptions
class ApiException(wexc.HTTPException, FlaskSmorestError):
    """A generic API error."""

    @classmethod
    def get_exception_by_code(cls, code):
        for exception in cls.__subclasses__():
            if exception.code == code:
                return exception
        return ValueError(f"Not a standard HTTP status code: {code}")

I think this error message is appropriate, but if you have another one, please let me know.. :)

code = 304
description = "Resource not modified since last request."
# API exceptions
class ApiException(wexc.HTTPException, FlaskSmorestError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering it it needs to inherit HTTPException. Perhaps. No objection, just wondering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class NotModified(wexc.HTTPException, FlaskSmorestError):
    """Resource was not modified (Etag is unchanged)

    Exception created to compensate for a lack in Werkzeug (and Flask)
    """

    code = 304
    description = "Resource not modified since last request."


class PreconditionRequired(wexc.PreconditionRequired, FlaskSmorestError):
    """Etag required but missing"""

    # Overriding description as we don't provide If-Unmodified-Since
    description = 'This request is required to be conditional; try using "If-Match".'


class PreconditionFailed(wexc.PreconditionFailed, FlaskSmorestError):
    """Etag required and wrong ETag provided"""

This is part of original exceptions.py for flask_smorest. As you can see here, flask_smorest inherits and overrides a few HttpExceptions, so I wanted to make sure that I was calling them "http API errors", so I overridden them. (This is good, in my opinion, because it makes it clear what are API errors and what are internal errors that are not).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class PreconditionRequired(wexc.PreconditionRequired, ApiException):
    """Etag required but missing"""

    # Overriding description as we don't provide If-Unmodified-Since
    description = 'This request is required to be conditional; try using "If-Match".'


class NotModified(ApiException):
    """Resource was not modified (Etag is unchanged)

    Exception created to compensate for a lack in Werkzeug (and Flask)
    """

    code = 304
    description = "Resource not modified since last request."


for code, exc in wexc.default_exceptions.items():
    api_exc = type(exc.__name__, (exc, ApiException), {})
    setattr(module, exc.__name__, api_exc)

As you said, with this approach, we could subclass all Werkzeug errors to our exception class.

image

I'm a little curious here. Should we also allow users to import and use syntax like from flask_smorest.exceptions import NotFound? Because it doesn't seem to work when importing in code, not in the REPL.

image

For example, what code should a user use to get a 404 error?

# 1
from flask_smorest.exceptions import ApiException
raise ApiException.get_exception_by_code(404)(...)

# 2
from flask_smorest imort abort
abort(404)

# 3
from flask_smorest.exceptions import NotFound
raise NotFound(...)

#4 
# maybe.. is this possible?
from flask_smorest.exceptions import ApiException
raise ApiException(404)

Would love to hear your thoughts :)

err.data = kwargs
err.exc = exc
raise err
except Exception as err:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of catching any exception to re-raise it?

def abort(http_status_code, exc=None, **kwargs):
try:
raise ApiException.get_exception_by_code(http_status_code)
except ApiException as err:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to raise and catch? Can't we tweak the instance before raising?

description = "Resource not modified since last request."


def abort(http_status_code, exc=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to keep only exceptions in this file.

We may want to put this somewhere else. It could be in error_handler.py.

Copy link
Contributor Author

@TGoddessana TGoddessana May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix the location of that code, I think it's a good idea to fix it as you say

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

Successfully merging this pull request may close these issues.

2 participants