Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#21410 closed Bug (fixed)

Error when trying to ignore reverse relationships with related_name using the "+"

Reported by: Troy Grosfield Owned by: loic84
Component: Database layer (models, ORM) Version: 1.6
Severity: Release blocker Keywords: related_name,
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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='hello@world.com',
                                        password='hi')[0]
        u2 = User.objects.get_or_create(username='hello2',
                                        email='hello@world.com',
                                        password='hi')[0]
        u3 = User.objects.get_or_create(username='hello3',
                                        email='hello@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

Change History (13)

comment:1 by loic84, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:2 by loic84, 10 years ago

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

comment:3 by loic84, 10 years ago

Has patch: set
Owner: changed from nobody to loic84
Status: newassigned

comment:4 by loic84, 10 years ago

Patch needs improvement: set

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.

comment:5 by loic84, 10 years ago

Patch needs improvement: unset

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.

comment:6 by anonymous, 10 years ago

Triage Stage: AcceptedReady for checkin

Tested the change from loic and all my tests are passing again. Thanks for the quick turnaround loic84!

comment:7 by Anssi Kääriäinen, 10 years ago

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.

comment:8 by loic84, 10 years ago

Updated the PR with @akaariai's suggestion.

comment:9 by Anssi Kääriäinen <akaariai@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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

comment:10 by Anssi Kääriäinen <akaariai@…>, 10 years ago

In 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

comment:11 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In d3b71b976dee4b02908235b8007db254a9795268:

Fixed #21760 -- prefetch_related used an inefficient query for reverse FK.

Regression introduced by commit 9777442. Refs #21410.

comment:12 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In 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

comment:13 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In 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

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