Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#30628 closed Bug (fixed)

order_by() on union() querysets results with wrong ordering when the same field type is presented multiple times.

Reported by: Julien Enselme Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 2.2
Severity: Release blocker Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When doing a union of 2 querysets and then doing an order_by on the resulting queryset, if we order on a field whose type is present multiple time, the ordering will be incorrect if the field we sort on is not the 1st field of the query. Explicitly settings values with .values('field1', 'field2') on the base querysets or after the union has no effect.

For instance, with this model, with 2 DecimalFields and one BooleanField:

class Listing(models.Model):
    sale_price = models.DecimalField('Sale price', max_digits=10, decimal_places=2)
    yearly_rent = models.DecimalField('Yearly rent', max_digits=10, decimal_places=2)
    toto = models.BooleanField()
# Create 2 qs.
qs1 = Listing.objects.all()
qs2 = Listing.objects.all()

# Create the union QS.
qs3 = qs1.union(qs2)

# Order on the 1st decimal field. This prints (which is correct) :
# SELECT "union_listing"."id", "union_listing"."sale_price", "union_listing"."yearly_rent" FROM "union_listing" UNION SELECT "union_listing"."id", "union_listing"."sale_price", "union_listing"."yearly_rent" FROM "union_listing" ORDER BY (2) ASC
print(qs3.order_by('sale_price').query)
# Order on the 2nd deciamal field. This will print the same query as above which is incorrect.
print(qs3.order_by('yearly_rent').query)
# Not ordering on a DecimalField. This is correct again.
print(qs3.order_by('toto').query)

From the debugging I did, it seems to come from this commit: If I revert def __eq__ back to what it was in Django 2.1 (https://github.com/django/django/blob/stable/2.1.x/django/db/models/expressions.py#L363) it works as normal again. The difference between the two methods that can explain this is that in Django 2.1, we have a check on the actual field instances thanks to other_args[1], but in 2.2, because of identity[2][1] which is the class of the field, we can't distinguish two fields of the same type (please refer to the respective implementations to know the values of other_args and identity).

Sample projet to reproduce (sqlite db included) (Model, test file). Steps:

  1. Install Django 2.2
  2. Run DJANGO_SETTINGS_MODULE=testunion.settings python test-script.py
  3. You will see the queries for qs3.order_by('sale_price') and qs3.order_by('yearly_rent') They are exactly the same whereas they should be different (one with ORDER BY (1) and the other with ORDER BY (2)).

Change History (7)

comment:1 Changed 15 months ago by felixxm

Cc: Simon Charette added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the report!

Regression in bc7e288ca9554ac1a0a19941302dea19df1acd21.
Reproduced at bc91f27a86090b4c688b56cd4e37f95eebe6e969.

comment:2 Changed 15 months ago by felixxm

Summary: Order by with union can result in invalid ORDER BY clauseorder_by() on union() querysets results with wrong ordering when the same field type is presented multiple times.

comment:3 Changed 15 months ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:5 Changed 15 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In ee6e93e:

Fixed #30628 -- Adjusted expression identity to differentiate bound fields.

Expressions referring to different bound fields should not be
considered equal.

Thanks Julien Enselme for the detailed report.

Regression in bc7e288ca9554ac1a0a19941302dea19df1acd21.

comment:6 Changed 15 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 9dee8515:

[2.2.x] Fixed #30628 -- Adjusted expression identity to differentiate bound fields.

Expressions referring to different bound fields should not be
considered equal.

Thanks Julien Enselme for the detailed report.

Regression in bc7e288ca9554ac1a0a19941302dea19df1acd21.

Backport of ee6e93ec8727d0f5ed33190a3c354867669ed72f from master

comment:7 Changed 15 months ago by Julien Enselme

Thanks for the quick fix!

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