`values_list()` fails on queryset ordered by extra column

[Luc Saffre]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

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.

[Simon Litchfield]

Fix- against r17428

[Simon Litchfield]

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

[Lau Bech Lauritzen]

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

[Anssi Kääriäinen]

The patch doesn't contain tests.

[Ludovic Delaveau]

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.

[Ludovic Delaveau]

Added test to patch

[Ludovic Delaveau]

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

[Lau Bech Lauritzen]

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

[Simon Litchfield]

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

[Ludovic Delaveau]

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.

[Ludovic Delaveau]

Improved patch

[Ludovic Delaveau]

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.

[mkai]

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

[anonymous]

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

[Ludovic Delaveau]

Sorry, I forgot to log in.
Can you mark as ready for checkin ? I will do a pull request after your confirmation.

[Anssi Kääriäinen]

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

[fhahn]

updated version of the patch, with tests

[fhahn]

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

[fhahn]

made patch python 2.6 compatible

[fhahn]

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

[Tim Graham]

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

[Anssi Kääriäinen]

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

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

[Anssi Kääriäinen <akaariai@…>]

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.

[Steffen Jasper]

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

Best regards

[Tim Graham]

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

[anonymous]

So what we (v1.4/1.5 users) can do, if we can't upgrade Django to 1.7 because of broken compatibility?

[Tim Graham]

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.