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 )
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)
Change History (23)
by , 15 years ago
Attachment: | proj1.tar.gz added |
---|
comment:1 by , 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 , 15 years ago
Attachment: | clear_related_cache.diff added |
---|
comment:2 by , 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 , 15 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Couple things:
- del is a statement, so no need for the parentheses
- 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.
- 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 , 15 years ago
- That's my coding style slipping though, will fix
- I followed the style in get_all_related_objects_with_model, but agree that a hasattr() reads better.
- I'm very unfamiliar with django's test setup. Can you point me to some documentation?
comment:5 by , 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.
comment:6 by , 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 , 15 years ago
Needs tests: | unset |
---|
comment:8 by , 15 years ago
#11247 is a different manifestation of this bug, which is more likely to occur.
comment:9 by , 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.
comment:10 by , 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 , 15 years ago
Cc: | added |
---|
comment:12 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:13 by , 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 , 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 , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
Edit: didn't touched anything yet apparently somehow unset the "easy" flag. Turning back on, sorry for troubles.
comment:16 by , 14 years ago
Easy pickings: | set |
---|
comment:17 by , 13 years ago
Easy pickings: | unset |
---|
comment:18 by , 13 years ago
Description: | modified (diff) |
---|---|
UI/UX: | unset |
comment:19 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → 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.
Minimal project to demonstrate the bug