Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#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)

extra_order_by_values_list-17428.2.diff (1.3 KB ) - added by Simon Litchfield 12 years ago.
extra_order_by_values_list-17428.diff (1.3 KB ) - added by Simon Litchfield 12 years ago.
Fix- against r17428
extra_order_by_values_list-with_test.diff (2.4 KB ) - added by Ludovic Delaveau 11 years ago.
Added test to patch
extra_order_by_values_list-with_test-2.diff (3.7 KB ) - added by Ludovic Delaveau 11 years ago.
Improved patch
extra_order_by_values_list_with_tests_3.diff (5.5 KB ) - added by fhahn 11 years ago.
updated version of the patch, with tests
extra_order_by_values_list_with_tests_4.diff (5.5 KB ) - added by fhahn 11 years ago.
made patch python 2.6 compatible

Download all attachments as: .zip

Change History (40)

comment:1 by Russell Keith-Magee, 13 years ago

Component: UncategorizedDatabase layer (models, ORM)
Resolution: wontfix
Status: newclosed

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.

comment:2 by Russell Keith-Magee, 13 years ago

Resolution: wontfix
Status: closedreopened
Triage Stage: UnreviewedAccepted

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 anonymous, 13 years ago

Severity: Normal
Type: Bug

comment:4 by Robin, 13 years ago

Cc: robinchew@… added
Easy pickings: unset
UI/UX: unset

comment:5 by Simon Litchfield, 12 years ago

Cc: simon@… added
Has patch: set
Needs tests: set
Owner: changed from nobody to Simon Litchfield
Status: reopenednew

comment:6 by Simon Litchfield, 12 years ago

Status: newassigned

by Simon Litchfield, 12 years ago

by Simon Litchfield, 12 years ago

Fix- against r17428

comment:7 by Simon Litchfield, 12 years ago

Use extra_order_by_values_list-17428.diff (not the -2 version)

comment:8 by Simon Litchfield, 12 years ago

Easy pickings: set

comment:9 by gosia, 12 years ago

Cc: gosia added

comment:10 by Lau Bech Lauritzen, 12 years ago

Cc: lau@… added

Just hit this problem. Why hasn't the patch been pulled in?

comment:11 by Anssi Kääriäinen, 12 years ago

The patch doesn't contain tests.

comment:12 by Ludovic Delaveau, 11 years ago

Cc: Ludovic Delaveau 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.

by Ludovic Delaveau, 11 years ago

Added test to patch

comment:13 by Ludovic Delaveau, 11 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 Lau Bech Lauritzen, 11 years ago

But adding order_by as a keyword argument to extra doesn't work either.

comment:15 by Simon Litchfield, 11 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 Ludovic Delaveau, 11 years ago

Owner: changed from Simon Litchfield to Ludovic Delaveau
Status: assignednew

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.

Version 0, edited 11 years ago by Ludovic Delaveau (next)

comment:17 by Ludovic Delaveau, 11 years ago

Status: newassigned

by Ludovic Delaveau, 11 years ago

Improved patch

comment:18 by Ludovic Delaveau, 11 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 mkai, 11 years ago

Cc: django@… added

Can the patch be merged? I just hit this problem, too, and the patch fixes it.

comment:20 by anonymous, 11 years ago

Can you mark as ready for checkin ? I will do a pull request after your confirmation.

comment:21 by Ludovic Delaveau, 11 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 mkai, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:23 by Anssi Kääriäinen, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Unfortunately the patch isn't ready for checkin. The extra_regress tests do not pass.

by fhahn, 11 years ago

updated version of the patch, with tests

comment:24 by fhahn, 11 years ago

I've created a branch on Github as well: https://github.com/fhahn/django/tree/ticket_14930

comment:25 by fhahn, 11 years ago

Cc: fhahn added

by fhahn, 11 years ago

made patch python 2.6 compatible

comment:26 by fhahn, 11 years ago

Owner: changed from Ludovic Delaveau to fhahn
Patch needs improvement: unset

I've created a pull request as well: https://github.com/django/django/pull/713

comment:27 by Tim Graham, 11 years ago

Cc: timograham@… added
Triage Stage: AcceptedReady 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.

https://github.com/django/django/pull/1242

comment:28 by Anssi Kääriäinen, 11 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 fhahn, 11 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 Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 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.

comment:31 by Steffen Jasper, 10 years ago

Hello, will this fix also be integrated into the stable branches 1.4 and 1.5?

Best regards

comment:32 by Tim Graham, 10 years ago

No, per our support policy those branches are only receiving security fixes and critical (data loss) bug fixes respectively.

comment:33 by anonymous, 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 Tim Graham, 10 years ago

  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.
Note: See TracTickets for help on using tickets.
Back to Top