ImportError catching in urlresolvers.py
Michael Blume
David Cramer
love to see a generic/reusable approach at solving this everywhere.
Russell Keith-Magee
> This is currently a problem all over in the Django codebase, and I'd
> love to see a generic/reusable approach at solving this everywhere.
We already have a generic/reusable approach -- it's just not used
everywhere that it could be used.
Almost all the places that we are catching ImportError, we're using it
as part of an implementation of module discovery or dynamic loading.
In the case of RegexURLPattern, it's the resolution of a string-based
view specification to a view callable. Catching ImportError is the
easiest way to establish that a named module doesn't exist, and
assuming that the code is otherwise sound, it's 100% accurate.
However, that's a big assumption that doesn't play out in reality, and
so "couldn't import module due to error in code" errors get confused
with "module does not exist" errors, leading to the confusion reported
by the original message.
This is a case where we need to do better at eating our own dogfood.
Wherever we are using the ImportError trick to catch 'specified module
does not exist', we should be using a combination of
django.utils.module_loading.module_has_submodule and
django.utils.importlib.import_module. Any patch implementing this
pattern (wherever it occurs) would be looked upon favourably for
trunk.
Yours,
Russ Magee %-)
bur...@gmail.com
and what do you say about showing call stack properly?
The problem is not ViewDoesNotExist itself, but throwing away useful traceback.
If we do instead:
import sys
try:
self._callback = get_callable(self._callback_str)
except ImportError, e:
mod_name, _ = get_mod_func(self._callback_str)
raise ViewDoesNotExist("Could not import %s. Error was: %s" %
(mod_name, str(e))), None, sys.exc_info()[2]
This way we will not only see not only exception where was required
module that was not imported, but also how it was attempted to be
imported -- often there's cyclic imports, few other imports or
initializations causing other imports, which are hidden with simple
raise.
Few exceptions wrappers patched in my fork:
https://github.com/buriy/django/commit/72b7a7350022a4a0a9cb1f01be62b117dedf89e2.
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
--
Best regards, Yuri V. Baburov, Skype: yuri.baburov, MSN: bu...@live.com
Russell Keith-Magee
> Hi Russell,
>
> and what do you say about showing call stack properly?
>
> The problem is not ViewDoesNotExist itself, but throwing away useful traceback.
>
> If we do instead:
> import sys
> try:
> self._callback = get_callable(self._callback_str)
> except ImportError, e:
> mod_name, _ = get_mod_func(self._callback_str)
> raise ViewDoesNotExist("Could not import %s. Error was: %s" %
> (mod_name, str(e))), None, sys.exc_info()[2]
>
> This way we will not only see not only exception where was required
> module that was not imported, but also how it was attempted to be
> imported -- often there's cyclic imports, few other imports or
> initializations causing other imports, which are hidden with simple
> raise.
To my mind, this isn't the right approach -- it's papering over the
problem, not fixing it.
In this case, the underlying problem is that ImportError is actually
catching 2 different errors:
1) The view specified doesn't exist
2) The module containing the view contains an error.
The current implementation catches both errors, and turns them into a
ViewDoesNotExist error. You don't fix this problem by providing better
stack trace information on a ViewDoesNotExist error -- you fix the
problem by actually separating the two errors, making (1) return
ViewDoesNotExist, and (2) return ImportError. That way, you'll get a
meaningful error message *and* reliable stack trace data.
Yours,
Russ Magee %-)
Bas Peschier
On 15 jun, 07:36, Russell Keith-Magee <russ...@keith-magee.com> wrote:
> In this case, the underlying problem is that ImportError is actually
> catching 2 different errors:
> 1) The view specified doesn't exist
> 2) The module containing the view contains an error.
the code does know the difference between 1) and 2), but the exception
raised does not convey that.
It might be a better idea to move the ViewDoesNotExist into the
get_callable function, which get triggered with an AttributeException
or an ImportError when the owning module can be imported. All other
conditions are not caught.
Bas
Thomas Guettler
> In RegexURLPattern._get_callback, we attempt to fetch the callable named
> by the URL pattern, and catch a possible ImportError if this fails. If
> so, we raise ViewDoesNotExist.
>
> try:
> self._callback = get_callable(self._callback_str)
> except ImportError, e:
> mod_name, _ = get_mod_func(self._callback_str)
> raise ViewDoesNotExist("Could not import %s. Error was: %s"
> % (mod_name, str(e)))
I uncomment the try...except part in my local django version (and the AttributeError). I want to see the original
exception. It would be nice to have a better solution in django. Maybe a settings variable CATCH_EXCEPTIONS?
Thomas
--
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de
Bas Peschier
which takes Russell's approach.
Russell Keith-Magee
> For reference, I added a patch to https://code.djangoproject.com/ticket/10802
> which takes Russell's approach.
Looks good to me! I've just marked to RFC. Thanks for the patch Bas!
Yours,
Russ Magee %-)