Code

Opened 5 years ago

Closed 22 months ago

#10802 closed Bug (fixed)

urlresolvers RegexURLResolver._get_callback should not catch ImportError and AttributeError

Reported by: IonelMaries 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 IonelMaries 5 years ago.
django-10802.diff (6.3 KB) - added by bpeschier 3 years ago.
django-10802.2.diff (5.1 KB) - added by bpeschier 3 years ago.
django-10802.3.diff (6.3 KB) - added by bpeschier 3 years ago.

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by IonelMaries

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 4 years ago by IonelMaries

Any updates? 10 months is a long time.

comment:3 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to 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 3 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 3 years ago by bpeschier

  • 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 3 years ago by bpeschier

Changed 3 years ago by bpeschier

comment:6 Changed 3 years ago by bpeschier

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 3 years ago by russellm

  • Triage Stage changed from Design decision needed to Ready for checkin

First patch from bpeschier looks good to me.

comment:8 Changed 3 years ago by jezdez

  • Patch needs improvement set

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

Changed 3 years ago by bpeschier

comment:9 Changed 3 years ago by bpeschier

  • Patch needs improvement unset

Updated trunk and rediffed.

comment:10 Changed 3 years ago by jezdez

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

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 2 years ago by rod333@…

  • Component changed from Core (Other) to Core (URLs)
  • Has patch unset
  • Keywords urlresolver, ViewDoesNotExistError, AttributeError, ImportError added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from Bug to Uncategorized

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 22 months ago by anonymous

  • Triage Stage changed from Ready for checkin to Unreviewed

comment:13 Changed 22 months ago by oinopion

  • Has patch set
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Type changed from Uncategorized to Bug

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)

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.