Browse files

also rename indexes when a table or column is renamed

When a table or a column is renamed related indexes kept their name. This will lead to confusing names. This patch renames related indexes when a column or a table is renamed. Only indexes with names generated by rails will be renamed. Indexes with custom names will not be renamed.
  • Loading branch information...
1 parent 20ed3e0 commit 39eef1a565ef02e4dabc0811ef1bf4547ff9a60e @senny senny committed Dec 28, 2012
View
5 activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Rename related indexes on `rename_table` and `rename_column`. This
+ does not affect indexes with custom names.
+
+ *Yves Senn*
+
* Prevent the creation of indices with too long names, which cause
internal operations to fail (sqlite3 adapter only). The method
`allowed_index_name_length` defines the length limit enforced by
View
22 activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -696,6 +696,28 @@ def columns_for_remove(table_name, *column_names)
column_names.map {|column_name| quote_column_name(column_name) }
end
+ def rename_table_indexes(table_name, new_name)
+ indexes(new_name).each do |index|
+ generated_index_name = index_name(table_name, column: index.columns)
+ if generated_index_name == index.name
+ rename_index new_name, generated_index_name, index_name(new_name, column: index.columns)
+ end
+ end
+ end
+
+ def rename_column_indexes(table_name, column_name, new_column_name)
+ column_name, new_column_name = column_name.to_s, new_column_name.to_s
+ indexes(table_name).each do |index|
+ next unless index.columns.include?(new_column_name)
+ old_columns = index.columns.dup
+ old_columns[old_columns.index(new_column_name)] = column_name
+ generated_index_name = index_name(table_name, column: old_columns)
+ if generated_index_name == index.name
+ rename_index table_name, generated_index_name, index_name(table_name, column: index.columns)
+ end
+ end
+ end
+
private
def table_definition
TableDefinition.new(self)
View
2 activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -468,6 +468,7 @@ def bulk_change_table(table_name, operations) #:nodoc:
# rename_table('octopuses', 'octopi')
def rename_table(table_name, new_name)
execute "RENAME TABLE #{quote_table_name(table_name)} TO #{quote_table_name(new_name)}"
+ rename_table_indexes(table_name, new_name)
end
def add_column(table_name, column_name, type, options = {})
@@ -495,6 +496,7 @@ def change_column(table_name, column_name, type, options = {}) #:nodoc:
def rename_column(table_name, column_name, new_column_name) #:nodoc:
execute("ALTER TABLE #{quote_table_name(table_name)} #{rename_column_sql(table_name, column_name, new_column_name)}")
+ rename_column_indexes(table_name, column_name, new_column_name)
end
# Maps logical Rails types to MySQL-specific data types.
View
9 activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -321,14 +321,16 @@ def primary_key(table)
#
# Example:
# rename_table('octopuses', 'octopi')
- def rename_table(name, new_name)
+ def rename_table(table_name, new_name)
clear_cache!
- execute "ALTER TABLE #{quote_table_name(name)} RENAME TO #{quote_table_name(new_name)}"
+ execute "ALTER TABLE #{quote_table_name(table_name)} RENAME TO #{quote_table_name(new_name)}"
pk, seq = pk_and_sequence_for(new_name)
- if seq == "#{name}_#{pk}_seq"
+ if seq == "#{table_name}_#{pk}_seq"
new_seq = "#{new_name}_#{pk}_seq"
execute "ALTER TABLE #{quote_table_name(seq)} RENAME TO #{quote_table_name(new_seq)}"
end
+
+ rename_table_indexes(table_name, new_name)
end
# Adds a new column to the named table.
@@ -370,6 +372,7 @@ def change_column_null(table_name, column_name, null, default = nil)
def rename_column(table_name, column_name, new_column_name)
clear_cache!
execute "ALTER TABLE #{quote_table_name(table_name)} RENAME COLUMN #{quote_column_name(column_name)} TO #{quote_column_name(new_column_name)}"
+ rename_column_indexes(table_name, column_name, new_column_name)
end
def remove_index!(table_name, index_name) #:nodoc:
View
6 activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -435,8 +435,9 @@ def remove_index!(table_name, index_name) #:nodoc:
#
# Example:
# rename_table('octopuses', 'octopi')
- def rename_table(name, new_name)
- exec_query "ALTER TABLE #{quote_table_name(name)} RENAME TO #{quote_table_name(new_name)}"
+ def rename_table(table_name, new_name)
+ exec_query "ALTER TABLE #{quote_table_name(table_name)} RENAME TO #{quote_table_name(new_name)}"
+ rename_table_indexes(table_name, new_name)
end
# See: http://www.sqlite.org/lang_altertable.html
@@ -495,6 +496,7 @@ def rename_column(table_name, column_name, new_column_name) #:nodoc:
raise ActiveRecord::ActiveRecordError, "Missing column #{table_name}.#{column_name}"
end
alter_table(table_name, :rename => {column_name.to_s => new_column_name.to_s})
+ rename_column_indexes(table_name, column_name, new_column_name)
end
protected
View
25 activerecord/test/cases/migration/rename_column_test.rb
@@ -86,8 +86,29 @@ def test_rename_column_with_an_index
assert_equal 1, connection.indexes('test_models').size
rename_column "test_models", "hat_name", "name"
- # FIXME: should we rename the index if it's name was autogenerated by rails?
- assert_equal ['index_test_models_on_hat_name'], connection.indexes('test_models').map(&:name)
+
+ assert_equal ['index_test_models_on_name'], connection.indexes('test_models').map(&:name)
+ end
+
+ def test_rename_column_with_multi_column_index
+ add_column "test_models", :hat_size, :integer
+ add_column "test_models", :hat_style, :string, limit: 100
+ add_index "test_models", ["hat_style", "hat_size"], unique: true
+
+ rename_column "test_models", "hat_size", 'size'
+ assert_equal ['index_test_models_on_hat_style_and_size'], connection.indexes('test_models').map(&:name)
+
+ rename_column "test_models", "hat_style", 'style'
+ assert_equal ['index_test_models_on_style_and_size'], connection.indexes('test_models').map(&:name)
+ end
+
+ def test_rename_column_does_not_rename_custom_named_index
+ add_column "test_models", :hat_name, :string
+ add_index :test_models, :hat_name, :name => 'idx_hat_name'
+
+ assert_equal 1, connection.indexes('test_models').size
+ rename_column "test_models", "hat_name", "name"
+ assert_equal ['idx_hat_name'], connection.indexes('test_models').map(&:name)
end
def test_remove_column_with_index
View
12 activerecord/test/cases/migration/rename_table_test.rb
@@ -63,7 +63,17 @@ def test_rename_table_with_an_index
connection.enable_identity_insert("octopi", false) if current_adapter?(:SybaseAdapter)
assert_equal 'http://www.foreverflying.com/octopus-black7.jpg', connection.select_value("SELECT url FROM octopi WHERE id=1")
- assert connection.indexes(:octopi).first.columns.include?("url")
+ index = connection.indexes(:octopi).first
+ assert index.columns.include?("url")
+ assert_equal 'index_octopi_on_url', index.name
+ end
+
+ def test_rename_table_does_not_rename_custom_named_index
+ add_index :test_models, :url, name: 'special_url_idx'
+
+ rename_table :test_models, :octopi
+
+ assert_equal ['special_url_idx'], connection.indexes(:octopi).map(&:name)
end
def test_rename_table_for_postgresql_should_also_rename_default_sequence

7 comments on commit 39eef1a

@take

Hey @senny , I was working on a Rails project which was recently updated from 3.2 to 4.0.

And due to this commit, the following migration files started to fail

class RenameNameToDisplayName < ActiveRecord::Migration
  def self.up
    rename_column :users, :name, :display_name
  end

  def self.down
    rename_column :users, :display_name, :name
  end
end
class RenameOldIndex < ActiveRecord::Migration
  def self.up
    remove_index :users, :name
    add_index :users, :display_name
  end

  def self.down
    remove_index :users, :display_name
  end
end

With a error message

Index name 'index_users_on_name' on table 'users' does not

In these cases do u think we should just delete the second migration file(index renaming migration file) ?

If so, maybe we should add info to http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html ...?

Would help for feedbacks 😃

@senny
Ruby on Rails member

Hey @tkhr thanks for reporting 💛

As the work of your second migration file is now performed by Rails automatically I don't see a reason to keep it around. Already migrated databases shouldn't complain either.

@rafaelfranca @carlosantoniodasilva thoughts?

@take

As the work of your second migration file is now performed by Rails automatically I don't see a reason to keep it around. Already migrated databases shouldn't complain either.

agree 👍

@take

Oh sorry http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html is mentioning this in the AR section about this change

In Rails 4.0 when a column or a table is renamed the related indexes are also renamed. If you have migrations which rename the indexes, they are no longer needed.

@senny
Ruby on Rails member

@tkhr 😄 this was changed because of this comment of yours 💛

@take

oh i c thx 😃 !

@take

Hey I realized that due to this commit, there's a possibility that some projects which has been updated to 4.0 will encounter to a situation which the production db and the schema.rb aren't the same.

I'll post about the details later today, but since the post is gonna be long, I just wanted to tell that there's some further problem expect the first comment's problem. 😃

Please sign in to comment.