#11696 closed (fixed)
validate command in 1.1 hides errors in models that used to be shown
Reported by: | martin maney | Owned by: | Luc Saffre |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.1 |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Given a trivial error in a model, running "django manage.py validate" with Django 1.0.2 diagnoses the error, a spelling error/typo, very clearly (traceback trimmed, obviously):
File "/home/rocketry/rocketry/../rocketry/event/models.py", line 5, in <module>[[br]] from django.contrib.contentypes.models import ContentType[[br]] ImportError: No module named contentypes.models
With Django 1.1, it does its damndest to make you think there's something wrong in a completely different direction, making diagnosis damned near impossible:
$ python manage.py validate
Error: One or more models did not validate:
club.shiftworked: 'event' has a relation with model event.Event, which has either not been installed or is abstract.
club.attendance: 'event' has a relation with model event.Event, which has either not been installed or is abstract.
This is a clear regression in usability, at least for those of us whose code is not always flawless. BTW, at least in this case 1.1 produces the same unhelpful message for the shell command, so I don't know how you're supposed to fix bugs in models.py unless you have an older version of Django to hand.
Attachments (1)
Change History (15)
comment:1 by , 15 years ago
Description: | modified (diff) |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 15 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Looks simple enough. At a guess the try/except wrapper was thrown together to satisfy the case where it could be postponed and error reporting was simply not considered. This works for me in a small test case (validates OK with a small existing app, get the traceback when I inject a misspelt field type name). Unless there's some subtle problem with the postponing business, I think this will do.
try: models = import_module('.models', app_name) except ImportError, e: self.nesting_level -= 1 if can_postpone: # Either the app has no models, or the package is still being # imported by Python and the model module isn't available yet. # We will check again once all the recursion has finished (in # populate). self.postponed.append(app_name) return None raise e self.nesting_level -= 1
comment:3 by , 15 years ago
Previous patch considered harmful. It explodes if you have admindocs installed, as that app has no models.py. My guess is that this would happen any time the import fails because there's no models.py to be found.
Based on my current understanding of what's going on in import_module, this may be unfixable without extending the code brought back by time machine, as it fails to produce distinguishable results for the two cases (benign "just ain't no module here" and evil "the module failed to compile"). The old code could return the package containing "models" whether there was a "models" module or not; import_module raises ImportError for this (as well as when there's an actual compile error).
...okay, one last, bleary-eyed notion to try.
Yeah, you can, just barely, distinguish the benign and evil failures, but it seems to require parsing the text of the exception import throws - "No module named models" vs anything else. Yuck.
follow-up: 6 comment:5 by , 15 years ago
After having been busy with real life for a month, I'm back butting heads with this. Having read a scattered bunch of comments - in #8193, which was closed by r10088, as well as other discussions linked from there, and maybe another bug or two - I think the problem was that this use should never have been changed as it was in r10088. The __import__ non-bug ([''] causes double import) is irrelevant, since this always has ['model']. And the backported import_module cannot sanely be used to replace this __import__, since it conflates the two different cases of no models.py found, and failure importing an existant models.py. The latter needs to pass the exception so that the error, and what information is available in that original traceback, can be reported; not doing so kills developer's ponies at a furious rate. The former is necessary for applications that lack a models.py, such as admindocs, and that comes as a complete surprise to me, as I had thought that having models.py was one of the few things required of a Django application.
#10706 may be a different description of this same bug, or perhaps a different call site with the same problem in r10088?
comment:6 by , 15 years ago
Okay, not seeing anywhere to attach the patch file means it's not going to ask later either. So the hard way...
diff --git a/django/db/models/loading.py b/django/db/models/loading.py index e07aab4..1826559 100644 --- a/django/db/models/loading.py +++ b/django/db/models/loading.py @@ -70,10 +70,9 @@ class AppCache(object): """ self.handled[app_name] = None self.nesting_level += 1 - try: - models = import_module('.models', app_name) - except ImportError: - self.nesting_level -= 1 + mod = __import__(app_name, {}, {}, ['models']) + self.nesting_level -= 1 + if not hasattr(mod, 'models'): if can_postpone: # Either the app has no models, or the package is still being # imported by Python and the model module isn't available yet. @@ -81,10 +80,9 @@ class AppCache(object): # populate). self.postponed.append(app_name) return None - self.nesting_level -= 1 - if models not in self.app_store: - self.app_store[models] = len(self.app_store) - return models + if mod.models not in self.app_store: + self.app_store[mod.models] = len(self.app_store) + return mod.models def app_cache_ready(self): """
Diff against the 1.1 release tarball.
comment:7 by , 15 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I wrote my own patch and hope that it is the solution for this ticket:
http://lsaffre.blogspot.com/2009/11/patch-for-django-ticket-11696.html
by , 15 years ago
Attachment: | 20091107.diff added |
---|
comment:8 by , 15 years ago
I am reverting back the change to db/models/loading.py This seems to be all that is needed, and the unit test pass. Will review later for double loading issue once the following unit tests are written:
- valid models.py work
- missing models.py must not fail at this point (if it's needed then it will be reported later, not obscurely)
- models.py with a compile error must raise the actual import/compile exception so that the traceback shows the expected clues to what happened
comment:9 by , 15 years ago
Yeah, you can, just barely, distinguish the benign and evil failures, but it seems to require parsing the text of the exception import throws - "No module named models" vs anything else. Yuck.
the "yuck" solution.
Index: django/db/models/loading.py =================================================================== --- django/db/models/loading.py (revision 12257) +++ django/db/models/loading.py (working copy) @@ -73,7 +73,11 @@ self.nesting_level += 1 try: models = import_module('.models', app_name) - except ImportError: + except ImportError, exc: + msg = exc.args[0] + if not msg.startswith('No module named') or 'models' not in msg: + raise + self.nesting_level -= 1 if can_postpone: # Either the app has no models, or the package is still being
comment:10 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:11 by , 15 years ago
follow-up: 13 comment:12 by , 15 years ago
This changeset caused runserver
to crash on one of my projects:
$ ./manage.py runserver Validating models... Unhandled exception in thread started by <function inner_run at 0x9142304> Traceback (most recent call last): File "/home/akaihola/django/django/core/management/commands/runserver.py", line 48, in inner_run self.validate(display_num_errors=True) File "/home/akaihola/django/django/core/management/base.py", line 250, in validate num_errors = get_validation_errors(s, app) File "/home/akaihola/django/django/core/management/validation.py", line 28, in get_validation_errors for (app_name, error) in get_app_errors().items(): File "/home/akaihola/django/django/db/models/loading.py", line 135, in get_app_errors self._populate() File "/home/akaihola/django/django/db/models/loading.py", line 60, in _populate self.load_app(app_name, True) File "/home/akaihola/django/django/db/models/loading.py", line 77, in load_app imp.find_module('models', app_module.__path__) AttributeError: 'module' object has no attribute '__path__'
I'm trying to find the reason. Just thought to mention in case someone else googles for the same error message.
follow-up: 14 comment:13 by , 15 years ago
Replying to akaihola:
This changeset caused
runserver
to crash on one of my projects
Reason found. I had a module (not a package) in INSTALLED_APPS
for some dirty tricks. Changed it to a proper package with __init__.py
and models.py
, and all works correctly now.
comment:14 by , 15 years ago
Replying to akaihola:
I had a module (not a package) in
INSTALLED_APPS
for some dirty tricks. Changed it to a proper package with__init__.py
andmodels.py
, and all works correctly now.
Can you elaborate on why you had things set up as they were? I had noticed this difference yesterday, but had not had time to figure out what to do about it. The previous code suppressed the AttributeError when the loaded module did not have __path__
, and the current code does not.
The reason that check/suppression was originally put in referenced "odd things" in INSTALLED_APPS, like possibly modules from eggs, though the actual case that someone hit was where they had a mistake in INSTALLED_APPS and had 'django.core.mail' there for some reason. Modules from eggs are handled by a different code path now, so they won't hit that line of code.
I did actually test to see if 'django.core.mail' in INSTALLED_APPS would raise an error, and it did not, so I guessed something else had changed to prevent the error for the edge case of a non-package in INSTALLED_APPS. But actually the something else that had changed is 'django.core.mail' is now a package on trunk...I noticed the error being raised when I happened to try something in my test project setup on the 1.1.X branch.
It's easy enough to suppress the error, I just wasn't immediately sure whether that was the right thing to do. The effect is to hide the fact that something not-quite-right is listed in installed apps. So I'm wondering for what reason you had something that was not a valid app package listed in INSTALLED_APPS?
This looks to be due to r10088, which changed:
to:
in django/db/models/loading.py
The new code's try/except ImportError is not equivalent to the old code's testing if the returned mod has a 'models' attribute, as the new code now swallows import errors that the old code used to raise.
I have no idea how to fix it, though, since I don't know what would be equivalent to what the old code did when using the new import_module.