This project is archived and is in readonly mode.
[PATCH] Table name not escaped on dropping index
-
pupeno@pupeno.com- 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" -
pupeno@pupeno.com
- 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.08 KB
-
pupeno@pupeno.com
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...
-
Paul BarryLooks 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
-
pupeno@pupeno.com
Thanks Paul, I just updated my repo with your patch.
-
Étienne Barrié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_indexcould also be in anassert_nothing_raisedblock.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).
-
pupeno@pupeno.com
- 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...
-
Étienne Barrié
Paul added the
assert_nothing_raisedblock toremove_index, but not toadd_index.
You added the test to migration_test.rb but in a newTestCase, outside ofMigrationTest. 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 intest_add_index_length_limitfor
example. -
pupeno@pupeno.com
É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.
-
pupeno@pupeno.com
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
3.99 KB
-
-
Étienne Barrié
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.
-
pupeno@pupeno.com
Oh, right, I only worked in Rails 3 for this patch.
-
Norman Clarke- 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
- Importance changed from to Low
Can you rebase the patch to a single commit, rather than a group of three?
-
pupeno@pupeno.com
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
- Assigned user set to José Valim
- State changed from new to open
- Milestone cleared.
-
José ValimIt's ok to be three commits. I will apply it soon.
-
Repository
- 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... -
-
