Code

Opened 4 years ago

Last modified 15 months ago

#13480 assigned Cleanup/optimization

(patch) Gracefully handling ImportErrors in user modules.

Reported by: FunnyMan3595 Owned by: axiak
Component: Core (Other) Version: master
Severity: Normal Keywords: exception handling
Cc: mike@…, funnyman3595@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

(This issue is filed against 1.0 because that's the version I have installed (1.0.2, specifically) and can confirm it with. Code inspection suggests that the problem still exists in SVN, and the attached patch applies to SVN successfully, albeit with some grumbling about the line numbers being wrong.)

Consider the following line of code, taken from a Django application's views.py file:

from django.contrib.models.auth import User

This represents a fairly common type of programmer error, a mistake in naming. In this case, I attempted to import models.auth instead of the correct auth.models. As expected, this generates an error which must be fixed.

If this error happens within a view function, Django will catch it and (if DEBUG is on) print a nice error message.

If, however, this error happens at the module level, Django fails to catch the error, and it becomes a generic 500 error. The exception falls through to the webserver's error logs instead of being instantly displayed to the programmer.

The reason for this distinction is that at the module level, any exception is transformed into an ImportError, making the module completely unavailable and propagating the exception instantly. This causes Django to fail before it has entered the usual error-catching mechanism.

The attached patch fixes this by wrapping the code which triggers the initial import in the same error-catching routines, and also alters the point of import so that the original ImportError may be displayed instead of the ViewDoesNotExist exception which Django raises in its stead.

This patch should be considered a proof-of-concept. It has not been polished beyond basic debugging, and may well be implemented at a suboptimal level of the initial traceback or written in a non-idiomatic fashion. It may also be worth searching for other places where a similar treatment is necessary, but this is beyond my current understanding of Django.

The patch does, however, solve my problem.

Attachments (1)

django-1.0.2-nice-ImportError.patch (2.9 KB) - added by FunnyMan3595 4 years ago.
Updated version of patch, catches SyntaxError as well.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by axiak

  • Cc mike@… added
  • Keywords exception handling added
  • milestone set to 1.3
  • Needs documentation set
  • Needs tests set
  • Owner changed from nobody to axiak
  • Patch needs improvement set
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Design decision needed
  • Version changed from 1.0 to SVN

I think this is pretty nifty, though there are a few questions that I think about. This only works when you catch an ImportError, but what about SyntaxError. Then what about all of the other errors you can get on module load?

And why is regular module error logging such a bad thing?

All in all, it's an interesting feature request. I think that this should be raised as a possible feature addition during the 1.3 feature discussion phase (which might occur in 2 weeks or so on django-developers?).

Marking as design decision needed.

Changed 4 years ago by FunnyMan3595

Updated version of patch, catches SyntaxError as well.

comment:2 Changed 4 years ago by FunnyMan3595

SyntaxError is now caught. I had assumed that it would be transformed into an ImportError like other exceptions, but as was pointed out, this was incorrect. Indeed, SyntaxError slips through the ViewNotFound transformation as well.

As far as other errors on module load go, I don't know that it's safe to handle them carte blanche, without considering the implications of each one individually. ImportError and SyntaxError are, to my knowledge, the two most common ones to hit, and they're both relatively well-defined as "the programmer screwed up; we should tell them".

Standard webserver error logging is not bad per se, but it's clearly sub-optimal. The webserver's error log may be disabled, inaccessible, unknown to the programmer, or simply inconvenient to reach. The case can also be made that it's nontrivial to read, and that unrelated errors from other parts of the webserver may mask the one which the programmer is concerned with. If the programmer is testing, they're guaranteed to have the webpage open, so Django should give them the information they need directly (provided, of course, it's allowed to by settings.DEBUG).

Put another way, if the webserver's error log is sufficient, why does Django have a custom debugging error page at all? Clearly, the decision has already been made that a custom error page is preferable, so it makes sense to extend it to cover (at minimum) the most common types of programmer error. I can't speak for others, but my experience is that after any given change, there's about a 50% chance that my code contains a syntax error, either a missing : on an if, for, or while statement or some mismatched parentheses. Module-level errors are up there too, but SyntaxError is by far the most common exception I see.

comment:3 Changed 4 years ago by FunnyMan3595

  • Cc funnyman3595@… added

comment:4 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:5 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 15 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from axiak to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.