Opened 15 years ago
Closed 10 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: | dev |
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)
Change History (10)
comment:1 by , 15 years ago
Cc: | added |
---|---|
Keywords: | exception handling added |
milestone: | → 1.3 |
Needs documentation: | set |
Needs tests: | set |
Owner: | changed from | to
Patch needs improvement: | set |
Status: | new → assigned |
Triage Stage: | Unreviewed → Design decision needed |
Version: | 1.0 → SVN |
by , 15 years ago
Attachment: | django-1.0.2-nice-ImportError.patch added |
---|
Updated version of patch, catches SyntaxError as well.
comment:2 by , 15 years ago
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 by , 15 years ago
Cc: | added |
---|
comment:4 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:13 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:14 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.
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.