register user errorhandler in the right order

[ryanwang520]

solved the problem proposed by #1291 in a lighter way,
thus determining the exception hierarchy at app setup time instead of running time.

[untitaker]

I honestly like this a lot more than #1291, although I am wondering if we could sort exception handlers similarly to routes: with a key function.

[untitaker]

This version seems to work as well:

def _append_error_handler(self, key, new_exc, f):
    registered_errors = self.error_handler_spec \
        .setdefault(key, {}) \
        .setdefault(None, [])
    new_entry = (new_exc, f)

    for i, (exc, _) in enumerate(registered_errors):
        if new_exc is exc:
            registered_errors[i] = new_entry
            break
        elif issubclass(new_exc, exc):
            registered_errors.insert(i, new_entry)
            break
    else:
        registered_errors.append(new_entry)

[untitaker]

cc @flying-sheep

[flying-sheep]

good idea! you don’t adress the profoundly broken logic in this line though.

and i think you both misunderstand the performance characteristics of my approach.

the “heavy” feel mainly comes from the fact that my dict subclass tries to transparently alias error codes and default error classes. else it will likely be faster than this.

why?

because you here have one central error registry realized as list. mine is one registry per error code, in the form of a dict.

this here works by traversing the whole registry for a blueprint until a matching error is found. also things like dict(registered_errors) are expensive. it’s maybe O(n × len(registered_errors))

mine works by traversing the mro and looking for a match in a dict for each class in the mro: O(len(mro)-2), which is mostly less than 3 or so. also note that the code in handle_http_exception and handle_user_exception gets prettier and simpler with my PR.

[untitaker]

you don’t adress the profoundly broken logic in this line though.

Yeah, about that... I think it's at least a small step in the right direction if we just remove that if-statement. I played around with this locally, it's actually pretty hard to unify HTTP exceptions and errorcodes.

performance characteristics

Maybe I should've clarified that I mostly like the idea of sorting a single list of handlers more than having a custom dict-like object. It's mostly easier to read (and audit) for me.

[flying-sheep]

it's actually pretty hard to unify HTTP exceptions and errorcodes.

doesn’t my PR completely and transparently address this issue? if not, why didn’t you tell me yet ;)

Maybe I should've clarified that I mostly like the idea of sorting a single list of handlers more than having a custom dict-like object. It's mostly easier to read (and audit) for me.

as said: it could also be a normal dict; the only difference is that valid HTTP error codes are converted to their canonical HTTPException subclass on item access. if you don’t like something about that part, again: why didn’t you simply state your concerns in my PR? what are those concerns? The real backing dict is just one attribute access away: ExceptionHandlerDict.data. The behavior is simple: ehd[http_error_code] == ehd[canonical_http_exception_class]. But if you don’t like it, just say it and we can talk about doing the conversion in _register_error_handler instead.

---

i’m going out on a limb and say you didn’t try it. it’s actually very simple and as clear as it is ordered:

error_handler_spec = {
    blueprint1: { ... },
    None: {
        None: {  # all exceptions not a subclass of HTTPException
            NonHTTPException: some_handler_function, 
            ...
        },
        403: {  # all exceptions that are a subclass of Forbidden (403)
            Forbidden: some_handler_function, 
            ForbiddenSubclass: some_handler_function, 
        },
        ...
    }
}

[untitaker]

@flying-sheep Maybe I've sounded too decisive, I've never intended to say that a particular PR is going to be merged.

[ryanwang520]

@untitaker Thanks! Your version looks good to me.

@flying-sheep

• I think the separation of UserException and HTTPException in Flask is intended. Maybe The HTTPException subclasses should be marked as final as they are just according to the HTTP Specification, each corresponding to a certain HTTP code .You should not subclass a certain HTTPException cause you should not make two Exception type share the same HTTP code. Why are you trying to subclass a Forbidden instead of just raising the Forbidden exception in your own Exception handler? I'd like to hear about your opinion on this ;).
• As for the performance, as you won't register numerous error handlers, to transverse a small
  list seems not a big issue, but make code much more simpler to analyze, don't you think so?
  I also wonder if we can find a better approach @untitaker

[flying-sheep]

I think the separation of UserException and HTTPException in Flask is intended

you might notice that my PR instates this separation whereas yours (and the existing code, due to a bug) doesn’t.

please read the discussion in #1281, where i discovered the way the currently active code is broken.

You should not subclass a certain HTTPException cause you should not make two Exception type share the same HTTP code. Why are you trying to subclass a Forbidden instead of just raising the Forbidden exception in your own Exception handler? I'd like to hear about your opinion on this ;).

because things might be forbidden for different reasons and it might be useful to react differently?

e.g. the example in #1281:

app = Flask(__name__)

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

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

As for the performance, as you won't register numerous error handlers,

well, performance is a secondary concern. i only responded to @untitaker’s implication that your PR is in some way “lighter” than mine, which i disagree with.

i also think that it’s clearer to group handlers by HTTP code than just stuffing them all into a huge list.

to transverse a small list seems not a big issue, but make code much more simpler to analyze, don't you think so? I also wonder if we can find a better approach @untitaker

no, i don’t :D

look at the code in my previous comment: it’s just a nested dict handlers[blueprint][code][exception_class]. simpler than handlers[blueprint][code] if code is not None else next((handler for cls, handler in handlers[blueprint][None] if exception_class is cls))

[untitaker]

#1291 has been merged.