Django

Code

Ticket #11448 (new)

Opened 9 months ago

Last modified 4 days ago

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

Reported by: Dennis Kaarsemaker Assigned to: nobody
Milestone: Component: Database layer (models, ORM)
Version: SVN Keywords:
Cc: dennis.kaarsemaker@booking.com, jrose@morristechnology.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

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(c2pk=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

proj1.tar.gz (0.9 kB) - added by anonymous on 07/09/09 09:36:58.
Minimal project to demonstrate the bug
clear_related_cache.diff (1.5 kB) - added by Dennis Kaarsemaker on 07/09/09 15:28:40.
clear_related_cache_v2.diff (3.2 kB) - added by Dennis Kaarsemaker on 07/09/09 16:45:04.
updated patch, including testcase
patch.diff (0.9 kB) - added by josephdrose on 03/18/10 14:41:19.
Patch to rebuild cache if key error

Change History

07/09/09 09:36:58 changed by anonymous

  • attachment proj1.tar.gz added.

Minimal project to demonstrate the bug

07/09/09 15:17:45 changed by Dennis Kaarsemaker

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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.

07/09/09 15:28:40 changed by Dennis Kaarsemaker

  • attachment clear_related_cache.diff added.

07/09/09 15:32:01 changed by Dennis Kaarsemaker

  • has_patch set to 1.

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

07/09/09 15:43:16 changed by Alex

  • needs_tests set to 1.
  • stage changed from Unreviewed to Accepted.

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.

07/09/09 15:51:48 changed by 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?

07/09/09 16:26:59 changed by Alex

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.

07/09/09 16:45:04 changed by Dennis Kaarsemaker

  • attachment clear_related_cache_v2.diff added.

updated patch, including testcase

07/09/09 16:47:55 changed by 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.

07/10/09 11:17:01 changed by seveas

  • needs_tests deleted.

07/10/09 13:46:33 changed by seveas

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

12/12/09 15:18:12 changed by seveas

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.

03/18/10 14:41:19 changed by josephdrose

  • attachment patch.diff added.

Patch to rebuild cache if key error

03/18/10 14:42:50 changed by 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.

03/18/10 14:43:10 changed by josephdrose

  • cc changed from dennis.kaarsemaker@booking.com to dennis.kaarsemaker@booking.com, jrose@morristechnology.com.

Add/Change #11448 (Defining relationships after querying a model does not add a reverse lookup to the referenced model)




Change Properties
Action