#14930 closed Bug (fixed)
`values_list()` fails on queryset ordered by extra column
Reported by: | Luc Saffre | Owned by: | fhahn |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | robinchew@…, simon@…, gosia, lau@…, Ludovic Delaveau, django@…, fhahn, timograham@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
values_list()
returns an uncomplete QuerySet when called on a
QuerySet that is ordered by a column defined using QuerySet.extra()
.
For example, the following QuerySet works as expected::
qs = Article.objects.extra(select={'id_plus_one': 'id+1'},order_by=['id_plus_one'])
but when I execute a values_list('id')
on this, I'll get a
FieldError: Cannot resolve keyword 'id_plus_one' into field
.
To show this problem, I added
a test case to django/tests/modeltests/lookup/tests.py
:
# See http://lino.saffre-rumma.net/tickets/19.html qs = Article.objects.extra(select={'id_plus_one': 'id+1'}).order_by('id_plus_one') print qs.query.extra_select # output: {'id_plus_one': (u'id+1', [])} self.assertQuerysetEqual(qs, [ self.a1, self.a2, self.a3, self.a4, self.a5, self.a6, self.a7 ], transform=identity) qs = qs.values_list('id') print qs.query.extra_select # output: {} self.assertQuerysetEqual( qs, [ [self.a1.id], [self.a2.id], [self.a3.id], [self.a4.id], [self.a5.id], [self.a6.id], [self.a7.id] ], transform=identity)
Here is a diff file for this code against Django 14995.
Running the lookup tests with this patch will say::
ERROR: test_values_list (modeltests.lookup.tests.LookupTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "L:\snapshots\django\tests\modeltests\lookup\tests.py", line 347, in test_values_list transform=identity) File "l:\snapshots\django\django\test\testcases.py", line 526, in assertQuerysetEqual return self.assertEqual(map(transform, qs), values) File "l:\snapshots\django\django\db\models\query.py", line 84, in __len__ self._result_cache.extend(list(self._iter)) File "l:\snapshots\django\django\db\models\query.py", line 953, in iterator for row in self.query.get_compiler(self.db).results_iter(): File "l:\snapshots\django\django\db\models\sql\compiler.py", line 680, in results_iter for rows in self.execute_sql(MULTI): File "l:\snapshots\django\django\db\models\sql\compiler.py", line 725, in execute_sql sql, params = self.as_sql() File "l:\snapshots\django\django\db\models\sql\compiler.py", line 60, in as_sql ordering, ordering_group_by = self.get_ordering() File "l:\snapshots\django\django\db\models\sql\compiler.py", line 349, in get_ordering self.query.model._meta, default_order=asc): File "l:\snapshots\django\django\db\models\sql\compiler.py", line 378, in find_ordering_name opts, alias, False) File "l:\snapshots\django\django\db\models\sql\query.py", line 1215, in setup_joins "Choices are: %s" % (name, ", ".join(names))) FieldError: Cannot resolve keyword 'id_plus_one' into field. Choices are: author, headline, id, pub_date, tag
AFAICS qs.query.extra_select
on the ValuesListQuerySet
should *not* be empty.
Attachments (6)
Change History (40)
comment:1 by , 14 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
comment:2 by , 14 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Triage Stage: | Unreviewed → Accepted |
After a follow up by richard.anderson@… on django-dev:
It looks like my triage was mistaken. This isn't the Python iterator exception bug; it's a Django problem.
It should be possible to *order* by a field that isn't included in the result set. The logic constructing ValuesQuerySet appears to be truncating the extra columns, rather than adding them but applying a mask.
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:4 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:5 by , 13 years ago
Cc: | added |
---|---|
Has patch: | set |
Needs tests: | set |
Owner: | changed from | to
Status: | reopened → new |
comment:6 by , 13 years ago
Status: | new → assigned |
---|
by , 13 years ago
Attachment: | extra_order_by_values_list-17428.2.diff added |
---|
comment:8 by , 13 years ago
Easy pickings: | set |
---|
comment:9 by , 13 years ago
Cc: | added |
---|
comment:12 by , 12 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Patch needs improvement: | set |
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.
comment:13 by , 12 years ago
Actually it does not look as a bug to me.
If you refer to the documentation of the 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".
comment:14 by , 12 years ago
But adding order_by as a keyword argument to extra doesn't work either.
comment:15 by , 12 years ago
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).
comment:16 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Yes you're right, I just tested without the patch and the tests I wrote failed. With the patch and the order_by
argument inside the extra
instead of as an additional call, the test for values_list
passes.
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 and improve the patch, it seems related to the order_by
argument.
comment:17 by , 12 years ago
Status: | new → assigned |
---|
comment:18 by , 12 years ago
Patch needs improvement: | unset |
---|
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.
comment:19 by , 12 years ago
Cc: | added |
---|
Can the patch be merged? I just hit this problem, too, and the patch fixes it.
comment:20 by , 12 years ago
Can you mark as ready for checkin ? I will do a pull request after your confirmation.
comment:21 by , 12 years ago
Sorry, I forgot to log in.
Can you mark as ready for checkin ? I will do a pull request after your confirmation.
comment:22 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:23 by , 12 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Unfortunately the patch isn't ready for checkin. The extra_regress tests do not pass.
by , 12 years ago
Attachment: | extra_order_by_values_list_with_tests_3.diff added |
---|
updated version of the patch, with tests
comment:24 by , 12 years ago
I've created a branch on Github as well: https://github.com/fhahn/django/tree/ticket_14930
comment:25 by , 12 years ago
Cc: | added |
---|
by , 12 years ago
Attachment: | extra_order_by_values_list_with_tests_4.diff added |
---|
made patch python 2.6 compatible
comment:26 by , 12 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
I've created a pull request as well: https://github.com/django/django/pull/713
comment:27 by , 12 years ago
Cc: | added |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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.
comment:28 by , 12 years ago
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...
comment:29 by , 12 years ago
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.
comment:30 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:31 by , 11 years ago
Hello, will this fix also be integrated into the stable branches 1.4 and 1.5?
Best regards
comment:32 by , 11 years ago
No, per our support policy those branches are only receiving security fixes and critical (data loss) bug fixes respectively.
comment:33 by , 10 years ago
So what we (v1.4/1.5 users) can do, if we can't upgrade Django to 1.7 because of broken compatibility?
comment:34 by , 10 years ago
- If there is a bug in 1.7 that's preventing you from upgrading, please file a ticket.
- If your application needs to be updated to be compatible with 1.7, update it.
- 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.
The query isn't empty (strictly) -- it has raised an error. Unfortunately, due to a bug in Python that exists in 2.6.1 (which is unfortunately the default version on Snow Leopard), the exception is eaten silently.
If you try and print the SQL for the second queryset (the one with the id selected), you will raise an error, because 'id_plus_one' is no longer a selected column.
See #14766 for a similar bug that is hidden by this buggy version of Python. I'm going to say the resolution here is the same -- update your version of Python.