Don't pass unexpected exception types to error handlers

[taion]

Fixes #2778

cc @ThiefMaster

This follows my last proposal in #2778 (comment).

[garenchan]

Like you, I think the current implementation is a bit confusing.

But at least we can get the original error in the handler. This is useful for both logging and troubleshooting errors. But your solution doesn't take this into account.

[taion]

Perhaps InternalServerError should be tagged with the original error.

Note, though, that the implementation above will forward the original error when the error handler is installed on 500.

[garenchan]

I'm sorry to have misunderstood your solution. I think a test case for the handler of InternalServerError can help us understand your solution more easily.

[taion]

This assertion is actually already present in

flask/tests/test_user_error_handler.py

Line 43 in 92964b4

assert client.get('/keyerror').data == b'KeyError'

.

[garenchan]

Oh, I see. I only read the codes in your patch. Please forgive me for wasting your precious time!

[taion]

I mean, now that you mention it, though, I do think it would be a better API if InternalServerError had some reference to the actual original exception. That would likely constitute a breaking API change, though, whereas this might just barely be okay.

[garenchan]

Yes, I quite agree with you.

Perhaps InternalServerError should be tagged with the original error.

[davidism]

While I understand the results of this, it's making the code harder to follow for me. Based on previous discussions in PRs that led to where we are now, it seems that it was intended that 500 and InternalServerError were aliases that behave the same way.

Instead, based on your discussions, I went the other direction and made it consistent. 500 and InternalServerError handlers always received InternalServerError, and also add an original_exception attribute. While this could result in some less-specific generic error messages if an application with the old behavior upgrades, I think the behavior becoming easier to reason about is worth it.

Thanks for discussing and working on this! Closing in favor of #3266