Loading issue of models declared in tests.py due to a combination of AdminConfig, System checks, and Model._meta caching.

[loic84]

I just had a very weird problem with my test suite, where calls to select_related didn't seem to work and busted every assertNumQueries while still returning correct values. This only happened for models defined in tests.py which inherited from models from another app's models.py.

app:
    models.py
        class A(models.Model):
            pass
    admin.py
        @admin.register(A)
        class AAdmin(admin.ModelAdmin):
            list_filters = 'id',
app2:
    tests.py
        class B(A):
            pass

The problem turned out to be that the various caches in A._meta were wrong (those used by get_field_by_name() and friends). Calls to select_related('b') were silently ignored (because that's what select_related do for non-existing relations) but calls to filter(b=something) would have failed with a FieldError despite A having the A.b descriptor.

Here is how the cache got to hold the wrong values:

• Apps.populate() calls AdminConfig.ready().
• AdminConfig.ready() calls autodiscover().
• autodiscover() causes AAdmin to register, which in turn triggers ModelAdmin.check().
• ModelAdmin.check() inspects the model fields and fills up the caches in _meta by doing so.
• app2.tests.B is loaded but it's already too late.

[Simon Charette]

Well there's definitely something to fix here, do you think this could be related to #22459?

What about starting by making select_related complain when a non-existing field is referenced?

[loic84]

@charettes hard to tell for #22459, I'm gonna ask the OP to try commenting out the admin from INSTALLED_APPS.

I believe the select_related issue is tracked at #10414.

[Anssi Kääriäinen]

One idea is to make loading of a new model to clear all ._meta caches of all the models related to the currently loaded model. That is, as final step of loading a model do something like the following (pseudo-code):

    for related_model in loaded_model._meta.get_related_models():
        related_model._meta.clear_caches()

The situation is a bit more complex, as if you have a setup:

class A(models.Model):
    pass

class AProxy(models.Model):
    pass

class B(models.Model):
    a = models.ForeignKey(A)

Then AProxy's caches wouldn't be cleared with the above snippet, just A's caches. So the code would have to also fetch all childs of the related models, and clear the caches of those, too.

Another idea is to have a global (or in appcache) models_seen counter that is incremented for each model loaded. When caching the relations in model._meta, also mark the current count of seen models. When using the cache, first check if the cache has same models_seen value as the appcache. If not, recache the values. Yet another variation of this is to just clear all caches of all the seen models when a model is loaded - this leads to O(n^{2) performance, but it might be we already have that problem due to .check() calls.}

In general the related model handling and caching in the Options class isn't clean. It has evolved one addition at a time to a state where it isn't easy to understand. IIRC there is a GSoC proposal for improving the ._meta (at least making a public introspection API is in that plan), so maybe refactoring of caching can be done as part of that work.

In short, finding a good solution for model._meta caching requires some investigation.

As for select_related() ignoring incorrect fields - this has a bit of history. In earlier versions of Django (pre 1.6?) QuerySet caching was implemented in a way where exceptions inside iteration lead to returning an empty list. And select_related() handling is done as part of iteration, so it was better to not throw exceptions. However this has now changed and we could throw exceptions for broken select_related() calls now. That is a good idea, though it might require a deprecation period (it will break existing code, but the code never worked as intended so errors are a good thing).

[Aymeric Augustin]

This problem was construed as a complex interaction but it don't think it involves that many moving pieces. It's "just" a cache invalidation problem — of course, such problems aren't easy ;-)

I believe the proper fix is to discourage declaring models outside of models.py. Apps.register_model could complain when it's called after app loading has completed, for instance.

The reason is, models declared outside of models.py don't exist for Django until they're imported. This means that app loading can complete without seeing all the models, and that has all sorts of hard-to-diagnose side effects.

Anyway, I'm very strongly -1 on adding a special case in django.setup() for specifically for models declared in tests.py. This looks like bending Django to your personal use case without consideration for the general problem. What if I want to declare my models it test_models.py, would you add another special case?

[Aymeric Augustin]

See also #7835.

[Aymeric Augustin]

https://code.djangoproject.com/ticket/7835#comment:24 says that putting test models in tests.py used to work, based on how Django used to be structured.

https://code.djangoproject.com/ticket/7835#comment:32 implies that this ability was lost with the new test discovery (1.6).

In all cases, I'm inclined to close this as a duplicate of #7835. This ticket is based on the premise that Django supports declaring models in tests.py, but that isn't true.

[loic84]

I would have sworn it was supported and documented, but there is clearly no evidence of that. This assumption probably came from the time we documented that tests were discovered in both models.py and tests.py, I probably assumed it was also the case for models, especially since it has worked for me until recently.

After a quick search on GitHub I spotted a couple of projects with models in their project_package/tests package, namely django-extensions, django-model-utils; but it's not always obvious how tests are run with third party projects so they may not be affected by the issue.

I'm not trying to bend Django my way, I was genuinely under the impression that this was a regression and was only trying to help. I'll be reworking my tests as this is unsupported, and I suggest anyone in the same position do the same.

Replying to aaugustin:

What if I want to declare my models it test_models.py, would you add another special case?

The idea was that you could import models from anywhere into tests.py the same way you can do with models.py, hence the special casing of tests.py. But indeed, this was based on the flawed premise that this was ever supported/documented.

[Aymeric Augustin]

I tried adding a warning to help diagnose the problem but it creates many false positives when running Django's test suite, probably because Django's own tests routinely abuse the internals:

diff --git a/django/apps/registry.py b/django/apps/registry.py
index 41095bd..4e042a7 100644
--- a/django/apps/registry.py
+++ b/django/apps/registry.py
@@ -190,6 +190,14 @@ class Apps(object):
         return self.get_app_config(app_label).get_model(model_name.lower())
 
     def register_model(self, app_label, model):
+        # Deferred lookups dynamically create model classes at run time.
+        if self is apps and self.ready and not model._deferred:
+            warnings.warn(
+                "Model '%s' was created after the app registry was fully "
+                "initialized. Relations involving this model may not work "
+                "correctly. Make sure it's imported in the models module "
+                "of an application listed in INSTALLED_APPS." % model,
+                RuntimeWarning, stacklevel=2)
         # Since this method is called when models are imported, it cannot
         # perform imports because of the risk of import loops. It mustn't
         # call get_app_config().
diff --git a/tests/runtests.py b/tests/runtests.py
index e852346..2748eaf 100755
--- a/tests/runtests.py
+++ b/tests/runtests.py
@@ -165,7 +165,12 @@ def setup(verbosity, test_labels):
             settings.INSTALLED_APPS.append(module_label)
             app_config = AppConfig.create(module_label)
             apps.app_configs[app_config.label] = app_config
-            app_config.import_models(apps.all_models[app_config.label])
+            with warnings.catch_warnings():
+                warnings.filterwarnings(
+                    'ignore',
+                    "Model '.*' was created after the app registry",
+                    RuntimeWarning)
+                app_config.import_models(apps.all_models[app_config.label])
             apps.clear_cache()
 
     return state

[loic84]

I tried running the test suite without catching the warnings. Lot of them are justified, like tests that create models inside test_ methods, but others I couldn't figure out why they would trigger warnings. For instance runtests.py raw_query triggers warning for every model in models.py and I couldn't find anything special about this test package.

[Preston Timmons]

loic: You're not crazy. It's just that this feature is pseudo-documented.

aaugustin: Declaring models in tests.py still works in 1.6.

Custom user model tests depend on it and recommend this as part of the 1.6 upgrade path:

​https://docs.djangoproject.com/en/dev/releases/1.6/#custom-user-models-in-tests
​https://docs.djangoproject.com/en/dev/topics/auth/customizing/#custom-users-and-testing-fixtures

What changed with discover runner is that it now requires an expicit import of django.contrib.auth.tests.custom_user.CustomUser in tests.py. Unlike the old test runner, the discover runner doesn't always import every test file. Hence, the supplied CustomUser model wouldn't be registered.

Additional discussion is in #21164.

https://code.djangoproject.com/ticket/21164#comment:9 Starting here is discussion on whether this behavior should really be depended on.

[Aymeric Augustin]

Replying to prestontimmons:

aaugustin: Declaring models in tests.py still works in 1.6.

It still works in 1.7 too, for some value of works ;-) More accurately, the set of cases where it doesn't work seems to have increased a bit in 1.6 and again in 1.7.

[loic84]

It's a hack (sys.argv parsing, private APIs) but if like me you have a large test suite and really need a stopgap solution you may find this helpful:

import sys

from django.apps import AppConfig
from django.utils.module_loading import import_module, module_has_submodule


TEST_MODELS_MODULE_NAME = 'tests'


class AppConfigWithTestModels(AppConfig):
    def import_models(self, *args, **kwargs):
        super(AppConfigWithTestModels, self).import_models(*args, **kwargs)

        if len(sys.argv) >= 2 and sys.argv[1] == 'test':
            if module_has_submodule(self.module, TEST_MODELS_MODULE_NAME):
                models_module_name = '%s.%s' % (self.name, TEST_MODELS_MODULE_NAME)
                models_module = import_module(models_module_name)
                if self.models_module is None:
                    self.models_module = models_module

[Preston Timmons]

This might be a dumb idea, but what if test models could be specified with an attribute on the Meta class? For instance:

class TestModel(models.Model):

    class Meta:
        test = True

They could still live in models.py, to solve the app-loading issue, but only be synced when the test suite is running.

This doesn't solve the use-cases in Django's test suite, where models are sometimes defined for the duration of a test method and cleared from the app cache, but it's good enough for any time I've used test models.

[Aymeric Augustin]

I'm removing the release blocker flag since this wasn't an officially documented feature.

[Aymeric Augustin]

Closing as a duplicate of #7835.