Opened 7 years ago

Closed 7 years ago

#6804 closed (fixed)

QuerySets should allow for ordering by intermediary tables

Reported by: floguy Owned by: nobody
Component: Uncategorized Version: queryset-refactor
Severity: Keywords:
Cc: floguy@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Right now if one model is related to another model through a third model, ordering as one would expect is not possible to achieve.

Here is an illustration (both using patch #6095 and not):

class Group(models.Model):
    name = models.CharField(max_length=20)

    def __unicode__(self):
        return self.name

class Person(models.Model):
    name = models.CharField(max_length=20)
    groups = models.ManyToManyField(Group, through='Membership')

    def __unicode__(self):
        return self.name

class Membership(models.Model):
    person = models.ForeignKey(Person)
    group = models.ForeignKey(Group)
    priority = models.IntegerField()

This would be the output:

>>> from myapp.models import Group, Person, Membership
>>> g1 = Group.objects.create(name='beatles')
<Group: beatles>
>>> g2 = Group.objects.create(name='abc')
>>> p1 = Person.objects.create(name='John')
>>> p2 = Person.objects.create(name='Jane')
>>> m1 = Membership.objects.create(person = p1, group = g1, priority=1)
>>> m2 = Membership.objects.create(person = p2, group = g2, priority=2)
>>> p1.groups.order_by('membership__priority')
Traceback (most recent call last):
...
FieldError: Cannot order by many-valued field: 'membership__priority'

Without an intermediary model (through="Membership"), it could happen like this:

>>> Person.objects.filter(id=1).order_by('membership__priority')

The idea is that there are some cases where you're absolutely certain that one side of the many-to-many relationship will resolve to a single record (in this case, p1). In those cases, ordering should be possible on the property in the intermediary table. It will become more of a common use case if/when #6095's functionality is added.

Attachments (1)

hack.diff (933 bytes) - added by floguy 7 years ago.
This does special case checking to see if it's a RelatedObject and if so, doesn't raise a JoinError. This causes one test to break, and that's because I don't understand the implications of the change that was made. Maybe this hack can be expanded upon to get a real solution. (Using patch #6095)

Download all attachments as: .zip

Change History (4)

comment:1 Changed 7 years ago by floguy

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

By the way, I'm looking into implementing this (I'm thinking django.db.models.sql.query.setup_joins), but setup_joins looks like a crazy function and it might take quite a while to wrap my head around it.

Changed 7 years ago by floguy

This does special case checking to see if it's a RelatedObject and if so, doesn't raise a JoinError. This causes one test to break, and that's because I don't understand the implications of the change that was made. Maybe this hack can be expanded upon to get a real solution. (Using patch #6095)

comment:2 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

This isn't the right solution because it's disguising the problem. Ordering across a random one-to-many relation does not, in general, make sense because it's multi-valued and your change removes the error detection for that case.

What is needed is for the query construction code to be able to know for certain that there won't be multiple values returned. This has nothing to do with the presence or absence of the intermediate model (ordering on a many-to-many relation isn't possible because it returns multiple-values). Instead, the intermediate model in this case needs a unique_together designator so the query code can tell that when person and group are specified, there will only be a single Membership instance.

So this ticket is really about adding support for respecting unique_together on models, which we can certainly add.

comment:3 Changed 7 years ago by mtredinnick

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

Fixed in [7285]. It turns out to be logically impossible to always know when this ordering is permissible (see the example in the commit message), so I've removed the safety net.

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