#4809 [PATCH] Table name not escaped on dropping index - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4809 ✓resolved

[PATCH] Table name not escaped on dropping index

Reported by pupeno@pupeno.com | June 9th, 2010 @ 05:04 PM | in 3.0.2

While this works just fine:

add_index :values, [:field_id, :user_id]

this doesn't work:

remove_index :values, :column => [:field_id, :user_id]

failing with this error:

Mysql::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'values' at line 1: DROP INDEX index_values_on_field_id_and_user_id ON values

I think the issue is that values is not escaped at the end of that query, if it generated

DROP INDEX `index_values_on_field_id_and_user_id` ON `values`

it might work.

Comments and changes to this ticket

You flagged this item as spam.
pupeno@pupeno.com June 9th, 2010 @ 05:06 PM
- Tag changed from “mysql remove_index index” to “database, index, mysql, remove_index”
I workarrounded it this way:
```
remove_index "`values`", :name => "index_values_on_field_id_and_user_id"
```
You flagged this item as spam.
pupeno@pupeno.com June 12th, 2010 @ 02:37 AM
- Tag changed from “database, index, mysql, remove_index” to “database, index, mysql, patch, remove_index”
- Title changed from “Table name not escaped on dropping index” to “[PATCH] Table name not escaped on dropping index”
I'm attaching the patch that fixes this issue. It contains a test that shows the issue. I'm quite sure the way I wrote the test is not good, I've created another test class and I created the test as small and simple as possible. If it is not the correct way, can you tell me how you'd like me to do it and I'll make new patches.
- remove_index_from_values.patch 2.1 KB
You flagged this item as spam.
pupeno@pupeno.com June 12th, 2010 @ 02:38 AM
Oh, and it's also in this branch: http://github.com/pupeno/rails/tree/remove_index_from_values It's this commit: http://github.com/pupeno/rails/commit/13f9869896f5b39dd3558a2938ffe...
You flagged this item as spam.
Paul Barry June 12th, 2010 @ 04:05 AM
Looks good, it's definitely a bug that should be fixed. I've attached a new patch that slightly modifies the test so that it asserts that no exception is raised, rather than just stating that in a comment
- 4809-remove_index_not_escaped.patch 3.1 KB
You flagged this item as spam.
pupeno@pupeno.com June 12th, 2010 @ 11:05 AM
Thanks Paul, I just updated my repo with your patch.
You flagged this item as spam.
Étienne Barrié June 13th, 2010 @ 02:46 PM
It's a regression from http://github.com/rails/rails/commit/3809c80cd55ac2838f050346800889... (sorry about that).

I would have put the test along the other ones in MigrationTest, though. And the call to add_index could also be in an assert_nothing_raised block.

But the patch as it is solves the problem and works under 1.8.7 and 1.9.1 on master and 2-3-stable (cherry-picking raises a conflict, but it's easy to solve).
You flagged this item as spam.
pupeno@pupeno.com June 14th, 2010 @ 12:04 PM
- Tag changed from “database, index, mysql, patch, remove_index” to “database, index, migration, mysql, patch, remove_index”
Étienne,

Paul Barry added the assert_nothing_raised block. I'll look into the conflict.

Regarding MigrationTest, I should add the test here: activerecord/test/cases/migration_test.rb and then should I add a values table here: activerecord/test/schema/schema.rb?

With that second part I had some trouble: http://groups.google.com/group/rubyonrails-core/browse_thread/threa...
You flagged this item as spam.
Étienne Barrié June 14th, 2010 @ 01:19 PM
Paul added the assert_nothing_raised block to remove_index, but not to add_index.
You added the test to migration_test.rb but in a new TestCase, outside of MigrationTest. I would have put it inside, at line 160 for example.
And I don't think you need to add a new table, just add/remove indices with reserved names to an existing table, like it was done in test_add_index_length_limit for
example.
You flagged this item as spam.
pupeno@pupeno.com June 14th, 2010 @ 01:48 PM
Étienne, oh, got it regarding assert_nothing_raised. I'll fix it.

But the issue was the name of the table, not the name of the index. What wasn't being escaped was the name of the table, if the name of the table is not reserver it'll work just fine.
You flagged this item as spam.
pupeno@pupeno.com June 15th, 2010 @ 10:39 PM
New patch that applies cleanly (I actually couldn't find the conflict) and puts the add_index inside the no exceptions zone.
- remove_index_from_values.patch 4 KB
You flagged this item as spam.
Étienne Barrié June 16th, 2010 @ 08:42 AM
The conflict appears when you cherry-pick (or rebase --onto) your commits from master to 2-3-stable. But it's just a matter of removing the conflict markers.
You flagged this item as spam.
pupeno@pupeno.com June 16th, 2010 @ 08:50 AM
Oh, right, I only worked in Rails 3 for this patch.
You flagged this item as spam.
Norman Clarke June 29th, 2010 @ 01:19 PM
- Tag changed from “database, index, migration, mysql, patch, remove_index” to “database, index, migration, mysql, patch, remove_index, verified”
+1

I just applied this patch in my copy of rails master; it applies cleanly and all of the tests which pass in master pass with the patch applied on SQLite3, MySQL and Postgres.
Matt Jones June 29th, 2010 @ 02:50 PM
- Importance changed from “” to “Low”
Can you rebase the patch to a single commit, rather than a group of three?
You flagged this item as spam.
pupeno@pupeno.com June 29th, 2010 @ 03:57 PM
Matt,

Yes, I can do that. Wouldn't that remove authorship from Paul Barry, and if so, is it acceptable for me to do it?
Santiago Pastorino June 29th, 2010 @ 06:17 PM
- Assigned user set to “José Valim”
- State changed from “new” to “open”
- Milestone cleared.
José Valim June 29th, 2010 @ 06:26 PM
It's ok to be three commits. I will apply it soon.
Repository June 29th, 2010 @ 08:19 PM
- State changed from “open” to “resolved”
(from [21957b72ea394c679d9b17e75b570cc99596322d]) Test that adding an index also doesn't raise an exception.

[#4809 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/21957b72ea394c679d9b17e75b570c...
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Ryan Bigg November 8th, 2010 @ 01:52 AM
- Tag cleared.
Automatic cleanup of spam.