Code

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#121 closed enhancement (fixed)

[patch] Names in SQL should be quoted

Reported by: sune.kirkeby@… Owned by: adrian
Component: Metasystem Version:
Severity: normal Keywords: sql
Cc: rmunn@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Some valid Python-identifiers are reserved words in SQL-databases, for instance "when" in PostgreSQL. It would be very nice, if Django would quote all names (e.g. schema, table, row) in SQL statements, so these database-reserved words would not pose problems.

Attachments (3)

quote_name.patch (2.4 KB) - added by rmunn@… 9 years ago.
Patch to add quote_name() to all db backends
change_all_sql.patch (36.3 KB) - added by rmunn@… 9 years ago.
Patch to quote all SQL identifiers with db.quote_name()
fixed_quote_name.patch (2.4 KB) - added by rmunn@… 9 years ago.
Fixed patch to add quote_name() to all db backends - previous patch had wrong linecounts

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by rmunn@…

In both PostgreSQL and SQLite, identifiers such as table and field names are quoted using the " (double-quote) character. In MySQL, identifiers are quoted using the ` (backtick) character. A helper function will be needed to quote names appropriately for each database engine.

comment:2 Changed 9 years ago by rmunn@…

The above patch adds the necessary helper function. Now it's "just" a case of going through the Django source and finding all the places where field names need to be quoted. Not exactly non-trivial, but doable.

comment:3 Changed 9 years ago by rmunn@…

I meant "Not exactly trivial", of course.

comment:4 Changed 9 years ago by rmunn@…

After working on quoting the field names for a short while, I discovered that quote_identifier() is far too long to type, especially when in some places it needs to be repeated four times for a single SQL statement! So I renamed it to quote_name(). I also added a little extra "smarts" to the function, so that if an identifier has already been quoted it won't get quoted again. Please ignore the quote_identifier.patch above and replace it with the following quote_name.patch:

Changed 9 years ago by rmunn@…

Patch to add quote_name() to all db backends

comment:5 Changed 9 years ago by rmunn@…

And finally, here's the (very large) patch to correctly quote all identifiers (table and field names) in the SQL of all of Django. Phew!

Changed 9 years ago by rmunn@…

Patch to quote all SQL identifiers with db.quote_name()

comment:6 Changed 9 years ago by rmunn@…

  • Summary changed from Names in SQL should be quoted to [patch] Names in SQL should be quoted

Tagging summary with [patch] to flag it for developers' attention. Also noticed that I uploaded a broken version of the quote_name.patch; fixed version of that patch coming up.

Changed 9 years ago by rmunn@…

Fixed patch to add quote_name() to all db backends - previous patch had wrong linecounts

comment:8 Changed 9 years ago by rmunn@…

  • Cc rmunn@… added

comment:9 Changed 9 years ago by rmunn@…

It's worth noting Ian Sparks' comment at http://www.djangoproject.com/weblog/2005/jul/21/sqlite3/#c250 that if we quote all field names in the SQL, then those field names become case-sensitive. This shouldn't have any effect on the generated SQL, since we always use code like [f.name for f in self._meta.fields] and so the names being sent to the database are already case-consistent. But if we end up with strange errors immediately on applying this patch, one troubleshooting approach will be to look for any case-inconsistencies in field names throughout the Django code.that's one possible place to look

comment:10 Changed 9 years ago by adrian

  • Component changed from Core framework to Metasystem

comment:11 Changed 9 years ago by adrian

  • Status changed from new to assigned

comment:12 Changed 9 years ago by hugo

  • milestone set to Version 1.0

I think this really should go in, especially in the light of more db backends coming up. The problem is, more backends means more reserved words, means people get short on column names - or problems with incompatibilities over different databases are bound to happen, as people tend to use short names that might be reserved by some database.

I set the milestone to 1.0 because I think this really should go in, as it doesn't change djangos functionality at all, but makes portability of applications much simpler.

comment:13 Changed 9 years ago by adrian

(In [1039]) Added quote_name hook for each database backend. Refs #121. Thanks, Robin Munn -- we miss you.

comment:14 Changed 9 years ago by Sune Kirkeby <sune.kirkeby@…>

I have a recent version of the quote-names patch which works like a charm here. Any interest in merging this soon? I don't feel like spamming the list of attached files with an updated patch for every change in d.c.meta which touches the SQL :)

comment:15 Changed 9 years ago by adrian

Sure, go ahead and attach the patch to this ticket. I'm planning on integrating this shortly -- hopefully on Sunday, if I have the time.

comment:16 Changed 9 years ago by adrian

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

(In [1224]) Fixed #121 -- Django now quotes all names in SQL queries. Also added unit tests to confirm. Thanks, Robin Munn and Sune.

comment:17 Changed 7 years ago by anonymous

  • milestone Version 1.0 deleted

Milestone Version 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.