Opened 6 years ago

Closed 2 years ago

#13480 closed Cleanup/optimization (wontfix)

(patch) Gracefully handling ImportErrors in user modules.

Reported by: Charlie Nolan Owned by: Michael 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


(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 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 Charlie Nolan 6 years ago.
Updated version of patch, catches SyntaxError as well.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Michael Axiak

Cc: mike@… added
Keywords: exception handling added
milestone: 1.3
Needs documentation: set
Needs tests: set
Owner: changed from nobody to Michael Axiak
Patch needs improvement: set
Status: newassigned
Triage Stage: UnreviewedDesign decision needed
Version: 1.0SVN

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 6 years ago by Charlie Nolan

Updated version of patch, catches SyntaxError as well.

comment:2 Changed 6 years ago by Charlie Nolan

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 6 years ago by Charlie Nolan

Cc: funnyman3595@… added

comment:4 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Cleanup/optimization

comment:5 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 4 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:14 Changed 2 years ago by Tim Graham

Resolution: wontfix
Status: assignedclosed

ImportError in a module seems to cause runserver to crash and display the traceback there. I think that's fine and not worth adding additional complexity to display the message in a browser.

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