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

[rajivm]

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.

[rajivm]

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

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

[Anssi Kääriäinen]

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.

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

[Andrew Godwin]

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?

[Anssi Kääriäinen]

I'll finish this one.

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

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.

[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