[Django] #23259: Query extra() select_params are inserted in the incorrect order in the overall query parameters with values_list
Django
overall query parameters with values_list
----------------------------------------------+----------------------
Reporter: rajivm | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.7-rc-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23259>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by rajivm):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Also, imho, this could be a release blocker because it has potential
security ramifications since the parameters are being mismatched.
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:1>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by rajivm):
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:2>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:3>
Django
in the query with values_list excluding the extra, and extra(order_by=)
including it
-------------------------------------+-------------------------------------
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:4>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:5>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:6>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:7>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:8>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: akaariai
Type: Bug | Status: assigned
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => akaariai
* status: new => assigned
Comment:
I'll finish this one.
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:9>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: akaariai
Component: Database layer | Version: 1.7-rc-2
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f0b358880a6825d667c037757caac470bc526a1f"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:10>
Django
overall query parameters with values_list
Reporter: rajivm | Owner: akaariai
Type: Bug | Status: closed
Component: Database layer | Version: 1.7-rc-2
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"4ce5ced230481fc93288aeea922398bc36102d1e"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23259#comment:11>