[Django] #29694: QuerySet.values_list() combined with .extra() or .annotate() may produce wrong .union()
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Release | Keywords: union values_list
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is a revival of #29229, as a regression introduced in 2.0.5 and
1.11.13 by #29286.
Please refer to the demo code in the description of #29229, still
valuable.
--
Ticket URL: <https://code.djangoproject.com/ticket/29694>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: duplicate
Keywords: union values_list | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
Please don't deliberately open duplicate tickets.
If there is still an issue on #29229, please comment there.
See the test case introduced in a0c03c62a8ac586e5be5b21393c925afa581efaf.
Can you adjust this to demonstrate the issue you are still seeing? If so,
we can re-open #29229 and address this.
(Either way we need **some** information to go on as to how to replicate
the issue you are seeing. As far as we were aware the issue in the
original description was already resolved...)
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:1>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: duplicate =>
* stage: Unreviewed => Accepted
Old description:
> This is a revival of #29229, as a regression introduced in 2.0.5 and
> 1.11.13 by #29286.
> Please refer to the demo code in the description of #29229, still
> valuable.
New description:
This is a revival of #29229, as a regression introduced in 2.0.5 and
1.11.13 by #29286.
Please refer to the demo code in the description of #29229.
--
Comment:
Since a fix for that ticket has been released, it's appropriate to open a
new ticket. It seems that the test that was added in the patch doesn't
precisely reflect the steps to reproduce.
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:2>
Django
wrong .union()
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: union values_list | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by master):
(I wasn't sure and I don't mind to reopen a previous ticket, but my
reasoning to open a new ticket was exactly the one of Tim).
So why didn't the test case (test_union_with_two_annotated_values_list)
fail after the changes of #29286? Because it is written with annotate()
instead of extra(). To reproduce the steps of the demo code of #29229,
replace:
{{{
#!python
qs1 = Number.objects.filter(num=1).annotate(count=Value(0,
IntegerField()),).values_list('num', 'count')
}}}
by:
{{{
#!python
qs1 = Number.objects.filter(num=1).extra(select={'count':
0}).values_list('num', 'count')
}}}
Note: I just discovered that the usage of extra() is discouraged, and I
will consider a replacement, but not as a priority.
My suggestion for a fix in
django\db\models\sql\compiler.py\get_combinator_sql(), to work both with
extra and annotate, is something of this kind:
{{{
#!python
if (not compiler.query.values_select and not compiler.query.annotations
and (self.query.values_select or self.query.annotation_select or
self.query.extra_select)):
compiler.query.set_values(tuple(self.query.values_select) +
tuple(self.query.annotation_select) +
tuple(self.query.extra_select)
)
}}}
I'm not sure if we need the additional condition "and not
compiler.query.<extra>", as I don't know what <extra> would be amongst:
"extra", "extra_select" and "_extra".
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Keywords: union values_list | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Release blocker => Normal
Comment:
Since this was a regression, I'm open to fixing it on master and perhaps
stable/2.1.x, however, be aware that we're no longer fixing bugs with
`QuerySet.extra()` per [https://groups.google.com/d/topic/django-
developers/FojuU0syO8Y/discussion discussion on django-developers].
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: union values_list | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => felixxm
* version: 1.11 => 2.1
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: union values_list | 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/10335 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"39461a83c33f0cfe719d3b139413d1f5d1e75d5e" 39461a8]:
{{{
#!CommitTicketReference repository=""
revision="39461a83c33f0cfe719d3b139413d1f5d1e75d5e"
Fixed #29694 -- Fixed column mismatch crash with QuerySet.values() or
values_list() after combining querysets with extra() with union(),
difference(), or intersection().
Regression in 0b66c3b442875627fa6daef4ac1e90900d74290b.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: master | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: union values_list | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e7acd99113dfedee1bfcdadd2f81db96ab01e95d" e7acd99]:
{{{
#!CommitTicketReference repository=""
revision="e7acd99113dfedee1bfcdadd2f81db96ab01e95d"
[2.1.x] Fixed #29694 -- Fixed column mismatch crash with QuerySet.values()
or values_list() after combining querysets with extra() with union(),
difference(), or intersection().
Regression in 0b66c3b442875627fa6daef4ac1e90900d74290b.
Backport of 39461a83c33f0cfe719d3b139413d1f5d1e75d5e from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29694#comment:9>