Opened 7 years ago

Closed 4 years ago

#10802 closed Bug (fixed)

urlresolvers RegexURLResolver._get_callback should not catch ImportError and AttributeError

Reported by: Ionel Cristian Maries Owned by: nobody
Component: Core (URLs) Version: master
Severity: Normal Keywords: urlresolver, ViewDoesNotExistError, AttributeError, ImportError
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Because we lose valuable traceback information - it should be very clear where is the source of the import error.

Attachments (4)

core-urlresolvers.patch (938 bytes) - added by Ionel Cristian Maries 7 years ago.
django-10802.diff (6.3 KB) - added by Bas Peschier 5 years ago.
django-10802.2.diff (5.1 KB) - added by Bas Peschier 5 years ago.
django-10802.3.diff (6.3 KB) - added by Bas Peschier 5 years ago.

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by Ionel Cristian Maries

Attachment: core-urlresolvers.patch added

comment:1 Changed 7 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 7 years ago by Ionel Cristian Maries

Any updates? 10 months is a long time.

comment:3 Changed 5 years ago by Chris Beaven

Severity: Normal
Type: Bug

Indeed it is a long time. Bring it up in the django developers group and see if you can get things moving.

comment:4 Changed 5 years ago by grahamd

FWIW, I supplied the following code just recently to a mod_wsgi user so they could dump out the original traceback and thus work out where original source of problem is:

import traceback
import sys

def dump_exception(callable):
   def wrapper(*args, **kwargs):
       try:
           return callable(*args, **kwargs)
       except:
           traceback.print_exception(*sys.exc_info())
   return wrapper

import django.core.urlresolvers

urlresolvers.get_callable = dump_exception(urlresolvers.get_callable)

This was added into the WSGI script file, with the result being that traceback was then output to error log.

The Python agent for New Relic RPM I am working on will also instrument get_callable in a similar way so as to capture exception details within a view for which traceback is otherwise lost.

One other area where tracebacks are lost is in importing of the DJANGO_SETTINGS_MODULE. That only happens once on startup and is a PITA to wrap like above as there is no intermediary function that can wrap and so one would have to use byte code level instrumentation.

comment:5 Changed 5 years ago by Bas Peschier

Easy pickings: unset
UI/UX: unset

Ok, so this was brought up recently on django-developers [1]. Attached above is a patch which will raise ViewDoesNotExist, but only if

  1. The owning module exists, but has no attribute for the view.
  2. The owning module does not exist.
  3. The "view" does exists, but is not callable.

In case of an ImportError due to other reasons, it passes it on.

The patch moves ViewDoesNotExist to the get_callable function where it actually can detect it correctly.

[1]: http://groups.google.com/group/django-developers/browse_thread/thread/62bc422bd34e247e

Changed 5 years ago by Bas Peschier

Attachment: django-10802.diff added

Changed 5 years ago by Bas Peschier

Attachment: django-10802.2.diff added

comment:6 Changed 5 years ago by Bas Peschier

Hmmm, trac screwed up there for a minute, I tried to overwrite the patch because of missing added files. The first patch is the correct one.

comment:7 Changed 5 years ago by Russell Keith-Magee

Triage Stage: Design decision neededReady for checkin

First patch from bpeschier looks good to me.

comment:8 Changed 5 years ago by Jannis Leidel

Patch needs improvement: set

Doesn't apply to trunk after the i18n urls landed.

Changed 5 years ago by Bas Peschier

Attachment: django-10802.3.diff added

comment:9 Changed 5 years ago by Bas Peschier

Patch needs improvement: unset

Updated trunk and rediffed.

comment:10 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16420]:

Fixed #10802 -- Handle ImportErrors and AttributeErrors gracefully when raised by the URL resolver system during startup. Many thanks, IonelMaries and Bas Peschier.

comment:11 Changed 4 years ago by rod333@…

Component: Core (Other)Core (URLs)
Has patch: unset
Keywords: urlresolver ViewDoesNotExistError AttributeError ImportError added
Resolution: fixed
Status: closedreopened
Type: BugUncategorized

The traceback would be very useful in certain situations. For example:
http://pastebin.com/zs7Z2y4W

In this case, home2 would get a ViewDoesNotExist error for app.views.home2 - even if it has nothing to do with the form! That makes it very difficult to find the bug.

I don't mean to be annoying when I reopen a ticket like this. I'm genuinely curious - what's the logic behind making hiding the callback here?

comment:12 Changed 4 years ago by anonymous

Triage Stage: Ready for checkinUnreviewed

comment:13 Changed 4 years ago by Tomek Paczkowski

Has patch: set
Resolution: fixed
Status: reopenedclosed
Type: UncategorizedBug

rod333 it's generally bad to reopen ticket after it has been closed by core dev, doubly so when it was closed after commit, triply, if ticket was closed over year ago.
I'm re-closing this ticket. If you think there is a problem with current committed implementation, please open new one.

(Please don't take it personally, I'm not trying to be an ass, I'm just trying to make things more manageable on trac)

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