[Django] #22462: Loading issue of models declared in tests.py due to a combination of AdminConfig, System checks, and Model._meta caching.
Django
AdminConfig, System checks, and Model._meta caching.
---------------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:1>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:2>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:3>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:4>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
See also #7835.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:5>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:6>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 [comment:4 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:7>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:8>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:9>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
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 how much this behavior should be solidified in Django
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:10>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Replying to [comment:10 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:11>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:12>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:13>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* severity: Release blocker => Normal
Comment:
I'm removing the release blocker flag since this wasn't an officially
documented feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:14>
Django
AdminConfig, System checks, and Model._meta caching.
Reporter: loic84 | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* status: new => closed
* resolution: => duplicate
Comment:
Closing as a duplicate of #7835.
--
Ticket URL: <https://code.djangoproject.com/ticket/22462#comment:15>