[Django] #21410: Error when trying to ignore reverse relationships with related_name using the "+"
Django
the "+"
----------------------------------------------+---------------------------
Reporter: troygrosfield | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6
Severity: Normal | Keywords: related_name,
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+---------------------------
The setup and failing test can be seen below. It's trying to use the
related name as an actual field. I didn't see this issue until I just
upgraded to django 1.6 yesterday. Am I missing something here?
{{{
# models.py
from django.db import models
from django.contrib.auth.models import User
class ModelA(models.Model):
created_user = models.ForeignKey(
User,
related_name='%(app_label)s_%(class)s_created_user+')
last_modified_user = models.ForeignKey(
User,
related_name='%(app_label)s_%(class)s_last_modified_user+')
}}}
{{{
# tests.py
from tests.models import ModelA
from django.contrib.auth.models import User
class Django16PrefetchBug(TestCase):
def test_prefetch_bug(self):
u1 = User.objects.get_or_create(username='hello1',
email='he...@world.com',
password='hi')[0]
u2 = User.objects.get_or_create(username='hello2',
email='he...@world.com',
password='hi')[0]
u3 = User.objects.get_or_create(username='hello3',
email='he...@world.com',
password='hi')[0]
m1 = ModelA.objects.create(created_user=u1,
last_modified_user=u2)
m2 = ModelA.objects.create(created_user=u2,
last_modified_user=u3)
m3 = ModelA.objects.create(created_user=u3,
last_modified_user=u2)
x = ModelA.objects.all().prefetch_related('created_user')
# The line below fails when evaluating the query.
y = list(x)
}}}
{{{
======================================================================
ERROR: test_prefetch_bug (tests.prefetch_bug.Django16PrefetchBug)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/troy/github/django-recurrence/tests/prefetch_bug.py", line
249, in test_prefetch_bug
y = list(x)
File "/path/to/site-packages/django/db/models/query.py", line 96, in
__iter__
self._fetch_all()
File "/path/to/site-packages/django/db/models/query.py", line 856, in
_fetch_all
self._prefetch_related_objects()
File "/path/to/site-packages/django/db/models/query.py", line 517, in
_prefetch_related_objects
prefetch_related_objects(self._result_cache,
self._prefetch_related_lookups)
File "/path/to/site-packages/django/db/models/query.py", line 1598, in
prefetch_related_objects
obj_list, additional_prl = prefetch_one_level(obj_list, prefetcher,
attr)
File "/path/to/site-packages/django/db/models/query.py", line 1697, in
prefetch_one_level
prefetcher.get_prefetch_queryset(instances)
File "/path/to/site-packages/django/db/models/fields/related.py", line
277, in get_prefetch_queryset
qs = self.get_queryset(instance=instances[0]).filter(**query)
File "/path/to/site-packages/django/db/models/query.py", line 590, in
filter
return self._filter_or_exclude(False, *args, **kwargs)
File "/path/to/site-packages/django/db/models/query.py", line 608, in
_filter_or_exclude
clone.query.add_q(Q(*args, **kwargs))
File "/path/to/site-packages/django/db/models/sql/query.py", line 1198,
in add_q
clause = self._add_q(where_part, used_aliases)
File "/path/to/site-packages/django/db/models/sql/query.py", line 1232,
in _add_q
current_negated=current_negated)
File "/path/to/site-packages/django/db/models/sql/query.py", line 1100,
in build_filter
allow_explicit_fk=True)
File "/path/to/site-packages/django/db/models/sql/query.py", line 1351,
in setup_joins
names, opts, allow_many, allow_explicit_fk)
File "/path/to/site-packages/django/db/models/sql/query.py", line 1274,
in names_to_path
"Choices are: %s" % (name, ", ".join(available)))
FieldError: Cannot resolve keyword u'tests_modela_created_user+' into
field. Choices are: date_joined, email, first_name, groups, id, is_active,
is_staff, is_superuser, last_login, last_name, password, user_permissions,
username
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
the "+"
Reporter: troygrosfield | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: related_name, | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_better_patch: => 0
* needs_docs: => 0
* severity: Normal => Release blocker
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
I'm pretty sure this is a regression.
Bisected to 97774429aeb54df4c09895c07cd1b09e70201f7d.
I'll need to investigate some more but for now I'll mark it as a release
blocker.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:1>
Django
the "+"
Reporter: troygrosfield | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: related_name, | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
That should more or less do it https://gist.github.com/loic/7382392 (quick
experiment, not commit material).
For prefetching we probably shouldn't rely on the existence of reverse
managers.
The other prefetchers may be suffering from the same issue as well, we'll
need to investigate that.
Another problem is that my fix is not going to play well with composite
fields...
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:2>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: related_name, | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => loic84
* has_patch: 0 => 1
Comment:
PR https://github.com/django/django/pull/1898.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:3>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: related_name, | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_better_patch: 0 => 1
Comment:
We test directly `ForeignObject`
https://github.com/django/django/blob/master/tests/foreign_object/models.py#L25
which cause a `'ForeignObject' object has no attribute 'related_field'`
failure.
We may need to use something else than `related_field`, I'll get back to
this later on.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:4>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: related_name, | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by loic84):
* needs_better_patch: 1 => 0
Comment:
`ForeignKey().related_field` is a shortcut for
`ForeignObject().foreign_related_fields[0]`, I've update the patch to use
the latter so it works with plain `ForeignObject`.
This should be ready for reviews.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:5>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* stage: Accepted => Ready for checkin
Comment:
Tested the change from loic and all my tests are passing again. Thanks
for the quick turnaround loic84!
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:6>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
There is a problem with multicolumn fields here. I think I will go with an
approach that looks something like this:
{{{
if field.rel.is_hidden():
# code like in patch
else:
# code pre-patch
}}}
The reason is that we do want to use the `fk__in` lookup if at all
possible. Especially for multicolumn lookups what `fk__in=instances` does
can be quite complex. We don't want to duplicate all the logic needed
here. For 1.7 we will likely need to invent something better, as otherwise
we will end up having this exact same problem for hidden multicolumn
foreign keys.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:7>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
Updated the PR with @akaariai's suggestion.
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:8>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Component: Database layer | Version: 1.6
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anssi Kääriäinen <akaariai@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cb83448891f2920c926f61826d8eae4051a3d8f2"]:
{{{
#!CommitTicketReference repository=""
revision="cb83448891f2920c926f61826d8eae4051a3d8f2"
Fixed #21410 -- prefetch_related() for ForeignKeys with related_name='+'
Regression introduced by commit 9777442.
Thanks to trac username troygrosfield for the report and test case.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:9>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"b107421acfc4046ffaa799aceef2c3b4877207f2"]:
{{{
#!CommitTicketReference repository=""
revision="b107421acfc4046ffaa799aceef2c3b4877207f2"
[1.6.x] Fixed #21410 -- prefetch_related() for ForeignKeys with
related_name='+'
Regression introduced by commit 9777442.
Thanks to trac username troygrosfield for the report and test case.
Backpatch of cb83448891f2920c926f61826d8eae4051a3d8f2 from master.
Conflicts:
tests/prefetch_related/models.py
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:10>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"d3b71b976dee4b02908235b8007db254a9795268"]:
{{{
#!CommitTicketReference repository=""
revision="d3b71b976dee4b02908235b8007db254a9795268"
Fixed #21760 -- prefetch_related used an inefficient query for reverse FK.
Regression introduced by commit 9777442. Refs #21410.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:11>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"6b3a8d27052bb5b242ab6c1c2fb0d6da5b49681d"]:
{{{
#!CommitTicketReference repository=""
revision="6b3a8d27052bb5b242ab6c1c2fb0d6da5b49681d"
[1.7.x] Fixed #21760 -- prefetch_related used an inefficient query for
reverse FK.
Regression introduced by commit 9777442. Refs #21410.
Backport of d3b71b976d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:12>
Django
the "+"
Reporter: troygrosfield | Owner: loic84
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: related_name, | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"1252b77824639fba398cf70586bdd75c42f86fdf"]:
{{{
#!CommitTicketReference repository=""
revision="1252b77824639fba398cf70586bdd75c42f86fdf"
[1.6.x] Fixed #21760 -- prefetch_related used an inefficient query for
reverse FK.
Regression introduced by commit 9777442. Refs #21410.
Conflicts:
tests/prefetch_related/tests.py
Backport of d3b71b976d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21410#comment:13>