Ticket #3349 patch review

[ab]

I'd like some opinions on the existing patch for http://code.djangoproject.com/ticket/3349.
This ticket ("If an ImportError occurs within some loaders a rather
confusing exception is raised") seeks to address the difficulty of
debugging ImportErrors raised in template tag library.

The cause of the problem, generally speaking, is that an exception is
caught and a different one is raised, which loses information about
the source of the original exception. In this particular case, an
ImportError is caught in django.template.get_library, which then
raises InvalidTemplateLibrary. InvalidTemplateLibrary is then caught
by django.template.defaulttags.load, which raises TemplateSyntaxError.
The exception message[1] is kind of confusing, and it's not
immediately obvious whether the problem was in Django *finding* the
template tag library, or a problem in the library itself, either of
which could have caused the original ImportError.

The solution in the existing patch is to raise the outer exception
(InvalidTemplateLibrary, for the case of get_library) with the
*traceback* of the deeper exception.

My particular concerns with the patch are, from most to least
important:
1. Scope -- the patch generalizes the issue and addresses it
throughout Django. Are people ok with that?
2. Design -- The wrap_and_raise function needs to be used not only at
the site of the problem, but also possibly at a deeper level in the
call stack as well. In this particular case, it needs to be used in
both `get_library` and `load` to address the original issue.
3. Design -- Seeing the ImportError's traceback is indeed helpful in
identifying the source of the problem. But, it's also kind of weird to
have the traceback of an ImportError (ending with the line "import
notthere" or whatever) associated with a different exception.
4. Design -- the verbs "wrap" and "raise" in the wrap_and_raise
function refer to different objects (the old and new exceptions,
respectively).

How serious are these issues? Can anyone think of an alternative
solution?

[1] Depending on the case, you'll see one of the following:
'tag_lib' is not a valid tag library: Could not load template library
from django.templatetags.tag_lib, No module named notthere
'tag_lib' is not a valid tag library: Could not load template library
from django.templatetags.tag_lib, No module named tag_lib

Andrew

[Jeremy Dunck]

On Sat, Dec 12, 2009 at 4:18 PM, ab <andre...@gmail.com> wrote:
> I'd like some opinions on the existing patch for http://code.djangoproject.com/ticket/3349.
> This ticket ("If an ImportError occurs within some loaders a rather
> confusing exception is raised") seeks to address the difficulty of
> debugging ImportErrors raised in template tag library.

...

> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
> identifying the source of the problem. But, it's also kind of weird to
> have the traceback of an ImportError (ending with the line "import
> notthere" or whatever) associated with a different exception.

This may be helpful?
http://blog.ianbicking.org/2007/09/12/re-raising-exceptions/

[Andrew Durdin]

I'm the author of the current patch; I'll just add a bit of
background.

On Dec 12, 10:18 pm, ab <andrewb...@gmail.com> wrote:
>
> 1. Scope -- the patch generalizes the issue and addresses it
> throughout Django. Are people ok with that?

Since the problem of raising new exceptions and losing the original
one seemed systemic throughout much of Django, I thought it
appropriate to make the patch address all known instances of this
class of problem, and not just the one instance in the original
complaint.

> 2. Design -- The wrap_and_raise function needs to be used not only at
> the site of the problem, but also possibly at a deeper level in the
> call stack as well. In this particular case, it needs to be used in
> both `get_library` and `load` to address the original issue.

Correct. It needs to be used wherever Django is catching an exception
arising from user-supplied code and then raising a different
exception.

> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
> identifying the source of the problem. But, it's also kind of weird to
> have the traceback of an ImportError (ending with the line "import
> notthere" or whatever) associated with a different exception.

I'll just clarify for anyone that hasn't applied the patch and tried
it: with the patch in place, the traceback will still show down to the
TemplateSyntaxError, but instead of stopping there will also continue
down to where the ImportError originated. Yes, it's a little weird;
Python 3.0 handles this whole situation much better with "raise ...
from ..." which is intended for use in precisely these situations.

Andrew

[Johannes Dollinger]

Am 14.12.2009 um 10:53 schrieb Andrew Durdin:

> I'm the author of the current patch; I'll just add a bit of
> background.
>
> On Dec 12, 10:18 pm, ab <andrewb...@gmail.com> wrote:
>>
>> 1. Scope -- the patch generalizes the issue and addresses it
>> throughout Django. Are people ok with that?
>
> Since the problem of raising new exceptions and losing the original
> one seemed systemic throughout much of Django, I thought it
> appropriate to make the patch address all known instances of this
> class of problem, and not just the one instance in the original
> complaint.

Yes, please.

>> 2. Design -- The wrap_and_raise function needs to be used not only at
>> the site of the problem, but also possibly at a deeper level in the
>> call stack as well. In this particular case, it needs to be used in
>> both `get_library` and `load` to address the original issue.
>
> Correct. It needs to be used wherever Django is catching an exception
> arising from user-supplied code and then raising a different
> exception.
>
>> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
>> identifying the source of the problem. But, it's also kind of weird
>> to
>> have the traceback of an ImportError (ending with the line "import
>> notthere" or whatever) associated with a different exception.
>
> I'll just clarify for anyone that hasn't applied the patch and tried
> it: with the patch in place, the traceback will still show down to the
> TemplateSyntaxError, but instead of stopping there will also continue
> down to where the ImportError originated. Yes, it's a little weird;
> Python 3.0 handles this whole situation much better with "raise ...
> from ..." which is intended for use in precisely these situations.

`wrap_and_raise()` will appear in the traceback, `raise
wrap_exception(SomeException())` would be cleaner.

Better yet, make all exceptions that are used to reraise other
exceptions a subclass of WrappingException (pick a better name) that
either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't
have to add imports everywhere.

Your patch seems to swallow the new exception's traceback now instead
of the causing traceback. That might be good in some situations, but
generally both should be preserved.
__
Johannes

[ab]

> `wrap_and_raise()` will appear in the traceback, `raise  
> wrap_exception(SomeException())` would be cleaner.

I like that

> Better yet, make all exceptions that are used to reraise other  
> exceptions a subclass of WrappingException (pick a better name) that  
> either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't  
> have to add imports everywhere.

I don't like that. An invalid template exception might be "wrapping"
sometimes, but not others.

Another question: how should the tests for this ticket be written?

[Andrew Durdin]

On Dec 14, 11:00 pm, ab <andrewb...@gmail.com> wrote:
> > `wrap_and_raise()` will appear in the traceback, `raise  
> > wrap_exception(SomeException())` would be cleaner.
>
> I like that

But you must use the three-argument `raise` statement to supply your
own traceback in Python 2.x. You could dispense with the function
entirely if you're happy to repeat `import sys; raise NewException(),
None, sys.exc_info()[2]` instead--although then you lose some
information, see below.

> > Your patch seems to swallow the new exception's traceback now instead
> > of the causing traceback. That might be good in some situations, but
> > generally both should be preserved.

No; the only part of the traceback lost is the `raise` line within
`wrap_and_raise`; the remainder of the traceback which would
correspond to all but the `raise` line of the new exception's
traceback is preserved. But if you weren't using the `wrap_and_raise`
function, you would lose the line of the traceback where the new
exception was raised. Put the following code in a python script and
compare the tracebacks when it calls wrapped(), unwrapped(), or
manually_wrapped():


def wrap_and_raise(new_exception):
import sys
exc_class, exc, tb = sys.exc_info()
if issubclass(exc_class, Exception):
raise new_exception, None, tb
else:
raise new_exception

def valueerror():
raise ValueError("This is a ValueError")

def wrapped():
try:
valueerror()
except:
wrap_and_raise(StandardError("This error normally hides the
original error"))

def unwrapped():
try:
valueerror()
except:
raise StandardError("This error hides the original error")

def manually_wrapped():
import sys
try:
valueerror()
except:
raise StandardError("This error normally hides the original
error"), None, sys.exc_info()[2]

# Try each of these
wrapped()
# unwrapped()
# manually_wrapped()

> > Better yet, make all exceptions that are used to reraise other  
> > exceptions a subclass of WrappingException (pick a better name) that  
> > either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't  
> > have to add imports everywhere.
>
> I don't like that. An invalid template exception might be "wrapping"
> sometimes, but not others.

TemplateSyntaxError is an obvious example. Also, if you do this you're
still not preserving the original traceback.

> Another question: how should the tests for this ticket be written?

I'm not sure (which is why I didn't write any tests originally).
Obviously you'd raise an exception, catch it, and wrap_and_raise
another. I suppose you could examine the output of
`traceback.format_exc()` or one of its other functions to see if the
original exception is now mentioned in the traceback.

Andrew.

[Andrew Durdin]

While I think of it: One thing this patch should address is updating
the `contributing` page <http://docs.djangoproject.com/en/dev/
internals/contributing/> to mention calling wrap_and_raise whenever
you are "catching an exception

arising from user-supplied code and then raising a different
exception".

Andrew.

[Johannes Dollinger]

Am 15.12.2009 um 19:57 schrieb Andrew Durdin:

> On Dec 14, 11:00 pm, ab <andrewb...@gmail.com> wrote:
>>> `wrap_and_raise()` will appear in the traceback, `raise
>>> wrap_exception(SomeException())` would be cleaner.
>>
>> I like that
>
> But you must use the three-argument `raise` statement to supply your
> own traceback in Python 2.x. You could dispense with the function
> entirely if you're happy to repeat `import sys; raise NewException(),
> None, sys.exc_info()[2]` instead--although then you lose some
> information, see below.

I pictured the solution to be closer to PEP 3109 / Java: The wrapped
exception as an attribute on the new exception, with the traceback
attached in `exp.__cause__.__traceback__`. That's where my
`WrappingException(.., wrap=True)` came from.
I don't like the mangled tracebacks, but you're right - the reference
cycle-free python 2.x solution probably is to use the tree-argument
raise statement.

>>> Your patch seems to swallow the new exception's traceback now
>>> instead
>>> of the causing traceback. That might be good in some situations, but
>>> generally both should be preserved.
>
> No; the only part of the traceback lost is the `raise` line within
> `wrap_and_raise`; the remainder of the traceback which would
> correspond to all but the `raise` line of the new exception's
> traceback is preserved. But if you weren't using the `wrap_and_raise`
> function, you would lose the line of the traceback where the new
> exception was raised. Put the following code in a python script and
> compare the tracebacks when it calls wrapped(), unwrapped(), or
> manually_wrapped():

My mistake, of course you're right.

__
Johannes