Fixed and intuitivized exception handling #1291

flying-sheep · 2014-12-23T18:18:53Z

abandoning pull request #1281 for this.

the rationale for this is to

fix some logic flaws
make errorhandlers more intuitive by making them rely on exception hierarchy instead of registration order
Enable 500 handlers on blueprint level

and all this while without breaking compatibility in not-already-broken usecases (which rely on the logic flaws i fixed)

TODO:

bake this into some tests as well
one error says

It is currently not possible to register a 500 internal server error on a per-blueprint level.

why is this?

flask/app.py

@@ -62,6 +62,79 @@ def wrapper_func(self, *args, **kwargs):
    return update_wrapper(wrapper_func, f)


+def get_http_code(error_class_or_instance):


flask/app.py

+
+        if is_code:
+            # TODO: why is this?
+            assert code != 500 or key is None, \


flask/app.py

+        :type code_or_exception: int|T<=Exception
+        :type f: callable
+        """
+        if isinstance(code_or_exception, HTTPException):  # old broken behavior


untitaker · 2015-01-01T14:29:49Z

@flying-sheep This looks really good now BTW.

flask/app.py

+
+        exc_class, code = self._ensure_exc_class(code_or_exception)
+
+        handlers = self.error_handler_spec.setdefault(key, {}).setdefault(code, {})


flying-sheep · 2015-01-03T00:17:03Z

@flying-sheep This looks really good now BTW.

thanks! :D

untitaker · 2015-01-03T00:19:15Z

@flying-sheep Don't invest too much time in this yet, @mitsuhiko must decide this ultimatively.

flying-sheep · 2015-01-03T00:33:40Z

well, this is already discussed and implemented, everything that comes now is mere minutes of work, so i might as well.

otherwise i trust mitsuhiko’s judgment except in the matter of Python 2 vs Python 3 ;)

flying-sheep · 2015-01-03T00:39:44Z

and regarding judgment: do you think the cleanness and neatness of grouping exception handlers using their http codes (or lack thereof) is worth the fact that it’s unnecessary for the logic? or not?

untitaker · 2015-01-03T00:43:39Z

I don't think it's worth it, but @mitsuhiko may come up with a reason why it is absolutely necessary (I've encountered such surprises before).

flying-sheep · 2015-01-03T14:19:32Z

can we now start the magic dance that summons him?

untitaker · 2015-01-03T14:31:05Z

I still don't know how that dance works. But I usually spam him on IRC regularly.

untitaker · 2015-01-25T20:41:56Z

@flying-sheep Would it be possible to fix #503 in the run?

flask/app.py

+        def find_superclass(d):
+            if not d:
+                return None
+            for superclass in exc_class.mro():


untitaker · 2015-04-06T11:28:31Z

Requesting help on reviewing this PR. I think it's a great change that definetly should land in 1.0.

flying-sheep · 2015-04-11T12:23:18Z

btw. the regression test fails on master currently so i din’t break anything, i just rebased :)

untitaker · 2015-04-11T12:26:35Z

btw. the regression test fails on master currently so i din’t break anything, i just rebased :)

It doesn't fail for me (I reverted the offending commit a while ago)

flying-sheep · 2015-04-11T12:29:12Z

oh, strange. it doesn’t fail on travis. wat.

locally, test_memory_consumption fails.

so what did mitsuhiko say on IRC? you asking for reviews implies that he likes this?

untitaker · 2015-04-11T12:32:23Z

Hmm, how does #1407 behave for you then?

flying-sheep · 2015-04-11T12:35:44Z

so checkout unitaker/flask#no-threads and try? will do

/e: still test_regression.assert_no_leak fails

flask/app.py

+
+        assert issubclass(exc_class, Exception)
+
+        if issubclass(exc_class, HTTPException):


untitaker · 2015-04-11T13:07:24Z

OK, so behavior-wise the current PR is excellent as-is.

The only thing missing now are docs. From a quick glance at errorhandling.rst there doesn't seem to be anything contradictory in there, but we should definetly clarify the current behavior.

We also need something in upgrading.rst, and a changelog entry.

flying-sheep · 2015-04-11T13:47:22Z

writing something up right now, but errorhandling.rst doesn’t seem to contain anything about error handlers, only logging and debugging AFAICS

untitaker · 2015-04-11T13:48:43Z

We need to document how Flask prioritizes errorhandlers, doesn't matter where. errorhandling seemed like the most related page to me.

Fixed and intuitivized exception handling

untitaker · 2015-04-11T16:15:14Z

Thanks @flying-sheep!

flying-sheep · 2015-04-11T16:16:57Z

been a pleasure 😄

flying-sheep mentioned this pull request Dec 23, 2014

errorhandler should expect exception classes instead of exception instances #1281

Closed

This comment has been minimized.

Sign in to view

untitaker added this to the 1.0 milestone Dec 23, 2014

untitaker reviewed Dec 24, 2014
View reviewed changes

This comment has been minimized.

Sign in to view

ryanwang520 mentioned this pull request Dec 27, 2014

register user errorhandler in the right order #1295

Closed

This comment has been minimized.

Sign in to view

untitaker reviewed Dec 27, 2014
View reviewed changes

flask/app.py

if is_code:

# TODO: why is this?

assert code != 500 or key is None, \

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

flying-sheep force-pushed the errorhandler-rework branch 3 times, most recently from 16f0286 to 2977b77 Compare December 28, 2014 15:22

untitaker reviewed Jan 1, 2015
View reviewed changes

untitaker reviewed Jan 2, 2015
View reviewed changes

untitaker mentioned this pull request Jan 25, 2015

Allow blueprint-local routing for errorhandlers #503

Closed

untitaker reviewed Jan 25, 2015
View reviewed changes

flask/app.py

def find_superclass(d):

if not d:

return None

for superclass in exc_class.mro():

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

untitaker mentioned this pull request Mar 29, 2015

Generic error handler for any exception #1406

Closed

untitaker added the needs review label Apr 6, 2015

flying-sheep added 5 commits April 11, 2015 14:05

Removed unused http code get function

a6c6cc1

More verbose message for old broken behavior

aa4700c

Removed 500 not being registerable for blueprints

0e44cca

removed ExceptionHandlerDict

fd8e6b2

added caching

8c054f0

flying-sheep force-pushed the errorhandler-rework branch from a174d23 to eb2ca6d Compare April 11, 2015 12:21

addressed a few review concerns

b31252d

flying-sheep force-pushed the errorhandler-rework branch from eb2ca6d to b31252d Compare April 11, 2015 12:24

untitaker reviewed Apr 11, 2015
View reviewed changes

flask/app.py

assert issubclass(exc_class, Exception)

if issubclass(exc_class, HTTPException):

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

flying-sheep mentioned this pull request Apr 11, 2015

Added docs for the error handler rework #1429

Merged

untitaker added a commit that referenced this pull request Apr 11, 2015

Merge pull request #1291 from flying-sheep/errorhandler-rework

aed464b

Fixed and intuitivized exception handling

untitaker merged commit aed464b into pallets:master Apr 11, 2015

flying-sheep deleted the errorhandler-rework branch April 11, 2015 16:14

untitaker mentioned this pull request Apr 11, 2015

Deprecate and remove APIs #1182

Closed

6 tasks

This was referenced Apr 11, 2015

Some cleanup for errorhandling refactor #1431

Merged

Should we cache errorhandlers or not? #1433

Closed

untitaker mentioned this pull request Jun 4, 2016

Error Handlers versus Werkzeug Exceptions #175

Closed

davidism removed the needs review label Apr 19, 2017

davidism mentioned this pull request Jun 19, 2019

always pass InternalServerError instance to 500 handler #3266

Merged

github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020

		@@ -62,6 +62,79 @@ def wrapper_func(self, args, *kwargs):
		return update_wrapper(wrapper_func, f)


		def get_http_code(error_class_or_instance):


		exc_class, code = self._ensure_exc_class(code_or_exception)

		handlers = self.error_handler_spec.setdefault(key, {}).setdefault(code, {})


		assert issubclass(exc_class, Exception)

		if issubclass(exc_class, HTTPException):

Fixed and intuitivized exception handling #1291

Fixed and intuitivized exception handling #1291

Conversation

flying-sheep commented Dec 23, 2014

This comment has been minimized.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment has been minimized.

This comment has been minimized.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Jan 1, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

flying-sheep commented Jan 3, 2015

untitaker commented Jan 3, 2015

flying-sheep commented Jan 3, 2015

flying-sheep commented Jan 3, 2015

untitaker commented Jan 3, 2015

flying-sheep commented Jan 3, 2015

untitaker commented Jan 3, 2015

untitaker commented Jan 25, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Apr 6, 2015

flying-sheep commented Apr 11, 2015

untitaker commented Apr 11, 2015

flying-sheep commented Apr 11, 2015

untitaker commented Apr 11, 2015

flying-sheep commented Apr 11, 2015

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Apr 11, 2015

flying-sheep commented Apr 11, 2015

untitaker commented Apr 11, 2015

untitaker commented Apr 11, 2015

flying-sheep commented Apr 11, 2015