Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #1633 (closed defect: untested)

Opened 4 years ago

Last modified 2 years ago

[PATCH] quote_column_name in ActiveRecord::ConnectionAdapters AbstractAdapter/TableDefinition

Reported by: michael@schubert.cx Assigned to: Jeremy Kemper <rails@bitsweat.net>
Priority: normal Milestone: 1.2
Component: ActiveRecord Version: 0.13.1
Severity: normal Keywords: needy SQL quote quoting column names
Cc: michael@schubert.cx, bitsweat

Description

A simple patch is needed to make sure the column name is quoted (escaped) properly in TableDefinition's column function, which is used by the AbstractAdapter create_table method.

Otherwise when one calls create_table with column names that are reserved SQL words (e.g. "to"), a syntax error will be thrown. Argubly one should not use column names like that but we should not restrict the programmer from doing so if the SQL server does allow for it via escaping the column name.

Attachments

activerecord-abstract_adapter_rb.patch (0.9 kB) - added by michael@schubert.cx on 07/06/05 17:08:54.
Patch implementing described functionality
activerecord-abstract_adapter2_rb.patch (1.5 kB) - added by michael@schubert.cx on 07/06/05 19:31:53.
Patch for the other methods in AbstractAdapter
activerecord-mysql_adapter_rb.patch (1.3 kB) - added by michael@schubert.cx on 07/06/05 19:42:55.
Patch for MySQL Adapter to use quote_column_name
activerecord-postgresql_adapter_rb.patch (0.9 kB) - added by michael@schubert.cx on 07/06/05 19:43:24.
Patch for PostgreSQL Adapter to use quote_column_name
fixed-activerecord-abstract_adapter2_rb.patch (1.4 kB) - added by michael@schubert.cx on 07/06/05 20:16:41.
Fixed patch superceding activerecord-abstractadapter2_rb.patch
activerecord_1633.patch (4.8 kB) - added by michael@schubert.cx on 07/22/05 12:10:27.
Single patch unifying all previous patches
activerecord_1633-2.patch (6.0 kB) - added by michael@schubert.cx on 08/18/05 16:33:40.
New patch taking Rick Olson's changes from #1993 into consideration

Change History

07/06/05 17:08:54 changed by michael@schubert.cx

  • attachment activerecord-abstract_adapter_rb.patch added.

Patch implementing described functionality

07/06/05 19:31:15 changed by anonymous

  • summary changed from [PATCH] quote_column_name in ActiveRecord::ConnectionAdapters::TableDefinition column() to [PATCH] quote_column_name in ActiveRecord::ConnectionAdapters AbstractAdapter/TableDefinition.

07/06/05 19:31:53 changed by michael@schubert.cx

  • attachment activerecord-abstract_adapter2_rb.patch added.

Patch for the other methods in AbstractAdapter

07/06/05 19:42:55 changed by michael@schubert.cx

  • attachment activerecord-mysql_adapter_rb.patch added.

Patch for MySQL Adapter to use quote_column_name

07/06/05 19:43:24 changed by michael@schubert.cx

  • attachment activerecord-postgresql_adapter_rb.patch added.

Patch for PostgreSQL Adapter to use quote_column_name

07/06/05 19:45:57 changed by michael@schubert.cx

Okay I've added patches that should work for all the adapters that have their own methods where a column name is naked by itself in a SQL line, where it needs to be quoted to safely escape reserved words, and its just generally a good idea. I'm not sure where unit tests should be for this since it affects the connection adapters on a fundamental level (all the CREATE, ALTER stuff will change from this, thus it might break the return strings for some unit tests) I wil now run sqlite, postgres and mysql AR tests and if any tests need adjusted due the presence of the quoted columns I'll add those patches here.

07/06/05 20:09:53 changed by michael@schubert.cx

Running the test suite revealed a problem with the second patch to abstract adapter. I was assuming the column_name param was a single string, it can, in fact be an array (as used in migration_test.rb line 39). Therefore, lets not do a .split on the param, and just move on with .to_a.

07/06/05 20:16:41 changed by michael@schubert.cx

  • attachment fixed-activerecord-abstract_adapter2_rb.patch added.

Fixed patch superceding activerecord-abstractadapter2_rb.patch

07/07/05 15:18:56 changed by michael@schubert.cx

I re-ran the test-suite against MySQL, PostgreSQL and SQLite, no failures occured due to the quoting of column names. Verified via SQL logs that column names are quoted where appropiate

07/22/05 07:02:00 changed by david

Could you wrap all these up into one patch? That'd be great, thanks.

07/22/05 12:10:27 changed by michael@schubert.cx

  • attachment activerecord_1633.patch added.

Single patch unifying all previous patches

07/22/05 12:12:34 changed by michael@schubert.cx

Rolled up patch provided above. Let me know if you need anything else.

08/18/05 16:33:40 changed by michael@schubert.cx

  • attachment activerecord_1633-2.patch added.

New patch taking Rick Olson's changes from #1993 into consideration

08/18/05 16:34:33 changed by michael@schubert.cx

  • version set to 0.13.1.
  • severity changed from trivial to minor.
  • milestone set to 1.0.

I do wonder if we should quote the table name and if so... quote_column_name is misleading, it should simply be "quote_name"

11/21/05 16:33:28 changed by bitsweat

  • keywords changed from ActiveRecord, SQL to SQL quote quoting column names.
  • owner changed from David to bitsweat.
  • severity changed from minor to normal.

12/08/05 05:44:10 changed by bitsweat

  • cc changed from michael@schubert.cx to michael@schubert.cx, bitsweat.

Some of this has been implemented in order to support the Firebird adapter. Any volunteers to bring it up to date and perhaps quote table names as well?

12/08/05 23:25:42 changed by michael@schubert.cx

Since I really want to see this functionality completely and fully implemented for both columns and tables I'll take the task of updating it again, so long as you promise to get it in this time... pinky swear! ;-) I'll try to finish it by early next week.

Do you think there should be some specific unit tests to verify the correctness or do we assume that if the current 500+ AR tests that will implicitly be using quoted columns and table names reveal that things are probably okay otherwise all hell would break loose if it wasn't.

I suppose if we do have some tests we can look at SQL standards 92, 99 etc and picked specific reserved words from each and try to use them as columns and table names

09/03/06 22:08:39 changed by bitsweat

  • keywords changed from SQL quote quoting column names to needy SQL quote quoting column names.
  • status changed from new to closed.
  • resolution set to untested.