Code

Opened 5 years ago

Closed 5 weeks 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: master
Severity: Normal Keywords:
Cc: dennis.kaarsemaker@…, jrose@…, zuko 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 kmtracey)

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 5 years ago.
Minimal project to demonstrate the bug
clear_related_cache.diff (1.5 KB) - added by Dennis Kaarsemaker 5 years ago.
clear_related_cache_v2.diff (3.2 KB) - added by Dennis Kaarsemaker 5 years ago.
updated patch, including testcase
patch.diff (885 bytes) - added by josephdrose 4 years ago.
Patch to rebuild cache if key error

Download all attachments as: .zip

Change History (23)

Changed 5 years ago by anonymous

Minimal project to demonstrate the bug

comment:1 Changed 5 years ago by Dennis Kaarsemaker

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

Changed 5 years ago by Dennis Kaarsemaker

comment:2 Changed 5 years ago by Dennis Kaarsemaker

  • 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 Changed 5 years ago by Alex

  • Needs tests set
  • Triage 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.

comment:4 Changed 5 years ago 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?

comment:5 Changed 5 years ago 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.

Changed 5 years ago by Dennis Kaarsemaker

updated patch, including testcase

comment:6 Changed 5 years ago 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.

comment:7 Changed 5 years ago by seveas

  • Needs tests unset

comment:8 Changed 5 years ago by seveas

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

comment:9 Changed 5 years ago 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.

Changed 4 years ago by josephdrose

Patch to rebuild cache if key error

comment:10 Changed 4 years ago 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.

comment:11 Changed 4 years ago by josephdrose

  • Cc jrose@… added

comment:12 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:13 Changed 3 years ago by kmtracey

#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 Changed 3 years ago by julien

  • 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 Changed 3 years ago by zuko

  • Cc zuko added
  • Easy pickings unset

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

Last edited 3 years ago by zuko (previous) (diff)

comment:16 Changed 3 years ago by zuko

  • Easy pickings set

comment:17 Changed 3 years ago by ramiro

  • Easy pickings unset

comment:18 Changed 3 years ago by kmtracey

  • Description modified (diff)
  • UI/UX unset

comment:19 Changed 5 weeks ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.