Percent sign in SQL statement behaves different with CursorDebugWrapper

[Thomas Güttler]

Hi,

cursor.execute(sql_string) behaves different if used with CursorDebugWrapper or not, if the string
contains a percent sign.

If it is a psycopg2 cursor I need no escaping. If it is a CursorDebugWrapper,
I need to do sql=sql.replace('%', '%%') first.

The one line patch is attached.

My question on comp.lang.python:
​http://groups.google.de/group/comp.lang.python/browse_frm/thread/10ed61a621ea5aaa/0ff82407042441b1?hl=de&lnk=st&q=corner+case+python+guettler#0ff82407042441b1

Related: #3041

[Thomas Güttler]

Other backends might need to be changed, too:

user@host:~/_djangosvn/trunk/django/db/backends> find -name '*.py' | xargs grep -E 'def execute\('
./postgresql/base.py:    def execute(self, sql, params=()):
./sqlite3/base.py:    def execute(self, query, params=()):
./mysql/base.py:    def execute(self, query, args=None):
./oracle/base.py:    def execute(self, query, params=None):
./util.py:    def execute(self, sql, params=None):

[Malcolm Tredinnick]

This needs a test case, please. Something that fails beforehand and passes afterwards. You will see a few tests that set DEBUG=True and the start and reset it at the end to test things like this.

[Karen Tracey]

Note: the patch provided for MySQL backend on #10851 also made the same change to executemany, so when this is looked at for check in it should be investigated whether executemany needs fixing as well.

[Walter Doekes]

"""also made the same change to executemany"""

No I didn't ;-) I only replaced the parameter name "args" with "params" (which should've been "param_list" anyway, sloppy me). There was no actual change in the code for executemany.

[anonymous]

fix ​https://github.com/django/django/pull/101

[Claude Paroz]

Patch including tests for MySQL and SQLite

[Claude Paroz]

Surely the different default params/args to the backend's execute method is not very clean. With my latest patch, we don't change backends defaults, but simply do not enforce any default from CursorDebugWrapper. It might be even better if we had an example query for all backends.

[hlhicks@…]

I'm not sure how this ticket got sidetracked into parameters in execute. that may be an issue as well, but the original problem mentioned above refers to "if the string contains a percent sign." If the query has a percent sign, then self.db.ops.last_executed_query(...) will bomb when it tries to do a string substitution. It will also fail if the query has a different parameter sub method (like ?) . For example... cursor.execute("insert into the_table (name, phone, zip) values (?,?,?)",("me","800...","30101")) will throw an exception because it cannot substitute the three parameters into the query.

last_executed_query needs to use the backends substitution method (which may be complicated to do). Or else just store the parameters as a repr after the query string.

[Grégory Starck]

Hi,

just wondering what's missing here for having the proposed fix be included in stable/1.4.x branch ?

(cause I had the exact same problem and had open a duplicate of this bug (see https://code.djangoproject.com/ticket/19873) proposing the same kind of fix (the search of the tickets doesn't seem very effective)).

regards,

g.

[Walter Doekes]

@hlhicks: the point was not that percent signs are a problem. The point was that they were treated differently when in DEBUG mode.

The solution was to enforce string interpolation to be performed in all cases, instead of half of the cases. The default parameter should be () instead of None, so '%%' % () would become '%'.

Sure, the default could be None too. As long as it doesn't differ between DEBUG and PRODUCTION. But you don't want to mess with PRODUCTION, so changing the DEBUG is a safe bet.

---

For example... cursor.execute("insert into the_table
(name, phone, zip) values (?,?,?)",("me","800...","30101"))

As for the db.ops.last_executed_query, that's just code to aid in debugging. The important stuff happens here (in the MySQL case):

  File ".../MySQLdb/cursors.py", line 159, in execute
    query = query % db.literal(args)

Where db.literal escapes args into quoted variants when appropriate.

Here I could see you might want question marks (?) to work, but the web has this to say:

Sadly, DBAPI requires a parameter type, but it doesn't specify
which. You have to look at the module's paramstyle to determine
which. Some DBAPI modules use the qmark style. MySQLdb uses the
format style, which unfortunately results in exactly this
confusion between parameterisation and string formatting. It's a
mess. (bobince@stackoverflow)

​http://www.python.org/dev/peps/pep-0249/#paramstyle

$ grep paramstyle\ = .../MySQLdb/__init__.py
paramstyle = "format"

---

@gst: re: Search. Yeah. Trac isn't always good at finding what you're looking for.

As for the fix. I believe anyone is allowed to mark bugs as Ready For Checkin, when they've tested that the relevant patch applies cleanly to trunk and passes its tests.

[Walter Doekes]

Ok. I was only partially right.

(a) Cursor behaviour was inconsistent for the different backends:

• oracle and sqlite always got interpolation
• mysql and postgres only got interpolation if (DEBUG or len(params)>0)

(b) Changing () to None would be the fix that breaks DEBUG and not PRODUCTION, not the other way around.

---

I updated claudep's patch to make it consistent: percent signs only need escaping if params is supplied (and not None).

Tested with: mysql and sqlite. I also tested that passing None to postgres gives no interpolation, so that should work.

Unfortunately, this changes things for everyone using parameter-less queries. (Except mysql and postgres users in non-DEBUG.) So I'm switching it to design decision needed. (Apologies in advance if I'm not supposed to do that.)

Regards,
Walter Doekes

[Claude Paroz]

#19883 is a duplicate.

[Anssi Kääriäinen]

I think making the backends consistent is the right thing to do. All the backends should work similarly. Seems like breaking DEBUG is the right thing to do (assuming this ends up in situation where every backend works consistently). This can be classified as bug fix where backwards compatibility doesn't matter.

[Claude Paroz]

#20115 is a duplicate

[Claude Paroz]

I'm going to kill this one...

[Claude Paroz <claude@…>]

In 76aecfbc4b49f5ab0613cccff1df6fab03253fab:

Fixed #9055 -- Standardized behaviour of parameter escaping in db cursors

Previously, depending on the database backend or the cursor type,
you'd need to double the percent signs in the query before passing
it to cursor.execute. Now cursor.execute consistently need percent
doubling whenever params argument is not None (placeholder substitution
will happen).
Thanks Thomas Güttler for the report and Walter Doekes for his work
on the patch.

[Aymeric Augustin]

As said is comment 12 ("Unfortunately, this changes things for everyone using parameter-less queries") this is backwards-incompatible, it broke one of my sites. It must be mentioned in the release notes.

[Claude Paroz <claude@…>]

In 975c5afdb5a0c2f9f61f9faecf8dbd928c4996b7:

Added release note about percent literals in cursor.execute

Thanks Aymeric Augustin for noticing the omission and Tim Graham
for the text review.
Fixes #9055 (again).

[Aymeric Augustin]

Thank you very much!