Code

Opened 4 years ago

Closed 13 months ago

Last modified 9 months ago

#14930 closed Bug (fixed)

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

Reported by: lsaffre Owned by: fhahn
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: robinchew@…, simon@…, gosia, lau@…, vicould, 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 simon29 2 years ago.
extra_order_by_values_list-17428.diff (1.3 KB) - added by simon29 2 years ago.
Fix- against r17428
extra_order_by_values_list-with_test.diff (2.4 KB) - added by vicould 22 months ago.
Added test to patch
extra_order_by_values_list-with_test-2.diff (3.7 KB) - added by vicould 22 months ago.
Improved patch
extra_order_by_values_list_with_tests_3.diff (5.5 KB) - added by fhahn 17 months ago.
updated version of the patch, with tests
extra_order_by_values_list_with_tests_4.diff (5.5 KB) - added by fhahn 17 months ago.
made patch python 2.6 compatible

Download all attachments as: .zip

Change History (38)

comment:1 Changed 4 years ago by russellm

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

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 3 years ago by russellm

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to 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 Changed 3 years ago by anonymous

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 3 years ago by robin

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

comment:5 Changed 2 years ago by simon29

  • Cc simon@… added
  • Has patch set
  • Needs tests set
  • Owner changed from nobody to simon29
  • Status changed from reopened to new

comment:6 Changed 2 years ago by simon29

  • Status changed from new to assigned

Changed 2 years ago by simon29

Changed 2 years ago by simon29

Fix- against r17428

comment:7 Changed 2 years ago by simon29

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

comment:8 Changed 2 years ago by simon29

  • Easy pickings set

comment:9 Changed 2 years ago by gosia

  • Cc gosia added

comment:10 Changed 2 years ago by lau

  • Cc lau@… added

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

comment:11 Changed 2 years ago by akaariai

The patch doesn't contain tests.

comment:12 Changed 22 months ago by vicould

  • Cc vicould 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 22 months ago by vicould

Added test to patch

comment:13 Changed 22 months ago by vicould

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 22 months ago by lau

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

comment:15 Changed 22 months ago 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).

comment:16 Changed 22 months ago by vicould

  • Owner changed from simon29 to vicould
  • Status changed from assigned to new

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 22 months ago by vicould (next)

comment:17 Changed 22 months ago by vicould

  • Status changed from new to assigned

Changed 22 months ago by vicould

Improved patch

comment:18 Changed 22 months ago by vicould

  • 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 19 months 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 19 months ago by anonymous

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

comment:21 Changed 19 months ago by vicould

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 19 months ago by mkai

  • Triage Stage changed from Accepted to Ready for checkin

comment:23 Changed 19 months ago by akaariai

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 17 months ago by fhahn

updated version of the patch, with tests

comment:24 Changed 17 months ago by fhahn

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

comment:25 Changed 17 months ago by fhahn

  • Cc fhahn added

Changed 17 months ago by fhahn

made patch python 2.6 compatible

comment:26 Changed 17 months ago by fhahn

  • Owner changed from vicould to fhahn
  • Patch needs improvement unset

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

comment:27 Changed 13 months ago by timo

  • Cc timograham@… added
  • Triage Stage changed from Accepted to 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.

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

comment:28 Changed 13 months ago 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...

comment:29 Changed 13 months 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 13 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 9 months ago by knoffhoff

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

Best regards

comment:32 Changed 9 months ago by timo

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.