Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 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: master
Severity: Keywords:
Cc: rokclimb15@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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@…> 6 years ago.
01-url-import-exception.patch (866 bytes) - added by jarrow 6 years ago.
Simply always keep the original exception

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

comment:1 follow-up: Changed 6 years ago by Bastian Kleineidam <calvin@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 in reply to: ↑ 1 Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

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 Changed 6 years ago by ericholscher

  • milestone set to post-1.0
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 6 years ago by jarrow

Simply always keep the original exception

comment:4 Changed 6 years ago by jarrow

  • milestone changed from post-1.0 to 1.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 Changed 6 years ago by jarrow

Yeah, preview would have been great ;)

import should have been __import__

comment:6 Changed 6 years ago by anonymous

  • Cc rokclimb15@… added

comment:7 Changed 6 years ago by mrts

+1.

comment:8 Changed 6 years ago by ubernostrum

  • milestone changed from 1.0 to post-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 Changed 6 years ago by jarrow

#7979 is a duplicate of this issue.

comment:10 Changed 6 years ago by john_scott

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 Changed 6 years ago by jarrow

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

comment:12 Changed 6 years ago by Karen Tracey <kmtracey@…>

  • milestone changed from post-1.0 to 1.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 Changed 6 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:14 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:16 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.