Re: [Django] #14930: `values_list()` fails on queryset ordered by extra column
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: nobody
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: SVN | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by robin):
* cc: robinchew@… (added)
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by simon29):
* owner: nobody => simon29
* status: reopened => new
* has_patch: 0 => 1
* cc: simon@… (added)
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by simon29):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by simon29):
Use extra_order_by_values_list-17428.diff (not the -2 version)
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by simon29):
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: gosia (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: lau@… (added)
Comment:
Just hit this problem. Why hasn't the patch been pulled in?
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
The patch doesn't contain tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by vicould):
* cc: vicould (added)
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
Added two tests to the patch: one for values and one for values_list, as
the problem have the same cause.
The original patch by simon29 does not seem to fix the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by vicould):
Actually it does not look as a bug to me.
If you refer to the documentation of the
[https://docs.djangoproject.com/en/1.4/ref/models/querysets/#extra extra
method], it is specified ''"If you need to order the resulting queryset
using some of the new fields or tables you have included via extra() use
the order_by parameter to extra() and pass in a sequence of strings".''
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by lau):
But adding order_by as a keyword argument to extra doesn't work either.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: simon29
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by simon29):
From memory, the patch fixes the functionality (I used it in production);
but by the looks, doesn't fully update the internals (which is what the
test is testing on).
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:15>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* owner: simon29 => vicould
* status: assigned => new
Comment:
Yes you're right, I just tested without the patch and the tests I wrote
failed. But with the patch, and the {{{order_by}}} argument inside the
{{{extra}}} instead of as an additional call the test for
{{{values_list}}} pass. However, the test with the {{{values}}} fails, as
the additional ordering column is inserted in the list.
Here is the output of the test:
{{{
First differing element 0:
{u'num': 72, u'value_plus_one': 73}
{u'num': 72}
}}}
I tested with two extra columns, and only the one on which the results are
sorted is inserted in the values list. I will try to find the cause, but
it seems related to the {{{order_by}}} argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by vicould):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by vicould):
* needs_better_patch: 1 => 0
Comment:
Alright, this one should be good. I believe I fixed the issues, both
{{{order_by}}} on a QuerySet and {{{order_by}}} in {{{extra}}} directives
work.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: django@… (added)
Comment:
Can the patch be merged? I just hit this problem, too, and the patch fixes
it.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Can you mark as ready for checkin ? I will do a pull request after your
confirmation.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by vicould):
Sorry, I forgot to log in.
Can you mark as ready for checkin ? I will do a pull request after your
confirmation.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mkai):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Unfortunately the patch isn't ready for checkin. The extra_regress tests
do not pass.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by fhahn):
I've created a branch on Github as well:
https://github.com/fhahn/django/tree/ticket_14930
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: vicould
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: fhahn (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:25>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by fhahn):
* owner: vicould => fhahn
* needs_better_patch: 1 => 0
Comment:
I've created a pull request as well:
https://github.com/django/django/pull/713
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: timograham@… (added)
* stage: Accepted => Ready for checkin
Comment:
I've updated the patch with some cosmetic tweaks as well as to merge
cleanly to master (confirmed all tests pass as well). Would like someone
more familiar with the code to +1 before it gets committed.
https://github.com/django/django/pull/1242
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
I am not sure what the code in L1002-L1004 in models/query.py is doing.
The code might be OK, but why the code doesn't do: "if extra_names_mask"
instead of "if self.extra_names is not None"? If the self.extra_names is
not None check is correct, then the construction of extra_names_mask could
be moved inside the if branch. Also, could same fields end up in the mask
twice if they are both in self.extra_names and in the order by fields.
Maybe that is OK.
In addition it seems there are some slight optimization possibilities in
doing some more work outside the innermost loop. Likely not too
important...
Still, it seems this is a step forward already, so I guess commit is OK.
Of course, if the above issues can be dealt before commit then even
better...
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by fhahn):
Thanks for the feedback, I've updated the patch. I've pushed the changes
to my own branch at the moment
(https://github.com/fhahn/django/commit/025ad76b97b90fc45eae8a18dfb4ad58c3dcbe58),
because I wasn't sure if I should open another pull request for the
django/django repo.
The self.extra_names is not None check was there before and something (not
related to the test cases added by the patch) breaks if the condition is
replaced by self.extra_names. I'm not sure where exactly, but I could have
a closer look next week or so.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2f35c6f10fcbae541691207fb0c0560a13b754fc"]:
{{{
#!CommitTicketReference repository=""
revision="2f35c6f10fcbae541691207fb0c0560a13b754fc"
Fixed #14930 -- values_list() failure on qs ordered by extra column
Thanks lsaffre for the report and simon29, vicould, and Florian Hahn
for the patch.
Some changes done by committer.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by knoffhoff):
Hello, will this fix also be integrated into the stable branches 1.4 and
1.5?
Best regards
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
No, per our [https://docs.djangoproject.com/en/dev/internals/security
/#supported-versions support policy] those branches are only receiving
security fixes and critical (data loss) bug fixes respectively.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anonymous):
So what we (v1.4/1.5 users) can do, if we can't upgrade Django to 1.7
because of broken compatibility?
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: lsaffre | Owner: fhahn
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
1. If there is a bug in 1.7 that's preventing you from upgrading, please
file a ticket.
2. If your application needs to be updated to be compatible with 1.7,
update it.
3. If you don't want to update your application to be compatible with a
newer release of Django and want this fix, maintain your own fork of
Django with the fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/14930#comment:34>