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

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

Run script
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

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 21 (16 by maintainers)

Most upvoted comments

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

taion on Nov 1, 2018

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

taion on Nov 1, 2018

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.

ThiefMaster on May 14, 2018