Opened 3 years ago

Closed 16 months ago

Last modified 15 months ago

#19195 closed Bug (fixed)

Using distinct([*fields]) filter on a foreign key produces an ordering error when the foreign key has a Meta ordering field.

Reported by: chrisedgemon@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7-beta-2
Severity: Release blocker Keywords: distinct, query
Cc: charettes 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 (last modified by charettes)

I tried to using a distinct filter like this: Appearance.objects.order_by('team').distinct('team'); this fails with the following Database Error: "DatabaseError: SELECT DISTINCT ON expressions must match initial ORDER BY expressions"

It is possible to work around this problem by using this modified filter: Appearance.objects.order_by('team__id').distinct('team__id').

Model definition: http://pastebin.com/index/J45fy9fr
Full traceback: http://pastebin.com/feSFMbzX

Change History (16)

comment:1 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

The problem is that .distinct('team') can't do distinct on Team._meta.ordering as that will give unexpected results if the Team._meta.ordering isn't unique. In general, the user doesn't want that anyways. On the other hand we can't alter what .order_by('team') does. So, I think we have to disallow doing .distinct('team') if there is ordering defined for the related model.

The error should point to using .distinct('team_id').order_by('team_id') - though this syntax doesn't seem to work at the moment.

comment:2 Changed 3 years ago by chrisedgemon@…

There should actually be two underscores for the team_id filter - order_by fails on team_id.

comment:3 Changed 3 years ago by akaariai

I think we should allow using .order_by('team_id') here.

Related fields have two attributes on model level - team and team_id in this case. We allow using team_id in many places in the ORM already, and to me it seems we should allow it in order_by and distinct, too.

Is there some reason to *not* allow them?

comment:6 Changed 19 months ago by charettes

  • Cc charettes added
  • Description modified (diff)

Just hit this issue and had a hard time figuring out what I've done wrong.

Intuitively I tried .distinct('related_id').order_by('related_id') after realizing removing my Related._meta.ordering solved the issue but, as pointed out by akaariai, this is not allowed ATM.

Replying to akaariai:

I think we should allow using .order_by('team_id') here.

Related fields have two attributes on model level - team and team_id in this case. We allow using team_id in many places in the ORM already, and to me it seems we should allow it in order_by and distinct, too.

Is there some reason to *not* allow them?

I can't think of any reason we'd like *not* to allow them. It looks like sanest to expose an API to explicitly opt-out of the existing related model ordering behavior while maintaining backward compatibility.

comment:7 Changed 17 months ago by charettes

Calling order_by('team_id') doesn't raise a FieldError anymore on Django 1.7 and silently behave just like order_by('team').

If we want to avoid breaking code that might rely on this in 1.7 we should fix it now. Else we'll have to deal with this by introducing an entry point to order_by in order to opt-out of this unexpected behavior.

I wrote a patch for the order_by issue here.

comment:8 Changed 16 months ago by akaariai

Yes, I think we should do this for 1.7. Some review comments:

  • The actual code changes look correct
  • Release notes and some documentation changes for order_by() needed

The test changes looked a bit scary to me (do all existing tests continue to test what they originally tested?), but after a bit more reading I think they are safe. A final check here wouldn't hurt.

comment:9 Changed 16 months ago by timo

  • Severity changed from Normal to Release blocker
  • Version changed from 1.4 to 1.7-beta-2

Setting release blocker flag per previous comment.

comment:10 Changed 16 months ago by charettes

  • Has patch set

Create a PR with doc and release note. I would appreciate a wording review.

In order to make sure the test refactoring didn't break anything I review and moved every cases in their own methods. I you feel like this is adding too much noise to the patch I can remove this part from the final patch.

comment:11 Changed 16 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

Left some comments for doc improvements.

comment:12 Changed 16 months ago by Simon Charette <charette.s@…>

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

In 24ec9538b7ca400f68ba08fab380445ca95d7c02:

Fixed #19195 -- Allow explicit ordering by a relation _id field.

Thanks to chrisedgemon for the report and shaib, akaariai and
timgraham for the review.

comment:13 Changed 16 months ago by Simon Charette <charette.s@…>

In a6ecd5dbb34249f756a337c359eef1e8d78dc01e:

[1.7.x] Fixed #19195 -- Allow explicit ordering by a relation _id field.

Thanks to chrisedgemon for the report and shaib, akaariai and
timgraham for the review.

Backport of 24ec9538b7 from master

comment:14 Changed 15 months ago by Althalus

I've found an issue with Django 1.7 checks framework and this ticket.
Now we're allowed to order by fieldname as before and by fieldname_id (without joins).
But django/db/models/base.py _check_ordering method still does not allow to set fieldname_id as one of Meta.ordering values.
I've made a patch here: https://github.com/Vincent-Vega/django/commit/7d94a3d822746f2815e7184928311e1d91dff467

I'm quite new to contributing (and also django faq says that pull requests without ticket are not accepted on github).
So it would be nice if you explain what can I do next.

comment:15 Changed 15 months ago by charettes

Hi Althalus, you should open a ticket for this and refer to it from your pull request. This looks like a legitimate bug.

comment:16 Changed 15 months ago by Althalus

I think it's worth noting here that I've created #22711.

comment:17 Changed 15 months ago by Simon Charette <charette.s@…>

In d04e7302240f5be34cdd303002bc8e7dcd81f529:

Fixed #22711 -- Adjusted ordering checks to allow implicit relation fields.

refs #19195.

comment:18 Changed 15 months ago by Simon Charette <charette.s@…>

In d773a08b270e3b2c387985a9d9a4d01c991469c8:

[1.7.x] Fixed #22711 -- Adjusted ordering checks to allow implicit relation fields.

refs #19195.

Backport of d04e730224 from master

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