Skip to content

Drop the correct index after reverting a migration #14034

Merged
merged 1 commit into from Feb 13, 2014

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.

@senny senny commented on an outdated diff Feb 13, 2014
...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
Ruby on Rails member
senny added a note Feb 13, 2014

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
Ruby on Rails member
senny commented Feb 13, 2014

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

@senny senny added the activerecord label Feb 13, 2014
@senny senny added this to the 4.0.3 milestone Feb 13, 2014
@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
Ruby on Rails member
senny commented Feb 13, 2014

@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 rails:master Feb 13, 2014

1 check passed

Details default The Travis CI build passed
@senny
Ruby on Rails member
senny commented Feb 13, 2014

@hdabrows thank you for your contribution!

@yahonda
yahonda commented Feb 16, 2014

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
Ruby on Rails member
senny commented Feb 22, 2014

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

@yahonda
yahonda commented Feb 24, 2014

@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
Something went wrong with that request. Please try again.