Opened 12 years ago

Closed 11 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)

patch (1.1 KB ) - added by Klaas van Schelven 12 years ago.

Download all attachments as: .zip

Change History (4)

by Klaas van Schelven, 12 years ago

Attachment: patch added

comment:1 by Klaas van Schelven, 12 years ago

Cc: Klaas van Schelven added
Has patch: set

comment:2 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:3 by Aymeric Augustin, 11 years ago

Keywords: app-loading added
Resolution: fixed
Status: newclosed

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

Creating test database for alias 'default'...
E
======================================================================
ERROR: test_get_field_by_name (testapp.tests.SimpleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "~/django/django/db/models/options.py", line 371, in get_field_by_name
    return self._name_map[name]
KeyError: 'userprofile'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "~/django/t18510/testapp/tests.py", line 10, in test_get_field_by_name
    User._meta.get_field_by_name('userprofile')
  File "~/django/django/db/models/options.py", line 377, in get_field_by_name
    % (self.object_name, name))
django.db.models.fields.FieldDoesNotExist: User has no field named 'userprofile'

----------------------------------------------------------------------
Ran 1 test in 0.002s

FAILED (errors=1)
Destroying test database for alias 'default'...

stable/1.7.x -- explicit exception

Traceback (most recent call last):
  File "~/django/django/db/models/options.py", line 414, in get_field_by_name
    return self._name_map[name]
AttributeError: 'Options' object has no attribute '_name_map'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "~/django/django/db/models/options.py", line 559, in get_all_related_m2m_objects_with_model
    cache = self._related_many_to_many_cache
AttributeError: 'Options' object has no attribute '_related_many_to_many_cache'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "~/django/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "~/django/django/core/management/__init__.py", line 354, in execute
    django.setup()
  File "~/django/django/__init__.py", line 21, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "~/django/django/apps/registry.py", line 106, in populate
    app_config.import_models(all_models)
  File "~/django/django/apps/config.py", line 190, in import_models
    self.models_module = import_module(models_module_name)
  File "~/.virtualenvs/django/lib/python3.4/importlib/__init__.py", line 104, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 2231, in _gcd_import
  File "<frozen importlib._bootstrap>", line 2214, in _find_and_load
  File "<frozen importlib._bootstrap>", line 2203, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1200, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 1129, in _exec
  File "<frozen importlib._bootstrap>", line 1448, in exec_module
  File "<frozen importlib._bootstrap>", line 321, in _call_with_frames_removed
  File "~/django/t18510/testapp/models.py", line 5, in <module>
    User._meta.get_field_by_name('username')
  File "~/django/django/db/models/options.py", line 416, in get_field_by_name
    cache = self.init_name_map()
  File "~/django/django/db/models/options.py", line 445, in init_name_map
    for f, model in self.get_all_related_m2m_objects_with_model():
  File "~/django/django/db/models/options.py", line 561, in get_all_related_m2m_objects_with_model
    cache = self._fill_related_many_to_many_cache()
  File "~/django/django/db/models/options.py", line 575, in _fill_related_many_to_many_cache
    for klass in self.apps.get_models():
  File "~/.virtualenvs/django/lib/python3.4/functools.py", line 428, in wrapper
    result = user_function(*args, **kwds)
  File "~/django/django/apps/registry.py", line 156, in get_models
    self.check_ready()
  File "~/django/django/apps/registry.py", line 119, in check_ready
    raise RuntimeError("App registry isn't ready yet.")
RuntimeError: App registry isn't ready yet.
Note: See TracTickets for help on using tickets.
Back to Top