Opened 16 years ago

Closed 11 years ago

Last modified 11 years ago

#9055 closed Bug (fixed)

Percent sign in SQL statement behaves different with CursorDebugWrapper

Reported by: Thomas Güttler Owned by: Claude Paroz
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords:
Cc: hv@…, walter+django@… 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

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

Attachments (3)

9055.diff (485 bytes ) - added by Thomas Güttler 16 years ago.
9055-2.diff (3.3 KB ) - added by Claude Paroz 12 years ago.
Patch including tests for MySQL and SQLite
9055-3.diff (6.8 KB ) - added by Walter Doekes 11 years ago.

Download all attachments as: .zip

Change History (23)

by Thomas Güttler, 16 years ago

Attachment: 9055.diff added

comment:1 by Thomas Güttler, 16 years ago

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):

comment:2 by Malcolm Tredinnick, 16 years ago

Component: UncategorizedDatabase layer (models, ORM)
Needs tests: set
Triage Stage: UnreviewedAccepted

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.

comment:3 by Karen Tracey, 15 years ago

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.

comment:4 by Walter Doekes, 15 years ago

"""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.

comment:5 by Walter Doekes, 15 years ago

Cc: walter+django@… added

comment:6 by Luke Plant, 13 years ago

Severity: Normal
Type: Bug

comment:7 by anonymous, 12 years ago

Easy pickings: unset
UI/UX: unset

by Claude Paroz, 12 years ago

Attachment: 9055-2.diff added

Patch including tests for MySQL and SQLite

comment:8 by Claude Paroz, 12 years ago

Needs tests: unset

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.

comment:9 by hlhicks@…, 12 years ago

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.

comment:10 by Grégory Starck, 11 years ago

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.

comment:11 by Walter Doekes, 11 years ago

@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.

by Walter Doekes, 11 years ago

Attachment: 9055-3.diff added

comment:12 by Walter Doekes, 11 years ago

Triage Stage: AcceptedDesign decision needed
Version: 1.0master

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

comment:13 by Claude Paroz, 11 years ago

#19883 is a duplicate.

comment:14 by Anssi Kääriäinen, 11 years ago

Triage Stage: Design decision neededAccepted

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.

comment:15 by Claude Paroz, 11 years ago

#20115 is a duplicate

comment:16 by Claude Paroz, 11 years ago

Owner: changed from nobody to Claude Paroz
Status: newassigned
Triage Stage: AcceptedReady for checkin

I'm going to kill this one...

comment:17 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

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.

comment:18 by Aymeric Augustin, 11 years ago

Resolution: fixed
Severity: NormalRelease blocker
Status: closednew

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.

comment:19 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

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).

comment:20 by Aymeric Augustin, 11 years ago

Thank you very much!

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