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

always pass InternalServerError instance to 500 handler #3266

Merged
merged 2 commits into from Jun 21, 2019

Conversation

@davidism
Copy link
Member

commented Jun 19, 2019

Due to multiple PRs over the last 5 years, the error handler behavior has slowly changed with the goal of being more consistent. However, after #2314 was merged in 1.0.0, these changes all cascaded together to make some inconsistent behavior finally visible.

After extensive discussion in #1281, #1291, and #1429, the goal was to make error handlers trigger for exceptions in MRO order, rather than registration order. It formalized the idea that HTTP exception classes and status codes were aliases. Registering a handler for 401 was the same as Unauthorized.

However, it (unintentionally?) preserved some old behavior where user errors would only be looked up against a 500 error handler, not a InternalServerError handler, even though the goal was for these to be aliases.

#2314 ensured a more consistent lookup order between blueprints and app error handlers for codes and exception classes. #2362 simplified the code even more, and made it more correct for subclass handling. A side effect of these refactors was that it fixed the preserved behavior, so 500 and InternalServerError handlers were equivalent.

All these changes had the goal of making error handler registration and triggering more intuitive, and making maintenance easier.

When an unhandled exception is raised, handle_exception is triggered so that a final, generic internal server error is returned. Previously, the behavior was to pass the unhandled exception to the 500 error handler, rather than the generic InternalServerError. Now that 500 and InternalServerError were the same thing and were both considered as handlers for generic error, users who registered a handler for InternalServerError or the HTTPException base class were surprised to get other random exceptions passed to the handler, rather than strict subclasses (#2778, #2841).

A fix was proposed in #2983 which continued to preserve the old behavior by making a handler for 500 receive any error, while a handler for InternalServerError only received InternalServerError. I think this made the code harder to reason about, both for maintainers and for app devs.

Instead, I'm going the opposite direction and ensuring that those handlers only ever receive InternalServerError instances. For unhandled errors, the exception has a new original_exception attribute that has the original unhandled error. This will be formalized in Werkzeug 1.0.0, until then getattr can be used to check if the attribute is set. The upside of this is that it is safe to assume that all codes and classes are aliases, and will only receive matching classes of errors, which seems to have been the intention of previous discussions, and makes the most sense to me.

The downside is that there is no way for this to be 100% backwards compatible for 500 handlers that were written assuming any exception would be passed to them, and I couldn't think of a way to have a useful deprecation warning transition. e will always look like InternalServerError, possibly making existing generic error pages less useful. However, with the availability of e.original_exception, it should be straightforward to get the intended behavior back. Code shouldn't fail in the mean time, only be less specific. I think the benefit of more consistent behavior outweighs the drawback.

closes #2778
closes #2841
closes #2983

While fixing this, I noticed that finalize_request was only called if a 500 error handler was found. If no custom handler was registered, then an unhandled error would skip after_request functions, saving the session, and sending the request_finished signal. This is now fixed, so finalize_request is always called.

To clear up related confusion about very generic error handlers such as HTTPException and Exception, more docs have been added to the errorhandling.rst page. handle_exception has much clearer explanations of what it does too.

@davidism davidism added this to the 1.1.0 milestone Jun 19, 2019

@davidism davidism requested review from jab and ThiefMaster Jun 19, 2019

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Looks good to me!

I think one thing worth clarifying is a case where an app registers both an Exception and HTTPException error handler, e.g. this:

@app.errorhandler(HTTPException)
def handle_http_exception(exc):
    if not (400 <= exc.code <= 599):
        # if it's not an actual error, use it as a response.
        # this is needed e.g. for the 301 redirects that are raised
        # as routing exceptions and thus end up here
        return exc
    elif exc.response:
        # if the exception has a custom response, we always use that
        # one instead of showing the default error page
        return exc
    return custom_http_error(...)


@app.errorhandler(Exception)
def handle_exception(exc, message=None):
    return custom_unhandled_error(...)

In that case I'd expect the second handler not being called for any HTTP error handled by the first one.

@davidism davidism force-pushed the internal-server-error-type branch from 18ced9a to 0982a27 Jun 20, 2019

@davidism davidism removed the request for review from ThiefMaster Jun 20, 2019

@jab

jab approved these changes Jun 20, 2019

Copy link
Member

left a comment

Thank you for the thorough explanation in the description. It seems clear this is the right approach over that taken in #2983. The code changes, tests, and especially the docs are exemplary. 💯

src/flask/app.py Outdated Show resolved Hide resolved
@taion

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

FWIW, I do prefer this implementation to #2983. I suggested #2983 because it was arguably not breaking, but all else being the same I'd rather have this implementation.

That said, how are you planning on handling the release to avoid breaking semver?

@davidism davidism force-pushed the internal-server-error-type branch from 0982a27 to 54519ef Jun 21, 2019

@davidism davidism merged commit 185d7cb into master Jun 21, 2019

9 of 13 checks passed

Flask Build #20190621.1 failed
Details
Flask (Flask Python27Linux) Flask Python27Linux failed
Details
Flask (Flask Python35Linux) Flask Python35Linux failed
Details
Flask (Flask Style) Flask Style failed
Details
Flask (Flask DocsHtml) Flask DocsHtml succeeded
Details
Flask (Flask Pypy3Linux) Flask Pypy3Linux succeeded
Details
Flask (Flask Python27Windows) Flask Python27Windows succeeded
Details
Flask (Flask Python36Linux) Flask Python36Linux succeeded
Details
Flask (Flask Python37Linux) Flask Python37Linux succeeded
Details
Flask (Flask Python37Mac) Flask Python37Mac succeeded
Details
Flask (Flask Python37Windows) Flask Python37Windows succeeded
Details
Flask (Flask VersionRange) Flask VersionRange succeeded
Details
Flask (FlaskOnNightly) FlaskOnNightly succeeded
Details

@davidism davidism deleted the internal-server-error-type branch Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.