Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26092 closed Bug (fixed)

Regression when using order_by() on a ManyToManyField using the 'through' feature

Reported by: Wenzel Jakob Owned by: nobody
Component: Database layer (models, ORM) Version: 1.9
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

Hi,

I'm seeing a serious regression (AttributeError exception thrown) when using the order_by() feature on a ManyToManyField in combination with the 'through' feature.

This worked perfectly fine in Django 1.8.x, I'm seeing it for the first time now. IMHO it is a release blocker, but then the release is already out (so I set the severity as 'Normal').

See below for a minimal example:

class Person(models.Model):
    name = models.CharField(max_length=200)


class Publication(models.Model):
    title = models.CharField(max_length=200)
    authors = models.ManyToManyField(Person, through='Author')


class Author(models.Model):
    person = models.ForeignKey(Person, related_name='author_to_publication')
    publication = models.ForeignKey(Publication)
    order_id = models.PositiveSmallIntegerField(default=0)

    class Meta:
        ordering = ['order_id']

And here is an example test which causes the problem:

from django.test import TestCase

from .models import Person, Publication, Author


class Test(TestCase):
    def test(self):
        person = Person()
        person.save()
        publication = Publication(title='test publication')
        publication.save()
        author = Author(person=person, publication=publication, order_id=1)
        author.save()
        list(publication.authors.order_by('author_to_publication'))

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/query.py", line 234, in __repr__
    data = list(self[:REPR_OUTPUT_SIZE + 1])
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/query.py", line 258, in __iter__
    self._fetch_all()
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/query.py", line 1074, in _fetch_all
    self._result_cache = list(self.iterator())
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/query.py", line 52, in __iter__
    results = compiler.execute_sql()
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 837, in execute_sql
    sql, params = self.as_sql()
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 382, in as_sql
    extra_select, order_by, group_by = self.pre_sql_setup()
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 49, in pre_sql_setup
    order_by = self.get_order_by()
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 295, in get_order_by
    field, self.query.get_meta(), default_order=asc))
  File "/Users/wenzel/mysite/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 571, in find_ordering_name
    if field.is_relation and path and opts.ordering and name != field.attname:
AttributeError: 'ManyToOneRel' object has no attribute 'attname'

Change History (11)

comment:1 Changed 4 years ago by Tim Graham

Could you convert the steps to reproduce into a test case for Django's test suite (tests/m2m_through or m2m_through_regress, probably, and reusing existing models, if possible) and bisect to find the commit that caused the breakage?

comment:2 Changed 4 years ago by Wenzel Jakob

Hi --

unfortunately I don't have the cycles to dig deeper at the moment (I already gave up and switched back to Django 1.8.x).
My hope was that this would be easy for somebody from Django project to track down given a tiny self-contained example (which I submitted, see above).

Best, Wenzel

comment:3 Changed 4 years ago by Tim Graham

Description: modified (diff)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:4 Changed 4 years ago by Anssi Kääriäinen

I'll try to take a look today.

comment:5 Changed 4 years ago by Anssi Kääriäinen

I was going to introduce attname for Rel objects, but decided it is safer to use getattr(field, attname, None) as this will be backpatched. I think I got the functionality right.

As a side note, a thing to try is to introduce ForeignKey's attname field as a separate field in the model's meta. This would clean a lot of code. Instead of having a ForeignKey that also tries to mimic an integer/text/whatever field, you would have a ForeignKey that defines the relation, and an Integer/Text/...Field that contains the concrete value used by the relation. For example, this bug was at least partially caused by having a ForeignKey represent two different things (the attname check in compiler is for cases where you want to order by the concrete field instead of the relation field). Unfortunately changing the representation of foreign keys will likely be hard to implement in backwards compatible way. On the other hand, if we get the representation of foreign keys changed we have solved one of the hardest parts of multicolumn foreign keys.

comment:6 Changed 4 years ago by Anssi Kääriäinen

Has patch: set

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In ee596888:

Fixed #26092 -- Fixed QuerySet.order_by() regression with an M2M through model.

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

In 05e8fa83:

[1.9.x] Fixed #26092 -- Fixed QuerySet.order_by() regression with an M2M through model.

Backport of ee596888e1149864e7828f5cf63c0eda395744c3 from master

comment:9 Changed 4 years ago by Tim Graham <timograham@…>

In fb4272f:

Refs #26092 -- Added @skipUnlessDBFeature to a test.

comment:10 Changed 4 years ago by Tim Graham <timograham@…>

In c9d1d55:

[1.9.x] Refs #26092 -- Added @skipUnlessDBFeature to a test.

Backport of fb4272f0e6bbdaa3e19ed5fde59fdb5ab5a33baf from master

comment:11 Changed 4 years ago by Chris Lamb

FYI this was also affecting OneToOne fields, eg.:

class Parent(models.Model):
    pass
  
class Child(models.Model):
    parent = models.OneToOneField(Parent, related_name='child')
    ordering = models.IntegerField()

    class Meta:
        ordering = ('ordering',)

>>> Parent.objects.order_by('child')
# (boom)

Calling:

>>> Parent.objects.order_by('child__ordering')

.. would temporarily "fix" it.

Mentioning it in case you want to expand the release notes and for anyone else hit by this issue in 1.9.1

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