Opened 19 years ago

Last modified 6 hours ago

#6343 assigned Bug

% symbols not escaped in db_column column names when preparing queries

Reported by: Daniel Pope <dan@…> Owned by: Garvit Sharma
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Annabelle Wiegart Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Using % in database column names (specified using db_column) causes the database wrapper to fail when preparing queries.

This is because the % symbol is not properly quoted (as %%), and conflicts with the usage of %s for passing parameters to queries.

I am attaching a patch for the MySQL backend where I encountered the issue; I'm not sure if other backends exhibit this bug because it presumably depends both on whether the database's native capability to support % characters in column names, and on the ​Python DB-API paramstyle.

Attachments (1)

percent_column_names.diff​ (514 bytes ) - added by Daniel Pope <dan@…> 19 years ago.
Patch for MySQL backend

Download all attachments as: .zip

Change History (14)

by Daniel Pope <dan@…>, 19 years ago

Attachment: percent_column_names.diff​ added

Patch for MySQL backend

comment:1 by Simon Greenhill <dev@…>, 19 years ago

Triage Stage: Unreviewed → Design decision needed

You can use % in db column names? I really think that's a bad bad idea to start with, but I'll put this as Design Dec. Needed just in case.

comment:2 by Julien Phalip, 16 years ago

Severity: → Normal
Type: → Bug

Guessing from the report, I take it this is a bug, but I'm not sure if it is still a valid one though...

comment:3 by Julien Phalip, 16 years ago

Needs tests: set

comment:4 by Julien Phalip, 16 years ago

See #11391 for a potentially related issue.

comment:5 by Julien Phalip, 16 years ago

See some other issues relating to the use of the % character and escaping: #3485, #12268, #11391, #13648. Perhaps some of these could be tackled at the same time.

comment:6 by Carl Meyer, 15 years ago

Easy pickings: unset
Triage Stage: Design decision needed → Accepted
UI/UX: unset

This either needs to be fixed, or if that's not technically feasible in a sane way, documented.

#11391 closed as duplicate of this, also raises the issue that this can happen with field names, not just table names. That should also be addressed.

comment:7 by YuRong Kao, 5 months ago

Owner: changed from nobody to YuRong Kao
Status: new → assigned

comment:8 by YuRong Kao, 5 months ago

Owner: YuRong Kao removed
Status: assigned → new

comment:9 by Garvit Sharma, 4 weeks ago

Owner: set to Garvit Sharma
Status: new → assigned

Still reproducible on main (08e4c0d8e7), though the failure is not where the original patch targets.

Minimal reproduction:

class Thing(models.Model):
price = models.IntegerField(db_column="cost%")
str(Thing.objects.filter(price=5).query)
ValueError: unsupported format character '"' (0x22) at index 55

str(Thing.objects.all().query)
TypeError: not enough arguments for format string

Both come from Query.str in django/db/models/sql/query.py:

def str(self):
sql, params = self.sql_with_params()
return sql % params

The % in the column name is read as the start of a format specifier by the % operator. With params present it fails on the character following %; with no params it fails for lack of arguments.

sql_with_params() is unaffected and returns the correct SQL and parameters:

Thing.objects.filter(price=5).query.sql_with_params()
('SELECT "check_app_thing"."id", "check_app_thing"."cost%" FROM "check_app_thing" WHERE "check_app_thing"."cost%" = %s', (5,))

Query execution also works on SQLite, where the driver uses qmark paramstyle. I have not tested execution on MySQL or PostgreSQL, where substitution happens inside the driver — that path may still fail as originally reported.

Since Query.str is in the generic SQL layer rather than a backend, this failure appears to be backend-independent, but I have only tested SQLite.

I'd like to work on this.

comment:10 by Garvit Sharma, 12 days ago

​PR

Takes the narrow approach: Query.__str__() returns the uninterpolated SQL
rather than raising when an identifier contains %.

I also tried the fuller fix - escaping % in quote_name() like Oracle
does. This corrupts DDL: CREATE TABLE carries the escaped name through
and the column ends up called cost%%, which should have been cost%.
SQLite already has a %% → % collapse, and making that collapse
unconditional fixes SQLite, but MySQL and PostgreSQL have no collapse step
at all and one would have to be added. That would mean % processing on
paths that currently do none.

Happy to go that way instead if preferred.

comment:11 by Johannes Maron, 11 days ago

Needs documentation: set
Patch needs improvement: set

in reply to:  9 comment:12 by Annabelle Wiegart, 5 days ago

Replying to Garvit Sharma:

Still reproducible on main (08e4c0d8e7), though the failure is not where the original patch targets.

Minimal reproduction:

class Thing(models.Model):
price = models.IntegerField(db_column="cost%")
str(Thing.objects.filter(price=5).query)
ValueError: unsupported format character '"' (0x22) at index 55

str(Thing.objects.all().query)
TypeError: not enough arguments for format string

Both come from Query.str in django/db/models/sql/query.py:

def str(self):
sql, params = self.sql_with_params()
return sql % params

The % in the column name is read as the start of a format specifier by the % operator. With params present it fails on the character following %; with no params it fails for lack of arguments.

sql_with_params() is unaffected and returns the correct SQL and parameters:

Thing.objects.filter(price=5).query.sql_with_params()
('SELECT "check_app_thing"."id", "check_app_thing"."cost%" FROM "check_app_thing" WHERE "check_app_thing"."cost%" = %s', (5,))

Query execution also works on SQLite, where the driver uses qmark paramstyle. I have not tested execution on MySQL or PostgreSQL, where substitution happens inside the driver — that path may still fail as originally reported.

Since Query.str is in the generic SQL layer rather than a backend, this failure appears to be backend-independent, but I have only tested SQLite.

I'd like to work on this.

Hey Garvit, thank you for giving this a try! :)

I was unable to reproduce your query in ​DryORM. The ValueError already occurs at table creation. I'm wondering whether we should provide an error message stating that "%" is forbidden in column names in that case. That doesn't solve the problem with queries in legacy projects with database columns containing "%" though.

Would you be able to share how reproduce your query?

Version 0, edited 5 days ago by Annabelle Wiegart (next)

comment:13 by Annabelle Wiegart, 7 hours ago

Cc: Annabelle Wiegart added
Note: See TracTickets for help on using tickets.
Back to Top