Even better better error handling
Simon Belak
I have just submitted a new patch [1] to introduce better error handling
as proposed in #219 [2].
As this is my first patch any tips or criticisms would be most appreciated.
Which brings me to my main point. Do you even like this approach? Any
wishes for enchantments? Thanks.
[1] http://trac.turbogears.org/turbogears/ticket/258
[2] http://trac.turbogears.org/turbogears/ticket/219
Cheers!
Simon
Kevin Dangoor
> As this is my first patch any tips or criticisms would be most appreciated.
> Which brings me to my main point. Do you even like this approach? Any
> wishes for enchantments? Thanks.
>
> [1] http://trac.turbogears.org/turbogears/ticket/258
Interesting idea. I'll have to ruminate on this a bit (taking JP's
comments into account).
I have a feeling that the problem with this approach is that people
will most likely just turn the erroneous request around and back to
another method (which may not be as pleasant when using a generic
function this way, since all you have coming in is errors).
Kevin
jpel...@gmail.com
This is really interesting, and the rule-based error handler dispatch
is very cool. But I think the formulation in ticket #219 is a lot more
clear. More precisely I think that myself of 6 months from now, looking
at something I write now, will have a lot less trouble tracing the
execution within a controller if the form-handling method is decorated
with the name of the error handler, instead of vice versa. Combined
with splitting up turbogears.expose (cf tickets #159 and #113), it
actually reads pretty well:
def form(self):
# return cached or newly-minted form
@expose(html='etc')
def show_form(self):
# show the form, point action at submit
# not exposed
def submit_error(self, errors=[], exception=None, **input):
...
@expose(html='etc')
@error_handler(submit_error)
@input(form=form) # or @unpack as in #113
def submit(self, **data):
...
I'm leery of the whole decorator-forward approach (as must be clear
from my earlier posts), but structured like the above, and assuming the
error_handler decorator catches exceptions too, I can see how it would
reduce the boilerplate code in controllers without becoming
incomprehensible to my future self, who always winds up being much
stupider than I think he will be. :)
JP
Simon Belak
If we go with your approach I would still suggest using generic
functions (yes, I like them very much ;)) to differentiate between
errors. Otherwise I can see some error handlers consisting mostly of
code checking what went wrong instead of doing something about it. I
doubt this would help comprehensive.
If I may borrow your example:
@expose(html='etc')
# Something evil happend to our DB
@error_handler(sql_error, "isinstance(errors, SQLObjectIntegrityError)")
# Default
@error_handler(submit_error)
@input(form=form) # or @unpack as in #113
def submit(self, **data):
...
Simon
jpel...@gmail.com
> # Something evil happend to our DB
> @error_handler(sql_error, "isinstance(errors, SQLObjectIntegrityError)")
> # Default
> @error_handler(submit_error)
The power to dispatch your errors is there if you want it -- but if it
makes your head spin, you don't have to use it. That's really good.
JP
Kevin Dangoor
This does seem nice. It makes it easy to split up standard validation
problems from actual errors, if you need to.
Kevin
Simon Belak
in order not to over-bloat the trac I think it is best if I submit my
patch/proposal (see attachment) here first.
Presented is an implementation of JP's idea with some enchantments. It
allows defining error handlers as follows:
@expose(html='etc')
# Something evil happend to our DB
@error_handler(sql_error, "isinstance(errors, SQLObjectIntegrityError)")
# Default
@error_handler(submit_error)
@input(form=form)
def submit(self, **data):
...
Each exposed method can have multiple error handlers if generic
functions are harnessed.
Execution of decorated method is encapsulated in a try-catch block
passing any resulting exceptions to an appropriate error handler.
Compatibility with existing way of handling errors is retained.
There are three things I would like to add or change eventually but I am
unsure what would be the best/cleanest way of doing it:
1) Introduce a global default error handler. Perhaps in form of a
decorator. For example:
@default_error_handler
def my_default_error_handler(self, errors, *args, **kw):
...
2) Integrate Ian Bicking's excellent EvalException [1] for use during
debugging/development.
3) Currently all exceptions raised by the decorated method are caught.
Such behaviour is at times a bit counter-productive as it can obscure
trivial errors such as syntax errors and the like.
We could catch just base exceptions of all modules but since TG is
composed of many different projects this will most likely end up quite
messy. Even worse, any such filtering reduces flexibility and clarity.ž
I suppose 2) would help in this matter as well.
Cheers,
Simon
[1] http://blog.ianbicking.org/ajaxy-exception-catching.html
Simon Belak
Thanks for the tip,
Simon
P.S. this is a re-send (from a different mail account) since my
original reply has not been delivered for more than 24h.
Stephen Thorne wrote:
> Excellent ideas!
>
> I'd recently become quite vexed at the error handling, especially when SQL errors occur.
>
> Could you repost that patch as a unified diff? `diff -u`, or a diff created using svn diff?
>
> You've managed to use diff's default behaviour, which is not-optimal for situations where others have changes in the same file.
>
> Thanks :)
Kevin Dangoor
to those couple of tickets about better error handling) is that you
can't easily toss validation errors back to the method that originally
displayed the form. I'll take another look at those tickets sometime
soon, though, and decide which (if any) solution makes it in 0.9. If
nothing jumps out at me, I may defer this until sometime between 0.9
and 1.0.
Kevin
--
Kevin Dangoor
Author of the Zesty News RSS newsreader
email: k...@blazingthings.com
company: http://www.BlazingThings.com
blog: http://www.BlueSkyOnMars.com
Simon Belak
1) "Old-style" (==current approach ;)) where no error handler is
registered for a given method
2) A method can always be an error handler for itself:
@expose(...)
def foo(self, errors=None, bar=""):
...
foo = error_handler(foo)(foo)
There are two caveats (both trivial to fix) with 2) in my current
implementations. As can be seen above one cannot use decorator syntax
for a recursive error handler definition. I propose changing
error_handler to allow calls without arguments to handle this case.
And second, ff an exception is raised inside the method which handles
it's own errors, a nasty "infinite" loop can ensue. Some stack prodding
should do the trick here.
Cheers,
Simon
Simon Belak
> There are two caveats (both trivial to fix) with 2) in my current
> implementations. As can be seen above one cannot use decorator syntax
> for a recursive error handler definition. I propose changing
> error_handler to allow calls without arguments to handle this case.
> And second, ff an exception is raised inside the method which handles
> it's own errors, a nasty "infinite" loop can ensue. Some stack prodding
> should do the trick here.
>
Submitted patch for the former.
"Infinite loop" turned out to be a non-issue (I was lazy and didn't look
at the code. Sorry for disinformation).
One more thing about error-tossing. With method combination
(before/after, around, next_method) one can inform (call) the
originating method about the error and still have another method do the
view.
Simon
Kevin Dangoor
Thanks for the patch! This is looking pretty good and flexible.
I do have an issue:
http://trac.turbogears.org/turbogears/ticket/258
I like the idea, except for one thing: the common idiom for dealing
with form validation errors with turbogears widgets is this:
@expose(html="...")
def index(self):
...
@expose(html="...")
@validators(form=myform)
def save(self, val1, val2):
if cherrypy.request.form_errors:
return index()
Note that index doesn't have an errors parameter. An earlier version
of the TurboGears code would introspect the function to see if it had
an errors parameter, and pass the errors in if it does. Maybe we
should do that?
There's a trick there, of course. We now have 4 decorators:
expose validate error_handler identity.require
In order to introspect the function, we need direct access to it. I
think the TurboGears decorators should all set an attribute on the
function that has the original function object. That way, the
decorators are a bit more flexible in terms of how they are arranged
over the method in question.
Opinions?
Kevin
Simon Belak
> I like the idea, except for one thing: the common idiom for dealing
> with form validation errors with turbogears widgets is this:
>
> @expose(html="...")
> def index(self):
> ...
>
> @expose(html="...")
> @validators(form=myform)
> def save(self, val1, val2):
> if cherrypy.request.form_errors:
> return index()
>
> Note that index doesn't have an errors parameter. An earlier version
> of the TurboGears code would introspect the function to see if it had
> an errors parameter, and pass the errors in if it does. Maybe we
> should do that?
I am not sure I follow you, so forgive me for responding with two random
guesses. ;)
If error_handler() is not used, methods still get called through
_call_with_errors().
Or are you summarising that since 'errors' parameter is the sole
requirement of dispatch_error protocol, every controller method
declaring it should implicitly be declared it's own error handler?
Sure we could do that, if you think continence outweighs induced magic?
Personally I like it, as it can greatly reduce boiler-plate.
>
> There's a trick there, of course. We now have 4 decorators:
>
> expose validate error_handler identity.require
>
> In order to introspect the function, we need direct access to it. I
> think the TurboGears decorators should all set an attribute on the
> function that has the original function object. That way, the
> decorators are a bit more flexible in terms of how they are arranged
> over the method in question.
>
> Opinions?
Ideally *all* decorators would be true invariants. Unfortunately doing
so would mean quite a serious rewrite (all except error_handler of
course ;))). However I belive this is still the best solution in the
long-run.
Simon
Kevin Dangoor
Hey, that's a good idea. Not what I was talking about, but a good idea
nonetheless.
What I was angling for was to have @errorhandler also be used for the
idiom I described above. Specifically, the above could change to:
@expose(html="...")
def index(self):
...
@expose(html="...")
@error_handler('index')
@validators(form=myform)
def save(self, val1, val2):
# assume everything is good and do saving stuff
Note that the index method doesn't have an errors option, so that
should be left out. The widgets code will automatically display errors
and input values as appropriate.
> > In order to introspect the function, we need direct access to it. I
> > think the TurboGears decorators should all set an attribute on the
> > function that has the original function object. That way, the
> > decorators are a bit more flexible in terms of how they are arranged
> > over the method in question.
> >
> > Opinions?
>
> Ideally *all* decorators would be true invariants. Unfortunately doing
> so would mean quite a serious rewrite (all except error_handler of
> course ;))). However I belive this is still the best solution in the
> long-run.
Hmm... You're right. It would be nice for the decorators to be
invariants or as close as possible to invariants. What would be nice
is if the decorators actually maintained the signature of the
functions they're decorating but added additional parameters as
needed. I wonder how easy that would be?
Kevin
Simon Belak
>> Or are you summarising that since 'errors' parameter is the sole
>> requirement of dispatch_error protocol, every controller method
>> declaring it should implicitly be declared it's own error handler?
>> Sure we could do that, if you think continence outweighs induced magic?
>> Personally I like it, as it can greatly reduce boiler-plate.
>
> Hey, that's a good idea. Not what I was talking about, but a good idea
> nonetheless.
>
> What I was angling for was to have @errorhandler also be used for the
> idiom I described above. Specifically, the above could change to:
>
> @expose(html="...")
> def index(self):
> ...
>
> @expose(html="...")
> @error_handler('index')
> @validators(form=myform)
> def save(self, val1, val2):
> # assume everything is good and do saving stuff
>
> Note that the index method doesn't have an errors option, so that
> should be left out. The widgets code will automatically display errors
> and input values as appropriate.
>
I see. What can I say, will do. :)
>>> Opinions?
>> Ideally *all* decorators would be true invariants. Unfortunately doing
>> so would mean quite a serious rewrite (all except error_handler of
>> course ;))). However I belive this is still the best solution in the
>> long-run.
>
> Hmm... You're right. It would be nice for the decorators to be
> invariants or as close as possible to invariants. What would be nice
> is if the decorators actually maintained the signature of the
> functions they're decorating but added additional parameters as
> needed. I wonder how easy that would be?
Is adding parameters enough?
The way I would approach this problem is to decouple our controller form
CherryPy altogether and have decorators dynamically build the actual
implementation.
This would also allow changing "HTTP layer" if anybody wishes to do so.
Simon