All schema API operations should be reserved-word safe
Island Usurper - September 29, 2008 - 22:08
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | Quick fix |
Description
So, I admit that using reserved words for column names was a big mistake, but it was a typo, and I didn't know I had made it.
The gist of the bug is that db_change_field() uses backticks (`) to escape the new column name, but not the old column name. Since the column name is escaped when it is first created in drupal_install_schema(), it will gladly create a column that is a reserved word, but you can't change it after that.
Since using reserved words is an error, I'd like to change that column in an update function. But I can't.
Note that this issue is present in HEAD as well as D6.
#1
Here's the one-line patch to Drupal HEAD. I'll post the one for Drupal 6 after this one gets approved.
#2
#371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements
Its been an issue for a while.
#3
@earnie, I disagree. This is a much narrower issue.
#4
No, it isn't. Take a look at the patch. You'll find similar patches in the issue I pointed to. And we can't quote with ` because ` is MySql only.
#5
That's great because we are in a MySQL context.
#6
@earnie: it seems that we are taking the "don't use any of the reserved-words of any database engine and battle to change them if the list evolves" route. If we do, we have *no other choice* than to make *all* schema API operations reserved-word safe, because if we don't, we will not be able (as the original poster) to change the name of any column that uses a reserved word.
#7
This is actually a prerequisite for fixing the use of reserved words, since we can't fix the problem otherwise. So this issue is valid, the other should be marked "postponed".
Visually the patch looks fine, but I need to try applying it and testing it first. I'll try to get to that soon.
#8
You convinced me.
#9
Looks fine, applies fine. There's no Schema API unit tests yet, sadly, but this is trivial enough that I'm confident that it breaks nothing.
However, this only addresses MySQL. Can we give Postgres some love? (It is still a supported database, even if no one uses it.) Or should we push that off to a separate patch? (webchick/Dries, your call, given how lackluster Postgres interest is.)
#10
I'm not sure that Postgres is affected the same way by the bug. None of its specific Schema API functions appear to do anything with escaping reserved words in column names. (How would Postgres do that? Double quotes?)
Would it actually be better to restore symmetry in the other direction, as in removing the backticks? From my point of view, I would have been better off if I hadn't been allowed to make the mistake of creating a field called "values". But I don't know how that would affect core, or indeed, the rest of contrib.
#11
hm. I don't know Postgres well enough to know either way, and we're still lacking good schema API code coverage on unit tests to check it. (And I am scheming to rewrite Schema API anyway).
Given that there are reserved word columns in core now, and we will need Schema API to support them in order to convert away from them, we can't simply ignore the problem.
I think I'm actually going to push this up to RTBC. Committers, if you really want Postgres checked/fixed as part of this patch, find me a Postgres person who can. :-) Otherwise let's get MySQL taken care of and fix Postgres (if needed) separately.
#12
I committed this to CVS HEAD. I'm marking this 'active' as the PostgreSQL case needs fixing too.
#13
Marking accordingly.
#14
You shouldn't have a problem with PostgreSQL and reserved words in newer versions:
postgres=# \d type_test;Table "public.type_test"
Column | Type | Modifiers
--------+------+-----------
type | text |
type is a reserved word.
postgres=# select type from type_test;type
------
(0 rows)
If you want to be doubly sure (and conform to standard) you could do this:
postgres=# select "type" from type_test;type
------
(0 rows)
Oh and BTW don't be fooled, lots of people use Drupal + PostgreSQL.
#15
@linuxpoet: So the postgres driver is safe as-is, and doesn't need any more work? If so, let's mark this fixed. Or we can add " in the schema operations like we do ` for MySQL. Whichever you think is safer.
#16
Postgres will still complain if you use certain reserved words - GROUP comes to mind as an example:
postgres=# select group from mytable;
ERROR: syntax error at or near "group"
LINE 1: select group from mytable;
^
postgres=# select version();
version
----------------------------------------------------------------------------------
PostgreSQL 8.3.3 on i486-pc-linux-gnu, compiled by GCC cc (Debian 4.3.1-1) 4.3.1
(1 row)
#17
OK, so we need to quote fields in postgres, too. Is ' (apostrophe) the right character for that? If so, can you roll a quick patch a la the MySQL one above?
#18
No it would be the double quote " instead. The double quote character is the ANSI defined quoting character for tables and fields.
#19
Patch rolled for Postgres following earnie's suggestion that the double quote is the proper escaping character.
I only wrapped them around the name of the existing field, and not the new field to stay in sync with the MySQL patch above.
#20
The last submitted patch failed testing.
#21
Setting to 'needs review' - testbot was broken.
#22
The last submitted patch failed testing.
#23
tagging for next Postgres Code Sprint
#24
adding double quotes around the renamed old field. Doesn't hurt. Testbot shouldn't fail, it doesn't test PostgreSQL (yet), this is a no brainer to comit
#25
Committed to HEAD. Thanks.
#26
Thanks, everyone, for looking into this. Here's an equivalent patch for Drupal 6, for both MySQL and Postgres.
#27
Untag pgsql specificity.
#28
Island Usurper's patch in #26 is good. RTBC.
#29
Committed to Drupal 6 too, thanks.
#30
Automatically closed -- issue fixed for 2 weeks with no activity.