Opened 15 years ago

Closed 10 years ago

#11448 closed Bug (fixed)

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

Reported by: Dennis Kaarsemaker Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: dennis.kaarsemaker@…, jrose@…, Anton Strogonoff Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Karen Tracey)

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)

Attachments (4)

proj1.tar.gz (933 bytes ) - added by anonymous 15 years ago.
Minimal project to demonstrate the bug
clear_related_cache.diff (1.5 KB ) - added by Dennis Kaarsemaker 15 years ago.
clear_related_cache_v2.diff (3.2 KB ) - added by Dennis Kaarsemaker 15 years ago.
updated patch, including testcase
patch.diff (885 bytes ) - added by josephdrose 15 years ago.
Patch to rebuild cache if key error

Download all attachments as: .zip

Change History (23)

by anonymous, 15 years ago

Attachment: proj1.tar.gz added

Minimal project to demonstrate the bug

comment:1 by Dennis Kaarsemaker, 15 years ago

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.

by Dennis Kaarsemaker, 15 years ago

Attachment: clear_related_cache.diff added

comment:2 by Dennis Kaarsemaker, 15 years ago

Has patch: set

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).

comment:3 by Alex Gaynor, 15 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

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.

comment:4 by Dennis Kaarsemaker, 15 years ago

  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?

comment:5 by Alex Gaynor, 15 years ago

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.

by Dennis Kaarsemaker, 15 years ago

Attachment: clear_related_cache_v2.diff added

updated patch, including testcase

comment:6 by Dennis Kaarsemaker, 15 years ago

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.

comment:7 by Dennis Kaarsemaker, 15 years ago

Needs tests: unset

comment:8 by Dennis Kaarsemaker, 15 years ago

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

comment:9 by Dennis Kaarsemaker, 15 years ago

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.

by josephdrose, 15 years ago

Attachment: patch.diff added

Patch to rebuild cache if key error

comment:10 by josephdrose, 15 years ago

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.

comment:11 by josephdrose, 15 years ago

Cc: jrose@… added

comment:12 by Julien Phalip, 14 years ago

Severity: Normal
Type: Bug

comment:13 by Karen Tracey, 14 years ago

#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.

comment:14 by Julien Phalip, 14 years ago

Patch needs improvement: set

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.

comment:15 by Anton Strogonoff, 14 years ago

Cc: Anton Strogonoff added
Easy pickings: unset

Edit: didn't touched anything yet apparently somehow unset the "easy" flag. Turning back on, sorry for troubles.

Last edited 14 years ago by Anton Strogonoff (previous) (diff)

comment:16 by Anton Strogonoff, 14 years ago

Easy pickings: set

comment:17 by Ramiro Morales, 13 years ago

Easy pickings: unset

comment:18 by Karen Tracey, 13 years ago

Description: modified (diff)
UI/UX: unset

comment:19 by Aymeric Augustin, 10 years ago

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top