Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#32811 closed Bug (invalid)

Annotate removes Meta.ordering

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

Description

This is very simplified example

In [5]: str(Order.objects.values("id", "ordered_at").all().query)
Out[5]: 'SELECT "orders_order"."id", "orders_order"."ordered_at" FROM "orders_order" ORDER BY "orders_order"."ordered_at" DESC'

In [6]: str(Order.objects.values("id", "ordered_at").annotate(c=Count("id")).all().query)
Out[6]: 'SELECT "orders_order"."id", "orders_order"."ordered_at", COUNT("orders_order"."id") AS "c" FROM "orders_order" GROUP BY "orders_order"."id"'

As you can see ORDER BY disappears when annotate with Count exists.
It used to work on 3.0, but stopped working in 3.1 and 3.2

Change History (3)

comment:1 by Mariusz Felisiak, 3 years ago

Easy pickings: unset
Resolution: invalid
Status: newclosed

This is an expected behavior. Meta.ordering doesn't affect GROUP BY queries after 0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6 (which was deprecated in 1b1f64ee5a78cc217fead52cbae23114502cf564).

comment:2 by Peter Law, 2 years ago

Request that this be reconsidered.

My use-case was similar to the one mentioned here, however I was loading full models (with a similar count annotation) and not any form of .values(). Given the history it seems likely that that was never the intention.

I can see that this was previously deprecated and may now be expected, however it does not appear to be documented anywhere and is very surprising.

Here's where I've (since) checked for docs relating to this:

What I've seen of the reasoning for the deprecation appears to be related specifically to .values() queries, and then really to cases where that is used to create a grouping of the main queried table (where the user is probably aware that that's what they're doing). However the behaviour change impacts all querysets (including those which return full models) and the behaviour here is not intending to create a grouping of things on the main table, but rather of a related table for the purposes of counting (where the user is unlikely to be aware that a group-by is the result).

Perhaps the core issue here is that the ORM doesn't have an explicit "group by" mechanism and so is having to infer this by pattern matching the structure of the QuerySet, leading to the issues like the original one where default ordering is applied in ways that produce potentially unexpected results.

At the very least I think this (current behaviour) needs to be clearly documented as a part of both the Meta.ordering and aggregation docs, however to be honest I would prefer the previous behaviour. It feels very surprising that Django will silently drop part of its model configuration in some cases!
At least if this were documented people could see what those cases were, however at the moment the only way to know is when you're bitten by the issue described here.

Aside: the original ticket (https://code.djangoproject.com/ticket/10574) appears to suggest that part of the motivation is that in queries such as Model.objects.values('data') applying the default ordering is redundant, which I'd strongly argue against -- having values-only results ordered by something (even a field not loaded) seems entirely useful to me.

in reply to:  2 comment:3 by Mariusz Felisiak, 2 years ago

Replying to Peter Law:

Request that this be reconsidered.

...
At the very least I think this (current behaviour) needs to be clearly documented as a part of both the Meta.ordering and aggregation docs, however to be honest I would prefer the previous behaviour. It feels very surprising that Django will silently drop part of its model configuration in some cases!
At least if this were documented people could see what those cases were, however at the moment the only way to know is when you're bitten by the issue described here.

The previous behavior was implicit, unexpected, and caused many support questions. We reached the consensus that removing any default ordering for grouped queries is the right thing to do. You can always add order_by() (as documented).

You can start a discussion on DevelopersMailingList if you don't agree, but I don't think there would be consensus to restore it. Personally, I'm strongly against it.

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