[Django] #32231: It should be possible to pass None as params for Model.objects.raw
Django
-------------------------------------+-------------------------------------
Reporter: Alexander | Owner: nobody
Lyabah |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) | Keywords: raw, psycopg2,
Severity: Normal | execute, orm, db
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I was playing with:
* psycopg2==2.8.6
* last django 2.2 (-e
git...@github.com:django/django.git@e893c0ad8b0b5b0a1e5be3345c287044868effc4#egg=Django)
Model.objects.raw has raw_query and params=None attributes, as well as
cursor.execute has the same attributes, and `raw` will eventually calls
`execute`, since `raw` is more low-level way of using DB.
But, params=None of `raw` function doesn't pass as None into `execute`
function, but empty tuple instead.
The problem here is that, psycopg2 treats differently params=None and
params=(), and it is impossible to pass None using `raw` function.
The simple example can be found in test from patch
query = "SELECT * FROM raw_query_author WHERE first_name like
'J%'"
qset = Author.objects.raw(query)
results = list(qset)
print(len(results))
in current Django version (I was using 2.2) it raises an exception from
psycopg2
The attached patch allows using None in params
--
Ticket URL: <https://code.djangoproject.com/ticket/32231>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "params_none.diff" added.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "params_none.diff" added.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Not sure I understand the rationale here
Doing something along
{{{#!python
query = "SELECT * FROM raw_query_author WHERE first_name like 'J%'"
qset = Author.objects.raw(query)
}}}
Is prone to SQL injection assuming `'J%'` could be coming from user input,
you definitely want to be doing the following instead
{{{#!python
query = "SELECT * FROM raw_query_author WHERE first_name like %s"
qset = Author.objects.raw(query, ('J%',))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
From the ''Executing custom SQL'' directly section of the
''[https://docs.djangoproject.com/en/3.1/topics/db/sql/#executing-custom-
sql-directly Performing raw SQL queries documentation]''
> Note that if you want to include literal percent signs in the query, you
have to double them **in the case you are passing parameters**:
>
> `cursor.execute("SELECT foo FROM bar WHERE baz = '30%'")`
> `cursor.execute("SELECT foo FROM bar WHERE baz = '30%%' AND id = %s",
[self.id])`
So it seems that it was meant to work the way described above when not
parameters are provided but it was never tested to do so and might have
regressed at some point?
We should determine for how long the implementation has diverged from the
documentation to take an informed decision here as some users might have
simply worked around this limitation by always doubling percentage signs
even no parameters are provided over the years. If we were to simply
switch back to the documented way of doing things we could silently break
queries of the form
{{{#!python
cursor.execute("SELECT foo FROM bar WHERE baz = '30%%'")
}}}
I think we either need to go through a deprecation period where
`params=()` keeps being passed `if '%%' in sql` or bite the bullet and
adjust the documentation accordingly.
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "params_none.diff" added.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage:
execute, orm, db | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alexander Lyabah):
I update the patch for backward compatibility
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
execute, orm, db |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.2 => master
* stage: Unreviewed => Accepted
Comment:
To amend what I said above, `cursor` works fine, it's only `raw` that
doesn't so the documentation divergence seems moot.
Allowing `params=None` to be explicitly specified to opt-in this behaviour
like the reporter suggested seems like an acceptable compromise.
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:4>
Django
-------------------------------------+-------------------------------------
| Lyabah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage: Accepted
execute, orm, db |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Alexander Lyabah
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:5>
Django
-------------------------------------+-------------------------------------
| Lyabah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage: Accepted
execute, orm, db |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:6>
Django
-------------------------------------+-------------------------------------
| Lyabah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
execute, orm, db | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:7>
Django
-------------------------------------+-------------------------------------
| Lyabah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: raw, psycopg2, | Triage Stage: Ready for
execute, orm, db | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"aa3d36063174cc1e16a1e5150b6b47609dd1e79a" aa3d3606]:
{{{
#!CommitTicketReference repository=""
revision="aa3d36063174cc1e16a1e5150b6b47609dd1e79a"
Refs #32231 -- Added tests for QuerySet.raw() with an escaped % symbol.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:8>
Django
-------------------------------------+-------------------------------------
| Lyabah
Component: Database layer | Version: master
(models, ORM) |
Keywords: raw, psycopg2, | Triage Stage: Ready for
execute, orm, db | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"415f50298f97fb17f841a9df38d995ccf347dfcc" 415f5029]:
{{{
#!CommitTicketReference repository=""
revision="415f50298f97fb17f841a9df38d995ccf347dfcc"
Fixed #32231 -- Allowed passing None params to QuerySet.raw().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32231#comment:9>