[Django] #30628: Order by with union can result in invalid ORDER BY clause
Django
-------------------------------------+-------------------------------------
Reporter: Julien | Owner: nobody
Enselme |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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 `DecimalField`s 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
[https://github.com/django/django/commit/bc7e288ca9554ac1a0a19941302dea19df1acd21
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`).
[https://github.com/Jenselme/dj-test-unions Sample projet to reproduce]
(sqlite db included) ([https://github.com/Jenselme/dj-test-
unions/blob/master/union/models.py Model], [https://github.com/Jenselme
/dj-test-unions/blob/master/test-script.py 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)`).
--
Ticket URL: <https://code.djangoproject.com/ticket/30628>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Thanks for the report!
Regression in bc7e288ca9554ac1a0a19941302dea19df1acd21.
Reproduced at bc91f27a86090b4c688b56cd4e37f95eebe6e969.
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:1>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:2>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Simon Charette
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:3>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/11550
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:4>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Component: Database layer | Version: 2.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ee6e93ec8727d0f5ed33190a3c354867669ed72f" ee6e93e]:
{{{
#!CommitTicketReference repository=""
revision="ee6e93ec8727d0f5ed33190a3c354867669ed72f"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:5>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9dee8515d6f2876fa039aaebdfe8e2bc9de63085" 9dee8515]:
{{{
#!CommitTicketReference repository=""
revision="9dee8515d6f2876fa039aaebdfe8e2bc9de63085"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:6>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Julien Enselme):
Thanks for the quick fix!
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:7>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
{{{#!CommitTicketReference repository=""
revision="df2c4952df6d93c575fb8a3c853dc9d4c2449f36"
Fixed #36173 -- Stabilized identity of Concat with an explicit
output_field.
When Expression.__init__() overrides make use of *args, **kwargs
captures their argument values are respectively bound as a tuple and
dict instances. These composite values might themselves contain values
that require special identity treatments such as Concat(output_field)
as it's a Field instance.
Refs #30628 which introduced bound Field differentiation but lacked
argument captures handling.
Thanks erchenstein for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:8>
Django
field type is presented multiple times.
-------------------------------------+-------------------------------------
Reporter: Julien Enselme | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
{{{#!CommitTicketReference repository=""
revision="8b1e324ca4aa1ae0721f6f5dcfba8325a751ef3c"
[5.2.x] Fixed #36173 -- Stabilized identity of Concat with an explicit
When Expression.__init__() overrides make use of *args, **kwargs
captures their argument values are respectively bound as a tuple and
dict instances. These composite values might themselves contain values
that require special identity treatments such as Concat(output_field)
as it's a Field instance.
Refs #30628 which introduced bound Field differentiation but lacked
argument captures handling.
Thanks erchenstein for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30628#comment:9>