Opened 13 years ago

Closed 10 years ago

Last modified 9 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 Changed 13 years ago by Russell Keith-Magee

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 Changed 13 years ago by Russell Keith-Magee

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

Severity: Normal
Type: Bug

comment:4 Changed 13 years ago by Robin

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

comment:5 Changed 12 years ago by Simon Litchfield

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

comment:6 Changed 12 years ago by Simon Litchfield

Status: newassigned

Changed 12 years ago by Simon Litchfield

Changed 12 years ago by Simon Litchfield

Fix- against r17428

comment:7 Changed 12 years ago by Simon Litchfield

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

comment:8 Changed 12 years ago by Simon Litchfield

Easy pickings: set

comment:9 Changed 12 years ago by gosia

Cc: gosia added

comment:10 Changed 11 years ago by Lau Bech Lauritzen

Cc: lau@… added

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

comment:11 Changed 11 years ago by Anssi Kääriäinen

The patch doesn't contain tests.

comment:12 Changed 11 years ago by Ludovic Delaveau

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.

Changed 11 years ago by Ludovic Delaveau

Added test to patch

comment:13 Changed 11 years ago by 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".

comment:14 Changed 11 years ago by Lau Bech Lauritzen

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

comment:15 Changed 11 years ago by 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).

comment:16 Changed 11 years ago by Ludovic Delaveau

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

Last edited 11 years ago by Ludovic Delaveau (previous) (diff)

comment:17 Changed 11 years ago by Ludovic Delaveau

Status: newassigned

Changed 11 years ago by Ludovic Delaveau

Improved patch

comment:18 Changed 11 years ago by Ludovic Delaveau

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 Changed 11 years ago by mkai

Cc: django@… added

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

comment:20 Changed 11 years ago by anonymous

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

comment:21 Changed 11 years ago by Ludovic Delaveau

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

comment:22 Changed 11 years ago by mkai

Triage Stage: AcceptedReady for checkin

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

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

Changed 11 years ago by fhahn

updated version of the patch, with tests

comment:24 Changed 11 years ago by fhahn

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

comment:25 Changed 11 years ago by fhahn

Cc: fhahn added

Changed 11 years ago by fhahn

made patch python 2.6 compatible

comment:26 Changed 11 years ago by fhahn

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 Changed 11 years ago by Tim Graham

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 Changed 11 years ago by 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...

comment:29 Changed 11 years ago 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.

comment:30 Changed 10 years ago by Anssi Kääriäinen <akaariai@…>

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 Changed 10 years ago by Steffen Jasper

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

Best regards

comment:32 Changed 10 years ago by Tim Graham

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

comment:33 Changed 9 years ago 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?

comment:34 Changed 9 years ago by 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.
Note: See TracTickets for help on using tickets.
Back to Top