Opened 6 years ago

Last modified 2 months ago

#14357 new Bug

Prevent innapropriate order-based grouping on values+annotate queries

Reported by: Martin Chase Owned by:
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by Martin Chase

possible fallback orderings:

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

comment:2 Changed 6 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 6 years ago by Martin Chase

Attachment: unordering.patch added

comment:3 Changed 6 years ago by Martin Chase

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

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

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

comment:5 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:6 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

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

Component: ORM aggregationDatabase layer (models, ORM)

comment:9 Changed 2 months 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 2 months 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 2 months 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.

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