Flask: Registered error handler (using register_error_handler) processes inappropriate exceptions

[woxcab]

If register an error handler for exception class A then this error handler will be called for any exception B that isn't A or subclass of A.

Expected Behavior

Given error handler must be called only for exceptions that's instance of A or instance of subclass of A.

Actual Behavior

Error handler processes any exception.

How to reproduce

Minimal code:

import flask
from werkzeug.exceptions import HTTPException

app = flask.Flask(__name__)
app.register_error_handler(HTTPException, lambda e: (str(e), e.code))

@app.route('/')
def index():
    raise ValueError()

app.run()

1. Run script
2. Go to http://127.0.0.1:5000/

Server logs exception AttributeError: 'ValueError' object has no attribute 'code', i.e. registered error handler is called for ValueError exception, but the error handler is registered only for HTTPException and its subclasses.

Environment

• Python version: 3.5.2
• Flask version: 1.0.2
• Werkzeug version: 0.14.1

[sgunturu5]

Discussed with @davidism
HTTPException Handler and InternalServerError is the default exception handle for unhandled exceptions. Hence it need to be generic. Planning to update the documentation.

[ThiefMaster]

Is this really the behavior we want? I think an error handle for Exception should be catchall - but in any HTTPException error handler getting an exception that is not a subclass of HTTPException seems wrong/confusing.

[davidism]

Flask wants to raise a 500 error when there is an unhandled exception. But it tries to use the InternalServerError (500) error handler to handle that 500 error (which falls back to the HTTPException). That seems correct, but means that they need to not make assumptions about the incoming error.

[ThiefMaster]

What I mean is that the error handler should receive the 500 error, but with an InternalServerError exception -- not the original exception that caused it. IMO people should register an error handler for Exception, ValueError, etc. if they want these exceptions.

[taion]

@ThiefMaster

I spent a few minutes poking around at your recommended solution, because it was the one that seemed to make sense, and it's straightforward to implement.

However, the test suite gives an example for when the current behavior might be desired: https://github.com/pallets/flask/blob/7e714bd28b6e96d82b2848b48cf8ff48b517b09b/tests/test_user_error_handler.py#L29-L31

In other words, while receiving a KeyError is very strange if I registered a handler for InternalServerError, it's not at all strange if I registered a handler for 500. However, the current handler map implementation represents error handlers registered for 500 identically with those registered for InternalServerError (as error handlers registered for a code are treated as those registered for the default exception type for that code).

So, assuming the behavior above is desired (i.e. the handler for 500 gets the "real" exception, while the handler for InternalServerError gets the InternalServerError), the obvious implementation would be to track error handlers for status codes entirely separately from those for exception classes.

_However_, this would require changing the shape of error_handler_spec, and even though this isn't something users should have any good reasons to use, it's actually part of the documented API: http://flask.pocoo.org/docs/1.0/api/?highlight=error_handler_spec#flask.Flask.error_handler_spec. It's worth noting that this documentation is actually incorrect in the case where the status is None, though.

There's an interim not-really-breaking solution where we treat the exception class as None when registering errors on a status code. Perhaps I'll demonstrate this on a PR.

But otherwise let me know what we think about the possibility of a larger refactor here, and whether error_handler_spec is actually part of the public API.

[davidism]

Why do you think we should be treating exception classes differently than codes? They are the same.

However, the current handler map implementation represents error handlers registered for 500 identically with those registered for InternalServerError (as error handlers registered for a code are treated as those registered for the default exception type for that code).

This is correct.

[taion]

If I install an error handler on HTTPException, I don't expect to receive errors that are not instances of HTTPException. Likewise even with InternalServerError, I would argue.

However, if I install an error handler on 500, then it's less clear. And in fact the test suite currently explicitly asserts that error handlers on 500 receive the "real" exception, rather than an InternalServerError: https://github.com/pallets/flask/blob/7e714bd28b6e96d82b2848b48cf8ff48b517b09b/tests/test_user_error_handler.py#L43.

[davidism]

"500" is just a shortcut for "InternalServerError"

[taion]

Is your view that the linked test case above is wrong, or is it that error handlers on HTTPException need to handle non-HTTPException instances?

[taion]

It's also unintuitive that an error handler attached for e.g. the status code 422 will not catch exceptions that are instances of classes that extend HTTPException but not UnprocessableEntity.

From the docs http://flask.pocoo.org/docs/1.0/patterns/errorpages/#error-handlers:

A handler can be registered for a status code, like 404, or for an exception class.

But it seems like if I wrote a:

class MyUnprocessableEntity(HTTPException):
    code = 422
    description = "Some other description of an unprocessable entity error."

Then an error handler on 422 should catch this, which suggests that (in addition to the case above with handling internal errors) there's reason to not treat status codes as identical to the default exception classes in all cases.

[davidism]

That test still passes if you change @app.errorhandler(500) to @app.errorhandler(InteralServerError).

[davidism]

If you registered that custom exception with Werkzeug's exception map, it would work.

[taion]

That test still passes if you change @app.errorhandler(500) to @app.errorhandler(InteralServerError).

That's exactly the problem here, though. Per @ThiefMaster in https://github.com/pallets/flask/issues/2778#issuecomment-388956734, if I install an exception handler on InternalServerError, I do not except to receive exceptions that are not instances of InternalServerError.

[taion]

I'm not disputing your explanation of the current behavior.

The gist of this issue is that the current behavior is unexpected and seems buggy to me. Per the OP, if I do:

@app.errorhandler(HTTPException)
def my_error_handler(error):
    assert isinstance(error, HTTPException)
    return "I just had an error", error.code

My expectation is that the assert should always trivially pass, and that my error handler will work. This is not currently the case, because error can be whatever internal exception gets thrown in my app.

My PR in #2983 makes the above handler only receive instances of HTTPException, while preserving the ability to register handlers on 500, keeping the existing behavior there.

(And actually, per https://github.com/pallets/flask/issues/2984, it _really_ won't work as expected, because internal routing exceptions that I do not understand as errors will _also_ get handled by my handler – incorrectly, at that.)

[RawPlutonium]

My flask app is rejecting my custom definition of the 500 error catch and therefore not rendering the template. This is the code. From @miguelgrinberg 's tutorial. `from flask import render_template
from app import app, db

@app.errorhandler(500)
def internal_server_error(error):
    return render_template('500.html'), 500


@app.errorhandler(404)
def not_found_error(error):
    return render_template('404.html'), 404

Please help. Stack trace for the 500 error.

[miguelgrinberg]

@RawPlutonium where do you have your 500 handler defined? Are you sure you imported that module?

[RawPlutonium]

@miguelgrinberg do you mean the view or the logic?

[miguelgrinberg]

@RawPlutonium the logic, the code that you just pasted. I'm thinking you aren't importing it, so flask does not see it.

[RawPlutonium]

According to your tutorial it's just a file called errors.py . The 404 error is responding perfectly and referring to the particular class that is handling the 404 not found error. The 500 error however is not as friendly as when I trigger it, the system view is displayed instead of my custom one.

How come it's seeing the 404? @miguelgrinberg , Is there something else I'm supposed to import?

[davidism]

Please use another venue for Q&A. Stack Overflow or our Discord would be appropriate.

[RawPlutonium]

@davidism I'm sorry about that. @miguelgrinberg can we continue there?