Opened 21 months ago

Closed 21 months ago

Last modified 16 months ago

#21410 closed Bug (fixed)

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

Reported by: troygrosfield 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 Changed 21 months ago by loic84

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 21 months ago 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...

comment:3 Changed 21 months ago by loic84

  • Has patch set
  • Owner changed from nobody to loic84
  • Status changed from new to assigned

comment:4 Changed 21 months ago by loic84

  • 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 Changed 21 months ago by loic84

  • 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 Changed 21 months ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:7 Changed 21 months ago 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.

comment:8 Changed 21 months ago by loic84

Updated the PR with @akaariai's suggestion.

comment:9 Changed 21 months ago by Anssi Kääriäinen <akaariai@…>

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

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 Changed 21 months ago by Anssi Kääriäinen <akaariai@…>

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 Changed 16 months ago by Loic Bistuer <loic.bistuer@…>

In d3b71b976dee4b02908235b8007db254a9795268:

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

Regression introduced by commit 9777442. Refs #21410.

comment:12 Changed 16 months ago by Loic Bistuer <loic.bistuer@…>

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 Changed 16 months ago by Loic Bistuer <loic.bistuer@…>

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