errorhandler should expect exception classes instead of exception instances

[flying-sheep]

the current master says:

def _register_error_handler(self, key, code_or_exception, f):
    if isinstance(code_or_exception, HTTPException):
        ...

this will only work if you register a exception instance, not an exception class, which is definitely not what users expect. instead of:

---

from werkzeug.exceptions import Forbidden
@errorhandler(Forbidden)
def forbidden(e):
    ...

---

from werkzeug.exceptions import Forbidden
raise Forbidden()

---

you’d have to do

---

from werkzeug.exceptions import Forbidden
forbidden_instance = Forbidden()
@errorhandler(forbidden_instance)
def forbidden(e):
    ...

---

from ..exceptions import forbidden_instance
raise forbidden_instance

---

i think you meant issubclass(code_or_exception, HTTPException), but to preserve backwards compatibility, we should change it to accept both

[untitaker]

First off, thanks for your contribution!

• Your reasoning is sound, but your patch currently breaks the tests (see https://travis-ci.org/mitsuhiko/flask/builds/44139679).
• Since this is a bugfix, I think this should be filed against 0.10-maintenance (and then merged against master).
• Please add a test similar to the example you gave.

[untitaker]

Honestly I don't care that much about fixing this in 0.10, so ignore the second bulletpoint.

[flying-sheep]

i wrote this ad-hoc without testing locally, just to get it done before leaving that day.

i’ll fix it probably this evening…

[flying-sheep]

ahah! indeed the user exception path is taken in the tests.

user exceptions work by registering a list of (class, function) tuples, which is searched for every exception that is thrown. after that, if no registered exception handler is found, it is tested if an exception is a subclass of HTTPException, and if so, the specific exception code handlers are used.

this is important, because the wrong check i tried to fix leads to the registering of a user exception, and this way, the existing tests pass. which they shouldn’t, because:

what the defunct code path that currently is never triggered would do is that it’d simply extract the error code from the exception, which would lead to a replacement of that error handler (so register_error_handler(ForbiddenSubclass) is the same as register_error_handler(403))! simplified:

if issubclass(code_or_exception, HTTPException):  # current code tests isinstance instead.
    code = code_or_exception.code
elif isinstance(code_or_exception, integer_types):
    code = code_or_exception
self.error_handler_spec[key][code] = f  # there can be only one!

some examples

setup:

class HTTPExceptionSubClass(HTTPException):
    code = 418
http_exception_instance = HTTPExceptionSubClass()

the three paths (what they do now):

1. register_error_handler(418) → replaces the 418 error handler
2. register_error_handler(http_exception_instance) → replaces the 418 error handler
3. register_error_handler(HTTPExceptionSubClass) → registers a userexception for HTTPExceptionSubClass
4. register_error_handler(TypeError) → registers a userexception for TypeError

the third path (after my naive patch) would then also replace the 418 error handler, which would break the tests, because again…

summary

the tests are broken because they expect HTTPException subclasses to behave like user exceptions, i.e. registering a HTTPException subclass would not replace the HTTPExceptionSubclass.code handler.

the errorhandler registration code is broken so it actually does make HTTPException subclasses behave like user exceptions.

@mitsuhiko i’m unsure how to proceed. was it really intended than any kind of HTTPException subclass handler replaces the blueprint/app wide one? or what?

[flying-sheep]

approaches:

fuck it

remove special tests and code path for HTTPException subclasses. treat all exceptions the same, except that e.g. a instance of ForbiddenSubclass will trigger the 403 handler if no ForbiddenSubclass handler is registered.

cons: apparently this is not behavior intended by the code. also it might get ugly with subclasses of subclasses, because isinstance checks are performed in sequence, not specificallity

“fix” it

fix the tests to actually reflect what the code tries to do, i.e. replacement of the 403 error handler if a ForbiddenSubclass or ForbiddenSubclass2 handler is registered.

cons: surprising!

really fix it

create a subclass registry for every code that is searched (if exception is an instance of HTTPException, search the exception.code handler registry, not the user exception registry)

also filter the registered exception classes by which are superclasses of the thrown one, and call the handler for the most specific one!

replace self.error_handler_spec[*][*].__setitem__ with something that intercepts code like app.error_handler_spec[None][403] = foo and translates it into app.error_handler_spec[None][403][Forbidden] = foo.

cons: most complex

??

maybe someone who had the idea about what should happen can weigh in…

[untitaker]

Fuck-it seems like the most attractive of all of them. I'd be very interested in a complete re-write of exception handling, given @mitsuhiko's consent.

[flying-sheep]

well, wouldn’t that rewrite basically be really-fix-it? or do you have different plans?

i’d envision that the handler flow would be (pseudocode):

if is_http_exception(exception):
    code = 'user'
else:
    code = exception.code

handler = app.error_handler_spec[blueprint][code].find_most_specific_for(exception)
if not handler:
    handler = app.error_handler_spec[None][code].find_most_specific_for(exception)

with the invariant that app.error_handler_spec[None][?].find_most_specific_for will always return a valid handler, whereas app.error_handler_spec[blueprint][?].find_most_specific_for can return None.

fallthroughs would be (naturally) the least specific handler for that error code, i.e. the “http_exceptions.Forbidden” handler for any Forbidden subclass. (=code 403), for which flask will provide a app-wide default. if no default user exception handler is registered, flask would return the debugger in debug mode and invoke the 500 (default or not) handler elsewise.

[untitaker]

IMO a rewrite would simply work the same way Python's exception-handling works: There are no "403-handlers" (or any handlers for status codes), just handlers for exceptions. If there is no handler for ForbiddenSubclass, the handler for Forbidden would be used.

[flying-sheep]

well first off, yeah, codes and classes would be synonymous: @app.errorhandler(403) would do the same as @app.errorhandler(Forbidden).

but moving on, i’m not exactly sure what you mean:

in python, an exception handler is a try/except clause. the except clause that first matches the exception while it’s bubbling up the stack wins.

but we want to register exception handlers via decorators, so the intuition is: error handlers are independently from stack depth and order. at least my intuition is that module-level decorators are order-independent: i don’t expect the order in which i place my functions to have any impact, except when one is an override of the other.

so i guess the least surprising thing to do is:

class ForbiddenSub(Forbidden): pass

@app.errorhandler(ForbiddenSubClass)
def x(e): ...

@app.errorhandler(403)
def y(e): ...

#replaces y because synonymous. we should throw a warning here
@app.errorhandler(Forbidden)
def z(e): ...

@app.errorhandler(500)
def w(e): ...

def test_a():
    raise Forbidden()
    # triggers z, since z replaces y

def test_b():
    raise ForbiddenSubclass()
    # triggers x, because x is more specific than z (the Forbidden handler)

def test_c():
    raise TypeError()
    # user exception triggers the debugger or “w”, depending on debug settings

def test_d():
    raise ImATeapot()
    # triggers the default 418 handler since we defined none

and blueprints work the same, except that there are no blueprint-wide defaults: any class of Exceptions (e.g. Forbidden subclasses or user exceptions) will be routed to the app’s handlers if there are no fitting handlers registered on the blueprint.

[untitaker]

With "like Python" I mostly meant to match only by exception type and not any codes.

[untitaker]

the except clause that first matches the exception while it’s bubbling up the stack wins

That's what I am suggesting. I think the algorithm for find_most_specific_for would be too costly to implement at runtime, but also multiple inheritance would further complicate the behavior. I'm advocating a behavior where the order of registration determines which errorhandler gets called, which is IIRC what is currently implemented. So apart from removing special-casing for HTTP codes, nothing would change.

[flying-sheep]

I disagree. That's completely counter intuitive:

@app.errorhandler(BarException)
def handle_bar(e):
    ...

@app.errorhandler(FooException)
def handle_foo(e):
    ...

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

What Do you mean the BarException handler is called?

[untitaker]

Yes, but it's how python exception handling works, and how Flask's currently does too.

[flying-sheep]

but it's how python exception handling works

no. afaik there is no concept of error handlers, and there the sequence in which except clauses are testing is bound to the stack: the first except clause up from the exception raising wins.

we have declarative code here, with decorators, no call stack. no verticality, no bubbling. nothing of those things is exposed to the decorators. do you get where i’m coming from? :)

and how Flask's currently does too.

maybe, but nobody seems to use it in an advanced way if i’m the first one realizing that @app.errorhandler(ForbiddenSubclass) doesn’t work

---

finally, for subclassing exception the first registered handler wins, as in above example: if X is a subclass of Y, and you register a Y, then X handler, the X handler will never be used even if you throw a X instance.

and for codes, the last registered handler wins: if you register a handler for 403, and then one for forbidden_subclass_instance, the latter will get all 403s.

---

what i’m getting at: the whole errorhandler system is unintuitive, complex, and in places broken.

my simple idea is: the most specific error handler wins. the intuitive specificity sequence is

1. the exception class hierarchy. most specific superclass of the exception’s class matches
2. blueprint hierarchy (currently only 2 levels deep: blueprint and app). blueprint wins
3. default handlers if nothing registered matches

i bet there is a very efficient data structure for finding the most specific superclass of an object out of a set of registered ones.

[untitaker]

no. afaik there is no concept of error handlers

I am only talking about the "algorithm" used for prioritization:

• Python exception handling: The first matching except-clause (in the order specified) is used
• Flask exception handling: The first matching errorhandler (in the order registered) is used

As opposed to finding the most specific error handler (or except-clause) for an exception.

maybe, but nobody seems to use it in an advanced way

That's a really strong assumption, and I think it's impossible to avoid advanced usage in any sufficiently complex Flask app.

Note that I am not really opposed to a new behavior, but I can't really imagine that the benefit of "intuitiveness" would outweigh the new learning curve. Because, while ordering by registration time might seem counterintuitive, the behavior is easily explained, while a more complex ranking algorithm probably requires much more learning effort. For example, even with "specifity" defined, how should ties be resolved? Sorting by registration time passes that responsibility off to the user.

if i’m the first one realizing that @app.errorhandler(ForbiddenSubclass) doesn’t work

It does work for me (yes, with all the bugs), it doesn't with the bugs fixed.

[flying-sheep]

• Python exception handling: The first matching except-clause (in the order specified) is used
• Flask exception handling: The first matching errorhandler (in the order registered) is used

As opposed to finding the most specific error handler (or except-clause) for an exception

OK, gotcha. i still think that the mental model of registering handlers via decorators is horizontal and declarative, while the mental model with handlers is tied to the structure of the code, i.e. the call stack and is therefore vertical and sequential.

also in our case, handlers are endpoints, as in they return responses, whereas except clauses are just there in the code and either let things flow on or reraise the exceptions. both of those things reinforce the disparate mental models associated with the approaches.

apart from reacting to exceptions, those two approaches have nothing in common, look and feel very different. and again: top level function definitions (unlke nested ones inside of a function) have a order-independent feel to them, as have decorators. and every API i ever used made that work, by only breaking that mental model if you override something existing (mostly accidentally)

i think my idea is just adequate API design, whereas the way things work right now is surprising for everything but simple cases (where it works identically to my approach)

[…] For example, even with "specifity" defined, how should ties be resolved? Sorting by registration time passes that responsibility off to the user.

well, it still comes down to registration order when there are overrides and times. i just think that this behavior (in a ”note” box in the docs) is still more intuitive than e.g. following scenario:

app = Flask(__name__)

[...]

app.errorhandler(403)
def default_forbidden():
    return 'You’re not allowed to do that', 403

and somewhere else

@app.errorhandler(CheatException)
def cheat(e):
    return '''
        You thought you’d get through with faking your score
        while it really is {e.real_score}
    '''.format(e=e), CheatException.code

@app.route('/submit_score/<score>')
def submit_score(score):
    if not internal_score == score:
        raise CheatException(internal_score)

Now add templates and more complexity and have fun debugging why you raised a CheatException for which you clearly just defined a handler (it’s right there, for god’s sake!) and still some nondescript 403 page came!

It does work for me (yes, with all the bugs), it doesn't with the bugs fixed.

huh? my pull request is incomplete and naive, made at a point where i thought the rest of the code worked as it seemed like it should, but it turns out that the tests rely on broken functionality, thus our discussion here. what do you actually use? what works? what doesn’t? what fixed bugs are you talking about?

[flying-sheep]

by the way: i found a very simple and efficient implementation for my idea (example only with app, not blueprint).

half-pseudocode prototype:

class DefaultHandlers(object):
    def __getitem__(self, code):
        description = http_codes.get(code)
        if description is not None:
            return lambda e: '<h1>Error {}</h1>\n{}<br>{}'.format(code, description, e)
        else:
            raise KeyError(code)

Flask.handlers = ChainMap({}, DefaultHandlers())
# for blueprint: ChainMap({}, app.handlers, DefaultHandlers())

def is_http_exception(error_class_or_instance):
    return (
        isinstance(error_class_or_instance, HTTPException) or
        isinstance(error_class_or_instance, type) and
        issubclass(error_class_or_instance, HTTPException))


def Flask.get_handlers(self, e):
    code = e.code if is_http_exception(e) else None # None: user exception
    return self.handlers.setdefault(code, {})

def Flask.register_errorhandler(self, error_class, handler_function):
    assert issubclass(error_class, Exception)
    self.get_handlers(error_class)[error_class] = handler_function

def Flask.find_appropriate_handler(e):
    assert isinstance(error_class, Exception)
    handlers = self.get_handlers(e)
    for error_class in type(e).mro():
        handler = handlers.get(error_class)
        if hander is not None:
            return handler(e)

[untitaker]

what works? what doesn’t?

If I register an errorhandler for ForbiddenSubclass before registering less specific error handlers, it works as one would expect:

• Raising ForbiddenSubclass triggers the ForbiddenSubclass handler
• Raising Forbidden triggers one of the less specific ones

In summary: Feel free to open a PR with a draft of your proposed new behavior. Maybe it works, maybe it doesn't... your suggestions seem sound, but I am honestly unsure of the consequences regarding backwards compatibility. At this point I think only coding it up would make this clear.

[flying-sheep]

well, what you say will still work after that of course.

what won’t work is

1. registering error handlers on exception instances (i’m pretty sure nobody does that, since it would require understanding the broken code, which would have led to someone else filing this very bug)
2. relying on the order and registering a more generic exception handler before a more specific one and relying on the former being triggered (which would be broken code anyway: why would you want to register an error handler which will per definition never be triggered, like the latter one in this example)

[untitaker]

Go ahead, I'd have to play around with the implementation to be able to make any statements.

[mattupstate]

FWIW, I've never once thought to register custom HTTPException subclasses. Only plain old Python exceptions that do not have any meaning in the context of an HTTP application. I simply raise MyCustomException() (which is a subclass of HTTPException) at some point during a request and Flask just deals with it. I don't see the big deal here, honestly.

[flying-sheep]

but if you raise YourCustomException(), you’ll only see whatever handler is registered for that code, not any message based on YourCustomException, right?

like in my example: if you raise CheatException (subclass of Forbidden), you only get the handler registered for 403.

[untitaker]

If you put them in the correct order, your goal is possible:

from flask import Flask
from werkzeug.exceptions import Forbidden

class Cheated(Forbidden):
    pass

app = Flask(__name__)

@app.errorhandler(Cheated)
def cheated(e):
    return "cheated!"

@app.errorhandler(403)  # works with either 403 or Forbidden as arg
def custom_forbidden(e):
    return "forbidden"

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

with app.test_client() as c:
    assert c.get('/').data == 'cheated!'

[untitaker]

That is, with the master branch.

[mattupstate]

No, you'll see whatever your custom exception tells Flask to show. For example, here are some custom JSON exception classes from a project of mine:

class JSONException(HTTPException):
    links = None

    def __init__(self, links=None, *args, **kwargs):
        super(JSONException, self).__init__(*args, **kwargs)
        self.links = links or {}

    def get_links(self):
        return {name: {'href': href} for name, href in self.links}

    def get_headers(self, environ=None):
        return [('Content-Type', 'application/vnd.error+json')]

    def get_body(self, environ=None):
        return {'message': self.description, '_links': self.get_links()}

    def get_response(self, environ=None):
        rv = json.dumps(self.get_body(environ), indent=2)
        return current_app.response_class(rv, self.code, self.get_headers(environ))


class ValidationError(JSONException):
    code = 400
    description = _('Validation error')

    def __init__(self, errors=[], *args, **kwargs):
        super(ValidationError, self).__init__(*args, **kwargs)
        self.errors = errors

    def get_body(self, environ=None):
        rv = super(ValidationError, self).get_body(environ)
        if self.errors:
            rv['_embedded'] = {'errors': self.errors}
            rv['total'] = len(self.errors)
        return rv


class SearchQueryError(ValidationError):
    description = _('Invalid search query')


class AuthenticationError(JSONException):

    def get_headers(self, environ=None):
        return super(AuthenticationError, self).get_headers() + [
            ('WWW-Authenticate', 'JWT realm="Login Required"')
        ]


class Unauthenticated(AuthenticationError):
    code = 401
    description = _('Authentication required')


class Unauthorized(AuthenticationError):
    code = 403
    description = _('Unauthorized')


class ResourceNotFound(JSONException):
    code = 404
    description = _('Resource not found')

No where in my app have I registered these exceptions to be handled by my application. I just raise them, and if it happens to be in the context of a request, Flask returns an appropriate response for me.

[flying-sheep]

abandoning for #1291