Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#12923 closed Bug (worksforme)

Base last_executed_query() does not escape parameters

Reported by: Pablo Brasero Owned by: Pablo Brasero
Component: Database layer (models, ORM) Version: 1.2-beta
Severity: Normal Keywords: last_executed_query escaping
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The base implementation of last_executed_query() does not escape its parameters. For example, a query like the following is returned:

u'SELECT * FROM things WHERE name = unscaped"stuff'

When it should be something along the lines of:

u'SELECT * FROM things WHERE name = "unscaped\"stuff"'

Nevertheless, it can be argued that BaseDatabaseOperations does not make assumptions as to what quoting scheme each different database prefers. In this case, then the problem is that the MySQL backend (and probably all others except postgresql_psycopg2) suffer this problem, by not implementing their own versions of last_executed_query().

A specific third-party piece of software that is affected by this problem is django-devserver. This raises an exception in my computer when it tries to display recently executed SQL statements that contain characters such as quotes, as it relies on last_executed_query() to do so.

I'm attaching a patch for 1.2-beta, although 1.1 is affected too. It implements a MySQL-specific solution in the appropriate backend.

Attachments (2)

mysql-last_executed_query.patch (507 bytes ) - added by Pablo Brasero 14 years ago.
Patch for DatabaseOperations on the MySQL backend
last_executed_query.patch (2.9 KB ) - added by Pablo Brasero 14 years ago.
Patch for DatabaseOperations, with tests for MySQL and SQLite

Download all attachments as: .zip

Change History (11)

by Pablo Brasero, 14 years ago

Patch for DatabaseOperations on the MySQL backend

comment:1 by Russell Keith-Magee, 14 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

If we're going to address this problem, we should address it for all backends.

comment:2 by Pablo Brasero, 14 years ago

Owner: changed from nobody to Pablo Brasero
Status: newassigned

I've been working on this one today. I'm not entirely sure what the best approach would be, so I need a bit of advice at this point. The new patch provides a test, passing for the MySQL and SQLite backends.

I'm not entirely happy about the test. Testing the strings in such a literal fashion looks a bit brittle to me. However, it's the string that we are checking after all, so couldn't think of anything better.

Also, I have created a new method on the abstract backend. It's called construct_query() and makes the SQL template and the params into a "final" SQL query. It also receives the cursor so subclasses can use backend-specific methods. I provide generic quoting+escaping, later overriden on the MySQL subclass.

The method contruct_query() is called by last_executed_query(), therefore the Unicode conversions in the latter method are kept.

Before going on to implement this for other backends, can I have some feedback on this one, please?

by Pablo Brasero, 14 years ago

Attachment: last_executed_query.patch added

Patch for DatabaseOperations, with tests for MySQL and SQLite

comment:3 by Pablo Brasero, 14 years ago

Patch needs improvement: unset

Maybe I should have unticked the "patch needs improvement" box after offering a new patch? (Sorry, I'm not sure of how people roll here)

comment:4 by Luke Plant, 13 years ago

Type: Bug

comment:5 by Luke Plant, 13 years ago

Severity: Normal

comment:6 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 by Claude Paroz, 11 years ago

Resolution: worksforme
Status: assignedclosed

The current code either gets the query from the backend (which should be properly escaped) or provides the query and params separated ("QUERY = %r - PARAMS = %r"). Trying to reconstruct the backend query is probably bound to fail and is not Django's business. Therefore I don't think this issue is still valid.

comment:9 by Aymeric Augustin, 11 years ago

For what it's worth, reconstructing the backend query is doable in about 5 lines of code. I attached a patch that did that to another ticket one or two years ago. However, the technique I used was rejected by the BDFL and the ticket closed as wontfix.

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