Defining relationships after querying a model does not add a reverse lookup to the referenced model

[Dennis Kaarsemaker]

Ok, that sounds vague but I don't know how to better describe it. Basically it boils down to having this in models.py:

from django.db import models
import os

class c1(models.Model):
    name = models.CharField("Name", max_length=30)

default_c1 = c1.objects.get(name="non_existant")

class c2(models.Model):
    other = models.ForeignKey(c1, default=default_c1)

Querying c1 later with c1.objects.filter(c2__pk=0) will fail:

FieldError: Cannot resolve keyword 'c2' into field. Choices are: id, name

Minimal testcase project attached (models.py is slightly bigger than above). You can reproduce the problem with:

# Create the database (clobbers test.db in the current dir)
./manage.py syncdb
# See that without querying in between it works
echo -e "from proj1.app1.models import c1\nc1.objects.filter(c2__pk=0)" | ./manage.py shell
# See that with querying in between it fails
echo -e "from proj1.app1.models import c1\nc1.objects.filter(c2__pk=0)" | BREAK_ME=1 ./manage.py shell

Found on 1.0.2, confirmed with trunk (fresh checkout, less than 30 minutes ago)

[anonymous]

Minimal project to demonstrate the bug

[Dennis Kaarsemaker]

Manually deleting the _related_objects_cache (and for good measure the _related_many_to_many_cache) works around the problem. I'd rather see django do that when defining a new model. Patch to follow soon.

[Dennis Kaarsemaker]

Attached patch clears the relevant _related_*_cache in contribute_to_related_class of OneToOneField, ManyToManyField and ForeignKey. This makes new relationships visible even if a query that triggers filling this cache has been executed beforehand. Tested against the test project (and another, proprietary, one).

[Alex Gaynor]

Couple things:

1. del is a statement, so no need for the parentheses
2. I think the patch reads a little better as a hasattr() test instead of catching the exception. Also put a comment next to each of these saying if the cache is populated we clear it out because it needs to be repopulated to include the attr we're about to assign.
3. Can you put a testcase in the Django tests that demonstrates that this has been fixed.

Otherwise the patch looks good to me.

[Dennis Kaarsemaker]

1. That's my coding style slipping though, will fix
2. I followed the style in get_all_related_objects_with_model, but agree that a hasattr() reads better.
3. I'm very unfamiliar with django's test setup. Can you point me to some documentation?

[Alex Gaynor]

1) Yeah, I understand, but we try to follow PEP8 where possible.

3) Here are the docs on Django's test framework: ​http://docs.djangoproject.com/en/dev/topics/testing/?from=olddocs#writing-tests, here's some info on getting setup to run the test suite: ​http://lazypython.blogspot.com/2008/11/running-django-test-suite.html, lastly take a look at the tests/regressiontests directory of the source. Each directory in there is a set of self contained tests. However, the nature of this problem makes me think it will be very difficult to tests, so if it seems impossible I wouldn't waste a ton of time on it.

[Dennis Kaarsemaker]

updated patch, including testcase

[Dennis Kaarsemaker]

Updated patch: fixing del(), adding comments and adding a test case. ./runtests.py -v2 query_between_definitions fails without the (rest of the) patch applied and succeeds otherwise. I cheated a little bit by not actually running a query, but the calling init_name_map(). This is the bit that actually causes the problem and running a query would call this function too.

[Dennis Kaarsemaker]

#11247 is a different manifestation of this bug, which is more likely to occur.

[Dennis Kaarsemaker]

I retract this ticket, I just found that this creates issues elsewhere as well, which cannot be solved this easily. A warning in the docs about not doing models.py-level queries or mixing forms and models would be appreciated though.

[josephdrose]

Patch to rebuild cache if key error

[josephdrose]

I've upload a patch that rebuilds the cache if there is a key error in the options class. This should fix any instances where bad cache values cause a key error; also, there is minimal performance hit, as once a good cache is built, it is used.

[Karen Tracey]

#15771 looks like another case of this error, now triggered by importing from contrib.auth.admin before defining a model with a relationship to User -- something that used (1.2.X) to work. Not sure what has changed that it now (1.3) triggers this error.

[Julien Phalip]

The tests would need to be rewritten using unittests since this is now Django's preferred way. Also there seems to be multiple directions suggested for fixing this issue. One needs to clarify which approach suits best.

[Aymeric Augustin]

This is "fixed" by the app-loading refactor, in the sense that you get a RuntimeError: App registry isn't ready yet. with this definition of models. Making SQL queries at import time has never worked correctly.