Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#23259 closed Bug (fixed)

Query extra() select_params are inserted in the incorrect order in the overall query parameters with values_list

Reported by: rajivm Owned by: akaariai
Component: Database layer (models, ORM) Version: 1.7-rc-2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This is running the latest stable/1.7 (267630ad50c69ebfe594de37a0636264aa5be7d6)

User.objects.extra(select=OrderedDict([('points', 'id + %s')]), select_params=[5]).filter(id__gte=3).extra(order_by=['points']).values_list("id", flat=True)
[DEBUG django.db.backends] (0.001) SELECT "auth_user"."id" FROM "auth_user" WHERE "auth_user"."id" >= 5 ORDER BY (id + 3) ASC LIMIT 21; args=(5, 3)

Whereas in Django 1.6 (the correct query):

[DEBUG django.db.backends] (0.003) SELECT "auth_user"."id", (id + 5) AS "points" FROM "auth_user" WHERE "auth_user"."id" >= 3  ORDER BY "points" ASC LIMIT 21; args=(5, 3)

In the second case, the extra select_param of 5 is correctly matched with the extra select "id + %s", resulting in id+5.
However, in the first case, in Django 1.7, the extra select results in id + 3 (incorrectly inverting the parameters to the query).

It seems like what is happening is the "select" of "points" (the extra select) is being optimized out because of the values_list, but is being kept by the extra(order_by) -- resulting in it being in a different part of the query.

I tried to simplify the example as much as possible (I had a much crazier query this was destroying).

Let me know if you have any questions.

Change History (11)

comment:1 Changed 10 months ago by rajivm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Also, imho, this could be a release blocker because it has potential security ramifications since the parameters are being mismatched.

comment:2 Changed 10 months ago by rajivm

  • Type changed from Uncategorized to Bug

comment:3 Changed 10 months ago by rajivm

This bug seems to have been introduced at 2f35c6f10fcbae541691207fb0c0560a13b754fc when trying to resolve ticket #14930.

Based on some cursory research into the source of the bug, it looks like the parameters are simply being inserted too early by the sql compiler in this situation, and I have created a patch (sans tests) and created a pull-request: https://github.com/django/django/pull/3033. It's late now, but I will try to add tests to it tomorrow.

comment:4 Changed 10 months ago by rajivm

  • Summary changed from Query extra() select_params are inserted in the incorrect order in the overall query parameters with values_list to Query extra(select=) select_params are parameterized in the incorrect order in the query with values_list excluding the extra, and extra(order_by=) including it

comment:5 Changed 10 months ago by akaariai

  • Severity changed from Normal to Release blocker
  • Summary changed from Query extra(select=) select_params are parameterized in the incorrect order in the query with values_list excluding the extra, and extra(order_by=) including it to Query extra() select_params are inserted in the incorrect order in the overall query parameters with values_list
  • Triage Stage changed from Unreviewed to Accepted

I don't agree with security ramifications here. Yes, there could be some cases where this could lead to users seeing data they aren't supposed to. But, any bug in the ORM that produces wrong results qualify for that, and we definitely are not going to interpret all ORM bugs as security issues.

However, this is a regression and thus a release blocker.

I haven't tested this myself, but based on the research done this seems valid, so marking as accepted.

comment:6 Changed 10 months ago by charettes

  • Has patch set
  • Needs tests set
  • Patch needs improvement set

comment:7 Changed 10 months ago by rajivm

I have added tests to the provided pull request that cover this specific issue. However, on a more general note, it seems like tests covering this area are lacking (the previously-referenced issue / commit that introduced this bug added more tests, but they only cover a very narrow case, mainly, there are many tests around ordering in values() w/ extras, but they don't actually test ordering it-self).

comment:8 Changed 10 months ago by andrewgodwin

Can one of the other core devs or the patch author tell me what about this patch needs improvement? Or are we waiting for someone to review the tests?

comment:9 Changed 10 months ago by akaariai

  • Owner changed from nobody to akaariai
  • Status changed from new to assigned

I'll finish this one.

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

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

In f0b358880a6825d667c037757caac470bc526a1f:

Fixed #23259 -- Corrected insertion order of extra() select_params

A regression caused queries to produce incorrect results for cases where
extra(select) is excluded by values() but included by extra(order_by)

The regression was caused by 2f35c6f10fcbae541691207fb0c0560a13b754fc.

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

In 4ce5ced230481fc93288aeea922398bc36102d1e:

[1.7.x] Fixed #23259 -- Corrected insertion order of extra() select_params

A regression caused queries to produce incorrect results for cases where
extra(select) is excluded by values() but included by extra(order_by)

The regression was caused by 2f35c6f10fcbae541691207fb0c0560a13b754fc.

Backport of f0b358880a from master

Note: See TracTickets for help on using tickets.
Back to Top