Opened 3 years ago

Closed 3 years ago

#32231 closed Bug (fixed)

It should be possible to pass None as params for Model.objects.raw

Reported by: Alexander Lyabah Owned by: Alexander Lyabah
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: raw, psycopg2, execute, orm, db
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was playing with:

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

Attachments (1)

params_none.diff (4.3 KB ) - added by Alexander Lyabah 3 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Simon Charette, 3 years ago

Not sure I understand the rationale here

Doing something along

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

query = "SELECT * FROM raw_query_author WHERE first_name like %s"
qset = Author.objects.raw(query, ('J%',))

comment:2 by Simon Charette, 3 years ago

From the Executing custom SQL directly section of the 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 no 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 when 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

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.

Last edited 3 years ago by Simon Charette (previous) (diff)

by Alexander Lyabah, 3 years ago

Attachment: params_none.diff added

comment:3 by Alexander Lyabah, 3 years ago

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 when 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

The updated patch with backwards compatibility should prevent double-percentage being broken, and still allow using None as params

Last edited 3 years ago by Alexander Lyabah (previous) (diff)

comment:4 by Simon Charette, 3 years ago

Triage Stage: UnreviewedAccepted
Version: 2.2master

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.

comment:5 by Jacob Walls, 3 years ago

Owner: changed from nobody to Alexander Lyabah
Status: newassigned

comment:6 by Mariusz Felisiak, 3 years ago

Needs documentation: set
Patch needs improvement: set

comment:7 by Mariusz Felisiak, 3 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In aa3d3606:

Refs #32231 -- Added tests for QuerySet.raw() with an escaped % symbol.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 415f5029:

Fixed #32231 -- Allowed passing None params to QuerySet.raw().

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