Opened 12 years ago
Closed 10 years ago
#18510 closed Bug (fixed)
Options._fill_related_objects_cache should assert for app_cache_ready() or deal with app_cache_ready() == False
Reported by: | Klaas van Schelven | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.4 |
Severity: | Normal | Keywords: | app-loading |
Cc: | Klaas van Schelven | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It is possible to end up in Options._fill_related_many_to_many_cache (https://github.com/django/django/blob/1.4/django/db/models/options.py#L388) while app_cache_ready() is False. Since that method iterates over get_models() (line 401) and saves its result in an object attribute regardless of the value of app_cache_ready() in line 409 this may result in the _related_objects_cache missing elements, causing get_field_by_name (and possibly other methods) to fail.
Steps to reproduce:
# models.py from django.db import models from django.contrib.auth.models import User User._meta.get_field_by_name('username') # alternatively, import the below to trigger the error. # from django.contrib.auth.admin import UserAdmin class UserProfile(models.Model): user = models.ForeignKey(User, unique=True, editable=False) first_name = models.CharField(max_length=255, blank=True) User._meta.ordering = ('userprofile__first_name',)
# tests.py from django.test import TestCase from django.contrib.auth.models import User class SimpleTest(TestCase): def test_get_field_by_name(self): # no assertions needed, the test simply fails User.objects.create(username='test') User._meta.get_field_by_name('userprofile') # similarly methods using get_field_by_name themselves fail, such as u = User.objects.all()[0]
---------------------------------------------------------------------- Traceback (most recent call last): File "/tmp/ve13/reverselookup/userprofile/tests.py", line 9, in test_get_field_by_name User._meta.get_field_by_name('userprofile') File "/tmp/ve14/lib/python2.6/site-packages/django/db/models/options.py", line 315, in get_field_by_name % (self.object_name, name)) !FieldDoesNotExist: User has no field named 'userprofile'
This fails in both Django 1.3 and Django 1.4.
Solution:
The canonical solution in options.py seems to be to check for the value of app_cache_ready() before actually setting the internal cache attribute. E.g. https://github.com/django/django/blob/1.4/django/db/models/options.py#L447 . I'm not sure whether that would fix all problems caused by this or whether an assert for app_cache_ready() at the top of the method would be more appropriate.
I'm currently not able to come up with a proper way of providing tests for the situation above, because the global state of app loading and such makes coming up with tests somewhat harder. If anyone can help me with that, that would be great. Alternatively perhaps someone is willing to take the above method of reproduction and the fact that no old tests break as enough reason for a patch?
Admittedly the above scenario appears contrived (calling User._meta.get_field_by_name('username') in a models.py file). However, this is simply the easiest way to reproduce the problem.
I ran into this when further debugging #18507 . In that case I was using from django.contrib.auth.admin import UserAdmin somewhere early in one of my models files. Since Django 1.3 adds more validation to the admins, one of which contained a call to get_field_by_name this resulted in a rather hard-to-debug problem when upgrading from Django 1.2 to Django 1.3 (or 1.4).
Attachments (1)
Change History (4)
by , 12 years ago
comment:1 by , 12 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:3 by , 10 years ago
Keywords: | app-loading added |
---|---|
Resolution: | → fixed |
Status: | new → closed |
This is "fixed" by the app-loading refactor, in the sense that Django now throws an explicit exception.
stable/1.6.x -- obscure test failure
stable/1.7.x -- explicit exception