Opened 12 years ago

Closed 21 months ago

Last modified 11 months ago

#14357 closed Bug (fixed)

Prevent inappropriate order-based grouping on values+annotate queries

Reported by: Martin Chase Owned by:
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords: 3.1
Cc: Simon Charette, Mariusz Felisiak Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

class Foo(models.Model):
    name = models.TextField()
    type = models.TextField()

    class Meta:
        ordering = ["name",]

Foo.objects.all().values("type").annotate(models.Count("name"))

The above code fails to generate the expected result without adding .order_by() to the query set.

Ticket #10574 called this fixed with documentation, but indicated a proper fix would be preferred once backwards compatibility wasn't an issue. Lest this fall off the radar for being closed, I am putting this ticket in for 2.0.

Attachments (1)

unordering.patch (813 bytes) - added by Martin Chase 12 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by Martin Chase

possible fallback orderings:

  • pk
  • [the ordering fields] & [the fields referenced in the .values]

comment:2 Changed 12 years ago by Martin Chase

(that was a set & operator. sorry for the confusing notation.)

possible fallback orderings:

  • pk
  • only those fields which are in both the ordering and the values
  • all fields in the ordering which are not also in an annotate

Changed 12 years ago by Martin Chase

Attachment: unordering.patch added

comment:3 Changed 12 years ago by Martin Chase

attached unordering.patch is a poor attempt at changing the ordering.

comment:4 Changed 12 years ago by Russell Keith-Magee

Has patch: set
milestone: 2.0
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:5 Changed 12 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:6 Changed 11 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 11 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 Changed 10 years ago by Anssi Kääriäinen

Component: ORM aggregationDatabase layer (models, ORM)

comment:9 Changed 6 years ago by Tim Graham

Cc: Simon Charette added

Simon, do you have any opinions about this change given we abandoned the idea of "Django 2.0" with large backwards incompatible changes? Not sure if a deprecation path to change the behavior is feasible.

comment:10 Changed 6 years ago by Simon Charette

I think we could and should make this change as it really common pitfall but it should follow a deprecation path.

I've personally stopped using ordering because of its unexpected interaction with distinct() and values() for grouping by.

The deprecation path should detect when values() is used for grouping on a queryset where the only defined ordering is from _meta.ordering in the SQL compiling step and raise a deprecation warning saying such ordering will be ignored in future version of Django.

comment:11 Changed 6 years ago by Simon Charette

I think we should go as far as requiring all ordering clauses to be explicitly specified through values() when used to group by.

For example, in the following case

Foo.objects.order_by('name').values('type').annotate(Count('name'))

A deprecation warning would be raised to notify the user that 'name' won't automatically be added to values() in future Django version where the previous queryset would be equivalent to the following:

Foo.objects.values('type').annotate(Count('name'))

and the current behaviour could be preserved by adding 'name' to values():

Foo.objects.order_by('name').values('type', 'name').annotate(Count('name'))

I haven't tested it myself to see if it's already the case but we should also disallow ordering by a non-grouped column or aggregate alias.

Foo.objects.values('type').annotate(Count('name')).order_by('name') should raise a value error on the order_by('name') call.

comment:12 Changed 5 years ago by Mariusz Felisiak

I totally agree with Simon. In my opinion we shouldn't automatically add model.Meta.ordering to values(), values_list() and distinct(). I remember that a few years ago I lost a lot of time when I tried to figure out why the result wasn't what I expected.

Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:13 Changed 5 years ago by Mariusz Felisiak

Cc: Mariusz Felisiak added

comment:14 Changed 5 years ago by Tim Graham

I started a PR. It's not complete or ready for review yet.

comment:16 Changed 5 years ago by Tim Graham

Summary: Prevent innapropriate order-based grouping on values+annotate queriesPrevent inappropriate order-based grouping on values+annotate queries

comment:17 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

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

In 1b1f64ee:

Refs #14357 -- Deprecated Meta.ordering affecting GROUP BY queries.

Thanks Ramiro Morales for contributing to the patch.

comment:19 Changed 4 years ago by Carlton Gibson

Has patch: unset

I'm unchecking Has patch, since the PR (deprecating ordering being used for grouped queries was merged). What remains to close this ticket?

comment:20 Changed 4 years ago by Tim Graham

Keywords: 3.1 added
Triage Stage: AcceptedSomeday/Maybe

The behavior change will happen in Django 3.1 when the deprecation ends.

comment:21 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 0ddb4ebf:

Refs #14357 -- Made Meta.ordering not affect GROUP BY queries.

Per deprecation timeline.

comment:22 Changed 3 years ago by Mariusz Felisiak

Resolution: fixed
Status: newclosed

comment:23 Changed 21 months ago by Yuri Konotopov

Resolution: fixed
Status: closednew

I beleave this was not properly fixed in commit 0ddb4ebf.
While commit 0ddb4ebf removes ORDER BY when Meta.ordering is used it still does populates GROUP BY with Meta.ordering fields thus leads to wrong aggregation.

Look to get_group_by() at compiler.py [1] - it populates GROUP BY using previously obtained order_by with Meta.ordering fields [2][3].
For what I see we must filter off Meta.ordering fields from order_by in get_group_by() function.

[1] https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L128
[2] https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L56
[3] https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L286

comment:24 Changed 21 months ago by Mariusz Felisiak

Resolution: fixed
Status: newclosed

Please don't reopen closed tickets. You can create a new ticket if you can provide a sample project or test and prove that Meta.ordering is still included.

comment:25 Changed 18 months ago by GitHub <noreply@…>

In fcd44b88:

Refs #14357 -- Updated docs about interaction between aggregations and QuerySet.order_by().

Obsolete since 0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6.

comment:26 Changed 18 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 9b096063:

[3.2.x] Refs #14357 -- Updated docs about interaction between aggregations and QuerySet.order_by().

Obsolete since 0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6.
Backport of fcd44b889f36c4be87910745614a0a4c88d7a3d8 from main

comment:27 Changed 11 months ago by Adam Sołtysik

For anyone surprised, just like me, that Meta.ordering still breaks aggregation queries in Django 3.1 and 3.2, despite what the documentation says here and here, it seems that this was finally fixed in #32546 in Django 4.0. Possibly the documentation should be updated to reflect the actual version when the change was implemented, as there seems to be nothing about it in the release notes for 4.0. Or maybe the change should be backported to reflect the documentation.

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