implement rename_index natively for MySQL > 5.7 #13038

Merged
merged 1 commit into from Nov 28, 2013

Conversation

Projects
None yet
4 participants
Contributor

ccutrer commented Nov 25, 2013

No description provided.

@senny senny added a commit that referenced this pull request Nov 26, 2013

@senny senny `rename_index`: add the new index before removing the old one.
This prevents the following error when a MySQL index on a foreign key
column is renamed:

```
ActiveRecord::StatementInvalid: Mysql2::Error: Cannot drop index 'index_engines_on_car_id': needed in a foreign key constraint: DROP INDEX `index_engines_on_car_id` ON `engines`
```

refs: #13038.
6eba8d2
Owner

senny commented Nov 26, 2013

@ccutrer I pushed 6eba8d2 which changes the order of remove_index and add_index in the abstract adapter. This should allow you to call super in the else case.

@senny senny commented on an outdated diff Nov 26, 2013

activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Implement rename_index natively for MySQL > 5.7. Also prevent errors renaming indexes that support foreign keys.
@senny

senny Nov 26, 2013

Owner

can you wrap this to ~80 chars?

Owner

senny commented Nov 28, 2013

@ccutrer this does no longer apply cleanly. Can you push a rebased version?

Also please post a comment when you push the comments. This way I get a notification.

Contributor

ccutrer commented Nov 28, 2013

Rebased

senny merged commit cca7165 into rails:master Nov 28, 2013

1 check was pending

default The Travis CI build is in progress
Details

@egilburg egilburg commented on the diff Nov 28, 2013

..._record/connection_adapters/abstract_mysql_adapter.rb
@@ -487,6 +487,14 @@ def rename_table(table_name, new_name)
rename_table_indexes(table_name, new_name)
end
+ def rename_index(table_name, old_name, new_name)
+ if version[0] >= 5 && version[1] >= 7
@egilburg

egilburg Nov 28, 2013

Contributor

Will this work for e.g. mysql 6.0?

@chancancode

chancancode Nov 29, 2013

Owner

Also, along the same lines, the commit message says "> 5.7", but this logic here is ">= 5.7" (&& < 6)

@ccutrer

ccutrer Nov 29, 2013

Contributor

It needs to be fixed to allow 6+. Silly array version number. It should be

=5.7; I'll update the changelog in my next commit as well.

On Thursday, November 28, 2013, Godfrey Chan wrote:

In
activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:

@@ -487,6 +487,14 @@ def rename_table(table_name, new_name)
rename_table_indexes(table_name, new_name)
end

  •  def rename_index(table_name, old_name, new_name)
    
  •    if version[0] >= 5 && version[1] >= 7
    

Also, along the same lines, the commit message says "> 5.7", but this
logic here is ">= 5.7" (&& < 6)


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/13038/files#r7993765
.

@egilburg

egilburg Nov 29, 2013

Contributor

Is there some kind of Version object that allows queries such as:

Version.new(5, 6) >= "5.7" # => false

If not, it'd be pretty nice to have one!

@chancancode

chancancode Nov 29, 2013

Owner

There's Gem::Version from RubyGems that I always use for my own projects:

>> Gem::Version.new('5.7.1') >= Gem::Version.new('5.7')
=> true
>> Gem::Version.new('5.7') >= Gem::Version.new('5.7')
=> true
>> Gem::Version.new('6.0') >= Gem::Version.new('5.7')
=> true
>> Gem::Version.new('5.6') >= Gem::Version.new('5.7')
=> false

Not too sure about using that here though.

@senny

senny Nov 29, 2013

Owner

thanks for jumping in. We don't have a Rails specific Version object. Also by looking through the source there are very few actual version comparisons outside the AR adapters. I don't think we should depend on Gem::Version either. Let's keep using the version array until we see the need for a Version object in other parts of Rails.

@chancancode chancancode commented on the diff Nov 29, 2013

activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Implement rename_index natively for MySQL > 5.7.
@chancancode

chancancode Nov 29, 2013

Owner

This should be ">= 5.7" (if that's what you wanted)

ccutrer deleted the ccutrer:mysql_rename_index branch Nov 29, 2013

Contributor

ccutrer commented Nov 29, 2013

Fix is in #13093

@fluxusfrequency fluxusfrequency added a commit to fluxusfrequency/rails that referenced this pull request Dec 4, 2013

@senny @fluxusfrequency senny + fluxusfrequency `rename_index`: add the new index before removing the old one.
This prevents the following error when a MySQL index on a foreign key
column is renamed:

```
ActiveRecord::StatementInvalid: Mysql2::Error: Cannot drop index 'index_engines_on_car_id': needed in a foreign key constraint: DROP INDEX `index_engines_on_car_id` ON `engines`
```

refs: #13038.
77f958b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment