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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/flask_smorest/__init__.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
"""Api extension initialization"""

from webargs.flaskparser import abort # noqa

from .spec import APISpecMixin
from .blueprint import Blueprint # noqa
from .pagination import Page # noqa
from .error_handler import ErrorHandlerMixin
from .exceptions import abort # noqa
from .globals import current_api # noqa
from .pagination import Page # noqa
from .spec import APISpecMixin
from .utils import PrefixedMappingProxy, normalize_config_prefix


Expand Down
15 changes: 5 additions & 10 deletions src/flask_smorest/error_handler.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
"""Exception handler"""

from werkzeug.exceptions import HTTPException

import marshmallow as ma

from flask_smorest.exceptions import ApiException


class ErrorSchema(ma.Schema):
"""Schema describing the error payload
Expand All @@ -28,24 +28,19 @@ def _register_error_handlers(self):

This method registers a default error handler for ``HTTPException``.
"""
self._app.register_error_handler(HTTPException, self.handle_http_exception)
self._app.register_error_handler(ApiException, self.handle_http_exception)

def handle_http_exception(self, error):
"""Return a JSON response containing a description of the error

This method is registered at app init to handle ``HTTPException``.
This method is registered at app init to handle ``ApiException``.

- When ``abort`` is called in the code, an ``HTTPException`` is
- When ``abort`` is called in the code, an ``ApiException`` is
triggered and Flask calls this handler.

- When an exception is not caught in a view, Flask makes it an
``InternalServerError`` and calls this handler.

flask-smorest republishes webargs's
:func:`abort <webargs.flaskparser.abort>`. This ``abort`` allows the
caller to pass kwargs and stores them in ``exception.data`` so that the
error handler can use them to populate the response payload.

Extra information expected by this handler:

- `message` (``str``): a comment
Expand Down
158 changes: 147 additions & 11 deletions src/flask_smorest/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,166 @@ class FlaskSmorestError(Exception):
"""Generic flask-smorest exception"""


# Non-API exceptions
class MissingAPIParameterError(FlaskSmorestError):
"""Missing API parameter"""


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

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

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 :)

"""A generic API error."""

@classmethod
def get_exception_by_code(cls, code):
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.. :)



class BadRequest(wexc.BadRequest, ApiException):
"""Bad request"""


class Unauthorized(wexc.Unauthorized, ApiException):
"""Unauthorized access"""


class Forbidden(wexc.Forbidden, ApiException):
"""Forbidden access"""


class NotFound(wexc.NotFound, ApiException):
"""Resource not found"""


class MethodNotAllowed(wexc.MethodNotAllowed, ApiException):
"""Method not allowed"""


class NotAcceptable(wexc.NotAcceptable, ApiException):
"""Not acceptable"""


class RequestTimeout(wexc.RequestTimeout, ApiException):
"""Request timeout"""


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


class Gone(wexc.Gone, ApiException):
"""Resource gone"""


class LengthRequired(wexc.LengthRequired, ApiException):
"""Length required"""


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


class RequestEntityTooLarge(wexc.RequestEntityTooLarge, ApiException):
"""Request entity too large"""


class RequestURITooLarge(wexc.RequestURITooLarge, ApiException):
"""Request URI too large"""


class UnsupportedMediaType(wexc.UnsupportedMediaType, ApiException):
"""Unsupported media type"""


class RequestedRangeNotSatisfiable(wexc.RequestedRangeNotSatisfiable, ApiException):
"""Requested range not satisfiable"""


class PreconditionRequired(wexc.PreconditionRequired, FlaskSmorestError):
class ExpectationFailed(wexc.ExpectationFailed, ApiException):
"""Expectation failed"""


class ImATeapot(wexc.ImATeapot, ApiException):
"""I'm a teapot"""


class UnprocessableEntity(wexc.UnprocessableEntity, ApiException):
"""Unprocessable entity"""


class Locked(wexc.Locked, ApiException):
"""Locked"""


class FailedDependency(wexc.FailedDependency, ApiException):
"""Failed dependency"""


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 PreconditionFailed(wexc.PreconditionFailed, FlaskSmorestError):
"""Etag required and wrong ETag provided"""
class TooManyRequests(wexc.TooManyRequests, ApiException):
"""Too many requests"""


class CurrentApiNotAvailableError(FlaskSmorestError):
"""`current_api` not available"""
class RequestHeaderFieldsTooLarge(wexc.RequestHeaderFieldsTooLarge, ApiException):
"""Request header fields too large"""


class UnavailableForLegalReasons(wexc.UnavailableForLegalReasons, ApiException):
"""Unavailable for legal reasons"""


class InternalServerError(wexc.InternalServerError, ApiException):
"""Internal server error"""


class NotImplemented(wexc.NotImplemented, ApiException):
"""Not implemented"""


class BadGateway(wexc.BadGateway, ApiException):
"""Bad gateway"""


class ServiceUnavailable(wexc.ServiceUnavailable, ApiException):
"""Service unavailable"""


class GatewayTimeout(wexc.GatewayTimeout, ApiException):
"""Gateway timeout"""


class HTTPVersionNotSupported(wexc.HTTPVersionNotSupported, ApiException):
"""HTTP version not supported"""


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."


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

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?

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?

raise err
58 changes: 46 additions & 12 deletions tests/test_error_handler.py
Original file line number Diff line number Diff line change
@@ -1,39 +1,73 @@
import pytest

from flask import abort as flask_abort
from werkzeug.exceptions import InternalServerError, default_exceptions

from flask_smorest import Api, abort
from flask_smorest import Api
from flask_smorest import abort as api_abort
from flask_smorest.exceptions import ApiException


class TestErrorHandler:
@pytest.mark.parametrize("code", default_exceptions)
def test_error_handler_on_abort(self, app, code):
def test_error_handler_on_api_abort(self, app, code):
client = app.test_client()

@app.route("/abort")
@app.route("/api-abort")
def test_abort():
abort(code)
api_abort(code)

Api(app)

response = client.get("/abort")
response = client.get("/api-abort")
assert response.status_code == code
assert response.content_type == "application/json"
assert response.json["code"] == code
assert response.json["status"] == default_exceptions[code]().name

def test_error_handler_on_unhandled_error(self, app):
@pytest.mark.parametrize("code", default_exceptions)
def test_error_handler_on_default_abort(self, app, code):
client = app.test_client()

@app.route("/html-abort")
def test_abort():
flask_abort(code)

Api(app)

response = client.get("/html-abort")
assert response.status_code == code
assert response.content_type == "text/html; charset=utf-8"

def test_flask_error_handler_on_unhandled_error(self, app):
# Unset TESTING to let Flask return 500 on unhandled exception
app.config["TESTING"] = False
client = app.test_client()

@app.route("/uncaught")
@app.route("/html-uncaught")
def test_uncaught():
raise Exception("Oops, something really bad happened.")

Api(app)

response = client.get("/uncaught")
response = client.get("/html-uncaught")
assert response.status_code == 500
assert response.content_type == "text/html; charset=utf-8"
assert "<h1>Internal Server Error</h1>" in response.text

def test_api_error_handler_on_unhandled_error(self, app):
# Unset TESTING to let Flask return 500 on unhandled exception
app.config["TESTING"] = False
client = app.test_client()

@app.route("/api-uncaught")
def test_uncaught():
raise ApiException("Oops, something really bad happened.")

Api(app)

response = client.get("/api-uncaught")
assert response.content_type == "application/json"
assert response.json["code"] == 500
assert response.json["status"] == InternalServerError().name

Expand All @@ -45,19 +79,19 @@ def test_error_handler_payload(self, app):

@app.route("/message")
def test_message():
abort(404, message="Resource not found")
api_abort(404, message="Resource not found")

@app.route("/messages")
def test_messages():
abort(422, messages=messages, message="Validation issue")
api_abort(422, messages=messages, message="Validation issue")

@app.route("/errors")
def test_errors():
abort(422, errors=errors, messages=messages, message="Wrong!")
api_abort(422, errors=errors, messages=messages, message="Wrong!")

@app.route("/headers")
def test_headers():
abort(
api_abort(
401,
message="Access denied",
headers={"WWW-Authenticate": 'Basic realm="My Server"'},
Expand Down
Loading