[XPATCH] SQL query not escaping table names

Setting version to 1.1.0 since the error message indicates ActiveRecord 1.14.0

Here's my patch to fix this. I've tried to follow test-driven patching as far as I could. Patch includes complete fix with unit tests proving it. Although, tests are ignored on any database other than MySQL, because I'm not aware about table name escaping rules for other databases. In two words, this patch just adds quote_table_name() method to MysqlAdapter and adds a simple accessor AR::Base.quoted_table_name() (just a wrapper around AR::Base.table_name()). I've gone step by step through Base, replacing table_name() with quoted_table_name() where it made sense. Pretty simple. Also dumb quote_table_name() method was added to ConnectionAdapters::Quoting module, to make sure all databases without specific table name quoting rules act as they did before.

Patch to fix table names escaping issues

Replying to Dmitry V. Sabanin <dmitry@sabanin.ru>:

Here's my patch to fix this.

Can you check this patch against trunk? I get unresolved conflicts when trying to apply it.

Is there anyone who can do review on this patch?

Please update the patch for application to trunk.

eallik, could you update this patch?

Why does quote_table_name duplicate the functionality of quote_column_name? Couldn't it be one function, or is it better to be two because other db systems may quote columns and table names different?

I modified the patch to apply cleanly to edge rails, tests all pass.

Although I have not reviewed the patch, assuming that the initial tests for this patch were sufficient.

I just found this patch after writing it myself on #4905, which is a duplicate of this ticket. There are a couple of differences:

1: You put the quote_table_name method in the right place, while I did not. Whoops...

2: My patch fixes Fixtures, while yours does not. This is, in my mind, the most important fix.

Any way to sneak my changes into this patch so they can get committed together? I can produce a single revised patch if it is helpful.

Geoff

Replying to gwcoffey:

Any way to sneak my changes into this patch so they can get committed together? I can produce a single revised patch if it is helpful.

Sure, go ahead; this goes without saying. When you update the patch here, close your own ticket as duplicate.

patched fixtures, some missed cases, added more test cases

+1

This is a long overdue patch. Thanks for your work!

This looks good to me. +1. Please update for trunk.

I spent part of this morning updating patch to apply to trunk but it currently causes some other migration related tests to fail. I'm working on fixing that now.

Looks like there is a double quoting issue introduced in the column method of the mysql_adapter in relation to the change_column method. patch should be ready soon.

Updated to apply to root of trunk, dried up some repeated sql in the test, fixed double quoting issue in columns

In v4 of the patch I added a regex statement to solve the double quoting issue in the columns method of the mysql adapter. All mysql unit tests pass on my system, could someone verify that this patch breaks nothing else?

moved double quote checking code from mysql_adapter.rb columns method to quote_table_name method; a litter easier to see what it's doing

I submitted a new patch that moves the double quote checking code from the columns method to a more logical place. heh, who'd've thought it should've been in the quote_table_name method.

I think I can give it a +1 now.

(In [7932]) Quote table names. Defaults to column quoting. Closes #4593.

Table names are not being quoted. I noticed it breaking when using a table named 'keys' under mysql.

Patches the calculations table. Looks like it got missed with the last patch.

(In [8061]) Escape table names during calculation queries. [wesley.moxam, Koz] Closes #4593

Table names are not being quoted in associations. Breaks mysql support.

ex: Foo.find(:all, :include => [:keys])

Tests to ensure that AR associations are working with reserved words

The newly attached patch doesn't have any tests, and this ticket is getting a little long in the tooth.

Could you add some tests then create a new ticket please, feel free to assign it straight to me.

I did add a test for the reserved words that does test this patch (since all of the tests were passing in it's broken state)

See reserved_words_test_mysql.diff

Also, if that test is not enough, what else is needed?

Sorry I missed that :/ . Typically I'd prefer one patch with tests and implementation.

This looks fine to go in, but anything new should get its own ticket, this one's getting hard to follow ;)

  1) Failure:
test_construct_finder_sql_applies_association_conditions(InnerJoinAssociationTest)
    [./test/associations/inner_join_association_test.rb:29:in `test_construct_finder_sql_applies_association_conditions'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:
<"SELECT `authors`.* FROM `authors`   INNER JOIN `categorizations` ON (`authors`.`id` = `categorizations`.`author_id`)  INNER JOIN `categories` ON (`categories`.`id` = `categorizations`.`category_id`) AND `categories`.`name` = 'General'  WHERE (TERMINATING_MARKER) "> expected to be =~
</INNER JOIN categories ON.*AND.*'General'.*TERMINATING_MARKER/>.

  2) Failure:
test_construct_finder_sql_cascades_inner_joins(InnerJoinAssociationTest)
    [./test/associations/inner_join_association_test.rb:18:in `test_construct_finder_sql_cascades_inner_joins'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:
<"SELECT `authors`.* FROM `authors`   INNER JOIN `posts` ON `posts`.author_id = `authors`.id  INNER JOIN `comments` ON `comments`.post_id = `posts`.id  "> expected to be =~
</INNER JOIN posts ON posts.author_id = authors.id/>.

  3) Failure:
test_construct_finder_sql_creates_inner_joins(InnerJoinAssociationTest)
    [./test/associations/inner_join_association_test.rb:13:in `test_construct_finder_sql_creates_inner_joins'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:
<"SELECT `authors`.* FROM `authors`   INNER JOIN `posts` ON `posts`.author_id = `authors`.id  "> expected to be =~
</INNER JOIN posts ON posts.author_id = authors.id/>.

  4) Failure:
test_construct_finder_sql_inner_joins_through_associations(InnerJoinAssociationTest)
    [./test/associations/inner_join_association_test.rb:24:in `test_construct_finder_sql_inner_joins_through_associations'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:
<"SELECT `authors`.* FROM `authors`   INNER JOIN `categorizations` ON (`authors`.`id` = `categorizations`.`author_id`)  INNER JOIN `posts` ON (`posts`.`id` = `categorizations`.`post_id`)  "> expected to be =~
</INNER JOIN categorizations.*INNER JOIN posts/>.

  5) Failure:
test_construct_finder_sql_unpacks_nested_joins(InnerJoinAssociationTest)
    [./test/associations/inner_join_association_test.rb:35:in `test_construct_finder_sql_unpacks_nested_joins'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Library/Ruby/Gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:
<"SELECT `authors`.* FROM `authors`   INNER JOIN `posts` ON `posts`.author_id = `authors`.id  INNER JOIN `comments` ON `comments`.post_id = `posts`.id  "> expected to be =~
</INNER JOIN posts ON posts.author_id = authors.id/>.

Except for those test failures when running rake test_mysql. Can you deal to those, create a new ticket and send it to me?

I noticed that those popped up after [8109]

I'll revisit it tomorrow and get you a new ticket.