Opened 16 years ago

Closed 16 years ago

Last modified 13 years ago

#7524 closed (fixed)

Not concrete exception when urls module imports invalid modules.

Reported by: Petr Marhoun <petr.marhoun@…> Owned by: nobody
Component: Core (Other) Version: dev
Severity: Keywords:
Cc: rokclimb15@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This code is in django.core.urlresolvers:

    def _get_urlconf_module(self):
        try:
            return self._urlconf_module
        except AttributeError:
            try:
                self._urlconf_module = __import__(self.urlconf_name, {}, {}, [''])
            except Exception, e:
                # Either an invalid urlconf_name, such as "foo.bar.", or some
                # kind of problem during the actual import.
                raise ImproperlyConfigured, "Error while importing URLconf %r: %s" % (self.urlconf_name, e)
            return self._urlconf_module
    urlconf_module = property(_get_urlconf_module)

It was sufficient for configuration files (almost) without imports:

from django.conf.urls.defaults import *

urlpatterns = patterns('',
    (r'^articles/2003/$', 'news.views.special_case_2003'),
    (r'^articles/(\d{4})/$', 'news.views.year_archive'),
    (r'^articles/(\d{4})/(\d{2})/$', 'news.views.month_archive'),
    (r'^articles/(\d{4})/(\d{2})/(\d+)/$', 'news.views.article_detail'),
)

But now it is possible to use this file:

from django.conf.urls.defaults import *
from news import views

urlpatterns = patterns('',
    (r'^articles/2003/$', views.special_case_2003),
    (r'^articles/(\d{4})/$', views.year_archive),
    (r'^articles/(\d{4})/(\d{2})/$', views.month_archive),
    (r'^articles/(\d{4})/(\d{2})/(\d+)/$', views.article_detail),
)

This file imports views, views probably import models and there are many possibilities for syntax errors. It is very difficult to tell where the error is.

So I would like to propose to show original exception in the debug mode:

    def _get_urlconf_module(self):
        try:
            return self._urlconf_module
        except AttributeError:
            try:
                self._urlconf_module = __import__(self.urlconf_name, {}, {}, [''])
            except Exception, e:
                # Either an invalid urlconf_name, such as "foo.bar.", or some
                # kind of problem during the actual import.
                from django.conf import settings
                if settings.DEBUG:
                    raise
                else:
                    raise ImproperlyConfigured, "Error while importing URLconf %r: %s" % (self.urlconf_name, e)
            return self._urlconf_module
    urlconf_module = property(_get_urlconf_module)

Attachments (2)

00-url-import-exception.diff (813 bytes ) - added by Petr Marhoun <petr.marhoun@…> 16 years ago.
01-url-import-exception.patch (866 bytes ) - added by 16 years ago.
Simply always keep the original exception

Download all attachments as: .zip

Change History (17)

by Petr Marhoun <petr.marhoun@…>, 16 years ago

comment:1 by Bastian Kleineidam <calvin@…>, 16 years ago

How about using the new settings.DEBUG_PROPAGATE_EXCEPTIONS instead of settings.DEBUG?

in reply to:  1 comment:2 by Petr Marhoun <petr.marhoun@…>, 16 years ago

Replying to Bastian Kleineidam <calvin@debian.org>:

How about using the new settings.DEBUG_PROPAGATE_EXCEPTIONS instead of settings.DEBUG?

No, DEBUG_PROPAGATE_EXCEPTIONS is used in a handler (django.core.handlers.base.BaseHandler) not to show nice debugging page. But I want to show this page - with the right exception.

comment:3 by Eric Holscher, 16 years ago

milestone: post-1.0
Triage Stage: UnreviewedDesign decision needed

by , 16 years ago

Simply always keep the original exception

comment:4 by , 16 years ago

milestone: post-1.01.0

The last patch was working great but I think it's not necessary to change the exceptions to ImproperlyConfigured exceptions even when not debugging. This simply doesn't prevent anything. It's still an exception so there is no added danger. On a live site exceptions will not show anyway so no need to test for DEBUG simply import an be done with it ;)

I'm tagging this for 1.0 since it's a really trivial fix that will prevent a lot frustration.

comment:5 by , 16 years ago

Yeah, preview would have been great ;)

import should have been __import__

comment:6 by anonymous, 16 years ago

Cc: rokclimb15@… added

comment:7 by mrts, 16 years ago

+1.

comment:8 by James Bennett, 16 years ago

milestone: 1.0post-1.0

Since this is yet another case of a more general and highly debatable problem, I'm punting back to post-1.0. Please leave it there.

comment:9 by , 16 years ago

#7979 is a duplicate of this issue.

comment:10 by John-Scott Atlakson, 16 years ago

FYI, there is an older ticket [6537] with a patch (the 08/12/08 patch above is identical to the patch on ticket [6537]).

I am not suggesting this patch is the most philosophically pure solution but it certainly allows for more productive debugging for those who can't wait.

While one more ticket might not turn the raindrops into a 'big flood', there is perhaps more interest in this particular case of exception swallowing being resolved than was previously apparent.

comment:11 by , 16 years ago

Closed #6537 as a duplicate. As john_scott writes it has the exact same patch I submitted and similar arguments.

comment:12 by Karen Tracey <kmtracey@…>, 16 years ago

milestone: post-1.01.0

Moving back to 1.0 per this note from Jacob on django-dev: http://groups.google.com/group/django-developers/msg/08965e9852949ec8?hl=en

(Though he did not specifically OK this getting moved back, the last paragraph sounds like he agrees something needs to be done for 1.0 to fix this particular case. If that's being covered by some other ticket please point me to it.)

Also, #8596 looks like another user tripping over this behavior.

comment:13 by Jacob, 16 years ago

Triage Stage: Design decision neededAccepted

comment:14 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

(In [8664]) Fixed #7524: allow errors raised during import of a urlconf to bubble up.

comment:16 by Jacob, 13 years ago

milestone: 1.0

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top