Opened 7 years ago

Closed 2 years ago

Last modified 2 years ago

#9055 closed Bug (fixed)

Percent sign in SQL statement behaves different with CursorDebugWrapper

Reported by: guettli Owned by: claudep
Component: Database layer (models, ORM) Version: master
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 guettli 7 years ago.
9055-2.diff (3.3 KB) - added by claudep 4 years ago.
Patch including tests for MySQL and SQLite
9055-3.diff (6.8 KB) - added by wdoekes 3 years ago.

Download all attachments as: .zip

Change History (23)

Changed 7 years ago by guettli

comment:1 Changed 7 years ago by guettli

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 7 years ago by mtredinnick

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 6 years ago by kmtracey

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 Changed 6 years ago by wdoekes

"""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 Changed 6 years ago by wdoekes

  • Cc walter+django@… added

comment:6 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 4 years ago by anonymous

  • Easy pickings unset
  • UI/UX unset

Changed 4 years ago by claudep

Patch including tests for MySQL and SQLite

comment:8 Changed 4 years ago by claudep

  • 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 Changed 4 years ago by 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.

comment:10 Changed 3 years ago by gst

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 Changed 3 years ago by wdoekes

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

Changed 3 years ago by wdoekes

comment:12 Changed 3 years ago by wdoekes

  • Triage Stage changed from Accepted to Design decision needed
  • Version changed from 1.0 to master

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 Changed 3 years ago by claudep

#19883 is a duplicate.

comment:14 Changed 2 years ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 2 years ago by claudep

#20115 is a duplicate

comment:16 Changed 2 years ago by claudep

  • Owner changed from nobody to claudep
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Ready for checkin

I'm going to kill this one...

comment:17 Changed 2 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 2 years ago by aaugustin

  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to new

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 Changed 2 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 2 years ago by aaugustin

Thank you very much!

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