validate command in 1.1 hides errors in models that used to be shown

[martin maney]

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.

[Karen Tracey]

This looks to be due to r10088, which changed:

        self.nesting_level += 1
        mod = __import__(app_name, {}, {}, ['models'])
        self.nesting_level -= 1
        if not hasattr(mod, 'models'):

to:

        self.nesting_level += 1
        try:
            models = import_module('.models', app_name)
        except ImportError:
            self.nesting_level -= 1

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.

[martin maney]

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

[martin maney]

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.

[Karen Tracey]

#11779 reported this again.

[martin maney]

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?

[anonymous]

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.

[Luc Saffre]

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

[CarlFK]

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:

1. valid models.py work
2. missing models.py must not fail at this point (if it's needed then it will be reported later, not obscurely)
3. models.py with a compile error must raise the actual import/compile exception so that the traceback shows the expected clues to what happened

[michael.john.kirk@…]

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

[Karen Tracey]

(In [12950]) Fixed #11696: Changed app loading code so that it does not swallow import errors that used to be (prior to r10088) raised.

[Karen Tracey]

(In [12951]) [1.1.X] Fixed #11696: Changed app loading code so that it does not swallow import errors that used to be (prior to r10088) raised.

r12950 from trunk.

[Antti Kaihola]

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.

[Antti Kaihola]

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.

[Karen Tracey]

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 and models.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?