Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Drop the correct index after reverting a migration #14034

Merged
merged 1 commit into from

3 participants

@hdabrows

While working with partial indexes I've hit a problem where reverting a migration would drop the invalid index. The reason is that inverting add_index :table_name, :column_name, name: "named_index" results in remove_index :column_name, name: "named_index" which ends up dropping the index by its columns.

#8868 made remove_index :column_name, name: "named_index" drop the named_index only if a corresponding one with matching columns but without the name doesn't exist.

This change makes inverting an add_index use the name or the columns explicitly.

...ecord/lib/active_record/migration/command_recorder.rb
@@ -140,7 +140,11 @@ def invert_rename_column(args)
def invert_add_index(args)
table, columns, options = *args
- [:remove_index, [table, (options || {}).merge(column: columns)]]
+
+ index_name = options.try(:[], :name)
@senny Owner
senny added a note

can we use:

table, columns, options = *args
options ||= {}
index_name = options[:name]
# ...

instead of the try version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Owner

@hdabrows nice catch and good work! Added a minor comment. :yellow_heart:

@senny senny added the activerecord label
@senny senny added this to the 4.0.3 milestone
@hdabrows hdabrows Drop the correct index after reverting a migration
Previously when reverting a migration which added a named index it
would instead drop a corresponding index with matching columns but
without a name.
e1d4673
@hdabrows

@senny I've made the changes you've suggested. Is there any specific reason why you'd want to avoid try in this case?

@senny
Owner

@hdabrows I try to be explicit when I expect something to be nil. Also I did a quick grep through the project and found out that the Rails codebase does not really rely on try for such use-cases.

@senny senny merged commit ad48267 into from
@senny
Owner

@hdabrows thank you for your contribution!

@yahonda

This test do not work well with MySQL 5.7.3-m13 server and Oracle database.
For MySQL it would be probably because of the following change.
https://dev.mysql.com/doc/relnotes/mysql/5.7/en/news-5-7-0.html

The server now issues a warning if an index is created that duplicates an existing index, or an error in strict SQL mode. (Bug #37520, Bug #11748842)
  • MySQL server version
Server version: 5.7.3-m13 MySQL Community Server (GPL)
$ for i in mysql mysql2 oracle; do echo $i; ARCONN=$i ruby -Itest test/cases/invertible_migration_test.rb -n test_migrate_revert_add_index_with_name
> done
mysql
Using mysql
Run options: -n test_migrate_revert_add_index_with_name --seed 5115

# Running:

E

Finished in 0.064191s, 15.5784 runs/s, 0.0000 assertions/s.

  1) Error:
ActiveRecord::InvertibleMigrationTest#test_migrate_revert_add_index_with_name:
ActiveRecord::StatementInvalid: Mysql::Error: Duplicate index 'horses_index_named' defined on the table 'activerecord_unittest.horses'. This is deprecated and will be disallowed in a future release.: CREATE  INDEX `horses_index_named`  ON `horses` (`content`)
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `query'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `block in execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:373:in `block in log'
    /home/yahonda/git/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:367:in `log'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:523:in `add_index'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:649:in `block in method_missing'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `block in say_with_time'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `say_with_time'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:641:in `method_missing'
    test/cases/invertible_migration_test.rb:121:in `change'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:595:in `exec_migration'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:579:in `block (2 levels) in migrate'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:578:in `block in migrate'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:294:in `with_connection'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:577:in `migrate'
    test/cases/invertible_migration_test.rb:276:in `test_migrate_revert_add_index_with_name'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
mysql2
Using mysql2
Run options: -n test_migrate_revert_add_index_with_name --seed 9300

# Running:

E

Finished in 0.046106s, 21.6894 runs/s, 0.0000 assertions/s.

  1) Error:
ActiveRecord::InvertibleMigrationTest#test_migrate_revert_add_index_with_name:
ActiveRecord::StatementInvalid: Mysql2::Error: Duplicate index 'horses_index_named' defined on the table 'activerecord_unittest.horses'. This is deprecated and will be disallowed in a future release.: CREATE  INDEX `horses_index_named`  ON `horses` (`content`)
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `query'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `block in execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:373:in `block in log'
    /home/yahonda/git/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:367:in `log'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:301:in `execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:228:in `execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:523:in `add_index'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:649:in `block in method_missing'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `block in say_with_time'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `say_with_time'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:641:in `method_missing'
    test/cases/invertible_migration_test.rb:121:in `change'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:595:in `exec_migration'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:579:in `block (2 levels) in migrate'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:578:in `block in migrate'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:294:in `with_connection'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:577:in `migrate'
    test/cases/invertible_migration_test.rb:276:in `test_migrate_revert_add_index_with_name'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
oracle
Using oracle
Run options: -n test_migrate_revert_add_index_with_name --seed 35391

# Running:

E

Finished in 0.066850s, 14.9588 runs/s, 0.0000 assertions/s.

  1) Error:
ActiveRecord::InvertibleMigrationTest#test_migrate_revert_add_index_with_name:
ActiveRecord::StatementInvalid: OCIError: ORA-01408: such column list already indexed: CREATE  INDEX "HORSES_INDEX_NAMED" ON "HORSES" ("CONTENT")
    stmt.c:230:in oci8lib_210.so
    /home/yahonda/.rvm/gems/ruby-2.1.0@railsmaster/gems/ruby-oci8-2.1.7/lib/oci8/cursor.rb:129:in `exec'
    /home/yahonda/.rvm/gems/ruby-2.1.0@railsmaster/gems/ruby-oci8-2.1.7/lib/oci8/oci8.rb:278:in `exec_internal'
    /home/yahonda/.rvm/gems/ruby-2.1.0@railsmaster/gems/ruby-oci8-2.1.7/lib/oci8/oci8.rb:269:in `exec'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_oci_connection.rb:429:in `exec'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_oci_connection.rb:88:in `exec'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb:727:in `block in execute'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:373:in `block in log'
    /home/yahonda/git/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:367:in `log'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb:1500:in `log'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_adapter.rb:727:in `execute'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced_schema_statements.rb:189:in `add_index'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:649:in `block in method_missing'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `block in say_with_time'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:621:in `say_with_time'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:641:in `method_missing'
    test/cases/invertible_migration_test.rb:121:in `change'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:595:in `exec_migration'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:579:in `block (2 levels) in migrate'
    /home/yahonda/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/benchmark.rb:279:in `measure'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:578:in `block in migrate'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:294:in `with_connection'
    /home/yahonda/git/rails/activerecord/lib/active_record/migration.rb:577:in `migrate'
    test/cases/invertible_migration_test.rb:276:in `test_migrate_revert_add_index_with_name'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
$
@senny
Owner

@yahonda we can skip the tests for backends that do not support it.

@yahonda

@senny Thanks for the comment. #14177 has been opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 13, 2014
  1. @hdabrows

    Drop the correct index after reverting a migration

    hdabrows authored
    Previously when reverting a migration which added a named index it
    would instead drop a corresponding index with matching columns but
    without a name.
This page is out of date. Refresh to see the latest.
View
12 activerecord/CHANGELOG.md
@@ -1,3 +1,15 @@
+* When inverting add_index use the index name if present instead of
+ the columns.
+
+ If there are two indices with matching columns and one of them is
+ explicitly named then reverting the migration adding the named one
+ would instead drop the unnamed one.
+
+ The inversion of add_index will now drop the index by its name if
+ it is present.
+
+ *Hubert Dąbrowski*
+
* Add flag to disable schema dump after migration.
Add a config parameter on Active Record named `dump_schema_after_migration`
View
7 activerecord/lib/active_record/migration/command_recorder.rb
@@ -140,7 +140,12 @@ def invert_rename_column(args)
def invert_add_index(args)
table, columns, options = *args
- [:remove_index, [table, (options || {}).merge(column: columns)]]
+ options ||= {}
+
+ index_name = options[:name]
+ options_hash = index_name ? { name: index_name } : { column: columns }
+
+ [:remove_index, [table, options_hash]]
end
def invert_remove_index(args)
View
28 activerecord/test/cases/invertible_migration_test.rb
@@ -106,6 +106,22 @@ def change
end
end
+ class RevertNamedIndexMigration1 < SilentMigration
+ def change
+ create_table("horses") do |t|
+ t.column :content, :string
+ t.column :remind_at, :datetime
+ end
+ add_index :horses, :content
+ end
+ end
+
+ class RevertNamedIndexMigration2 < SilentMigration
+ def change
+ add_index :horses, :content, name: "horses_index_named"
+ end
+ end
+
def teardown
%w[horses new_horses].each do |table|
if ActiveRecord::Base.connection.table_exists?(table)
@@ -255,5 +271,17 @@ def test_migrate_down_with_table_name_prefix
ActiveRecord::Base.table_name_prefix = ActiveRecord::Base.table_name_suffix = ''
end
+ def test_migrate_revert_add_index_with_name
+ RevertNamedIndexMigration1.new.migrate(:up)
+ RevertNamedIndexMigration2.new.migrate(:up)
+ RevertNamedIndexMigration2.new.migrate(:down)
+
+ connection = ActiveRecord::Base.connection
+ assert connection.index_exists?(:horses, :content),
+ "index on content should exist"
+ assert !connection.index_exists?(:horses, :content, name: "horses_index_named"),
+ "horses_index_named index should not exist"
+ end
+
end
end
View
6 activerecord/test/cases/migration/command_recorder_test.rb
@@ -174,13 +174,13 @@ def test_invert_rename_column
end
def test_invert_add_index
- remove = @recorder.inverse_of :add_index, [:table, [:one, :two], options: true]
- assert_equal [:remove_index, [:table, {column: [:one, :two], options: true}]], remove
+ remove = @recorder.inverse_of :add_index, [:table, [:one, :two]]
+ assert_equal [:remove_index, [:table, {column: [:one, :two]}]], remove
end
def test_invert_add_index_with_name
remove = @recorder.inverse_of :add_index, [:table, [:one, :two], name: "new_index"]
- assert_equal [:remove_index, [:table, {column: [:one, :two], name: "new_index"}]], remove
+ assert_equal [:remove_index, [:table, {name: "new_index"}]], remove
end
def test_invert_add_index_with_no_options
Something went wrong with that request. Please try again.