Merge pull request #19456 from greysteil/index-exists-behaviour

Ignore index name in `index_exists?` when not passed a name to check for

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Ignore index name in `index_exists?` and `remove_index` when not passed a
+    name to check for.
+
+    *Grey Baker*
+
 *   Extract support for the legacy `mysql` database adapter from core. It will
     live on in a separate gem for now, but most users should just use `mysql2`.
 

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -82,11 +82,10 @@ module ActiveRecord
       #
       def index_exists?(table_name, column_name, options = {})
         column_names = Array(column_name).map(&:to_s)
-        index_name = options.key?(:name) ? options[:name].to_s : index_name(table_name, column: column_names)
         checks = []
-        checks << lambda { |i| i.name == index_name }
         checks << lambda { |i| i.columns == column_names }
         checks << lambda { |i| i.unique } if options[:unique]
+        checks << lambda { |i| i.name == options[:name].to_s } if options[:name]
 
         indexes(table_name).any? { |i| checks.all? { |check| check[i] } }
       end
@@ -700,15 +699,15 @@ module ActiveRecord
 
       # Removes the given index from the table.
       #
-      # Removes the +index_accounts_on_column+ in the +accounts+ table.
+      # Removes the index on +branch_id+ in the +accounts+ table if exactly one such index exists.
       #
       #   remove_index :accounts, :branch_id
       #
-      # Removes the index named +index_accounts_on_branch_id+ in the +accounts+ table.
+      # Removes the index on +branch_id+ in the +accounts+ table if exactly one such index exists.
       #
       #   remove_index :accounts, column: :branch_id
       #
-      # Removes the index named +index_accounts_on_branch_id_and_party_id+ in the +accounts+ table.
+      # Removes the index on +branch_id+ and +party_id+ in the +accounts+ table if exactly one such index exists.
       #
       #   remove_index :accounts, column: [:branch_id, :party_id]
       #
@@ -1127,21 +1126,35 @@ module ActiveRecord
         end
 
         def index_name_for_remove(table_name, options = {})
-          index_name = index_name(table_name, options)
+          # if the adapter doesn't support the indexes call the best we can do
+          # is return the default index name for the options provided
+          return index_name(table_name, options) unless respond_to?(:indexes)
 
-          unless index_name_exists?(table_name, index_name, true)
-            if options.is_a?(Hash) && options.has_key?(:name)
-              options_without_column = options.dup
-              options_without_column.delete :column
-              index_name_without_column = index_name(table_name, options_without_column)
+          checks = []
 
-              return index_name_without_column if index_name_exists?(table_name, index_name_without_column, false)
-            end
-
-            raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' does not exist"
+          if options.is_a?(Hash)
+            checks << lambda { |i| i.name == options[:name].to_s } if options.has_key?(:name)
+            column_names = Array(options[:column]).map(&:to_s)
+          else
+            column_names = Array(options).map(&:to_s)
           end
 
-          index_name
+          if column_names.any?
+            checks << lambda { |i| i.columns.join('_and_') == column_names.join('_and_') }
+          end
+
+          raise ArgumentError "No name or columns specified" if checks.none?
+
+          matching_indexes = indexes(table_name).select { |i| checks.all? { |check| check[i] } }
+
+          if matching_indexes.count > 1
+            raise ArgumentError, "Multiple indexes found on #{table_name} columns #{column_names}. " \
+                                 "Specify an index name from #{matching_indexes.map(&:name).join(', ')}"
+          elsif matching_indexes.none?
+            raise ArgumentError, "No indexes found on #{table_name} with the options provided."
+          else
+            matching_indexes.first.name
+          end
         end
 
         def rename_table_indexes(table_name, new_name)

activerecord/lib/active_record/migration/compatibility.rb

@@ -28,6 +28,42 @@ module ActiveRecord
           options[:null] = true if options[:null].nil?
           super
         end
+
+        def index_exists?(table_name, column_name, options = {})
+          column_names = Array(column_name).map(&:to_s)
+          options[:name] =
+            if options.key?(:name).present?
+              options[:name].to_s
+            else
+              index_name(table_name, column: column_names)
+            end
+          super
+        end
+
+        def remove_index(table_name, options = {})
+          index_name = index_name_for_remove(table_name, options)
+          execute "DROP INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)}"
+        end
+
+        private
+
+        def index_name_for_remove(table_name, options = {})
+          index_name = index_name(table_name, options)
+
+          unless index_name_exists?(table_name, index_name, true)
+            if options.is_a?(Hash) && options.has_key?(:name)
+              options_without_column = options.dup
+              options_without_column.delete :column
+              index_name_without_column = index_name(table_name, options_without_column)
+
+              return index_name_without_column if index_name_exists?(table_name, index_name_without_column, false)
+            end
+
+            raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' does not exist"
+          end
+
+          index_name
+        end
       end
 
       class V4_2 < V5_0

activerecord/test/cases/adapters/postgresql/active_schema_test.rb

@@ -54,8 +54,10 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase
   end
 
   def test_remove_index
-    # remove_index calls index_name_exists? which can't work since execute is stubbed
-    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send(:define_method, :index_name_exists?) { |*| true }
+    # remove_index calls index_name_for_remove which can't work since execute is stubbed
+    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send(:define_method, :index_name_for_remove) do |*|
+      'index_people_on_last_name'
+    end
 
     expected = %(DROP INDEX CONCURRENTLY "index_people_on_last_name")
     assert_equal expected, remove_index(:people, name: "index_people_on_last_name", algorithm: :concurrently)
@@ -64,7 +66,7 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase
       add_index(:people, :last_name, algorithm: :copy)
     end
 
-    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send :remove_method, :index_name_exists?
+    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.send :remove_method, :index_name_for_remove
   end
 
   private

activerecord/test/cases/adapters/postgresql/schema_test.rb

@@ -323,8 +323,6 @@ class SchemaTest < ActiveRecord::PostgreSQLTestCase
 
   def test_with_uppercase_index_name
     @connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)"
-    assert_nothing_raised { @connection.remove_index "things", name: "#{SCHEMA_NAME}.things_Index"}
-    @connection.execute "CREATE INDEX \"things_Index\" ON #{SCHEMA_NAME}.things (name)"
 
     with_schema_search_path SCHEMA_NAME do
       assert_nothing_raised { @connection.remove_index "things", name: "things_Index"}

activerecord/test/cases/migration/compatibility_test.rb

@@ -0,0 +1,42 @@
+require 'cases/helper'
+
+module ActiveRecord
+  class Migration
+    class CompatibilityTest < ActiveRecord::TestCase
+      attr_reader :connection
+      self.use_transactional_tests = false
+
+      def setup
+        super
+        @connection = ActiveRecord::Base.connection
+        @verbose_was = ActiveRecord::Migration.verbose
+        ActiveRecord::Migration.verbose = false
+
+        connection.create_table :testings do |t|
+          t.column :foo, :string, :limit => 100
+          t.column :bar, :string, :limit => 100
+        end
+      end
+
+      teardown do
+        connection.drop_table :testings rescue nil
+        ActiveRecord::Migration.verbose = @verbose_was
+      end
+
+      def test_migration_doesnt_remove_named_index
+        connection.add_index :testings, :foo, :name => "custom_index_name"
+
+        migration = Class.new(ActiveRecord::Migration[4.2]) {
+          def version; 101 end
+          def migrate(x)
+            remove_index :testings, :foo
+          end
+        }.new
+
+        assert connection.index_exists?(:testings, :foo, name: "custom_index_name")
+        assert_raise(StandardError) { ActiveRecord::Migrator.new(:up, [migration]).migrate }
+        assert connection.index_exists?(:testings, :foo, name: "custom_index_name")
+      end
+    end
+  end
+end

activerecord/test/cases/migration/index_test.rb

@@ -130,7 +130,17 @@ module ActiveRecord
       def test_named_index_exists
         connection.add_index :testings, :foo, :name => "custom_index_name"
 
+        assert connection.index_exists?(:testings, :foo)
         assert connection.index_exists?(:testings, :foo, :name => "custom_index_name")
+        assert !connection.index_exists?(:testings, :foo, :name => "other_index_name")
+      end
+
+      def test_remove_named_index
+        connection.add_index :testings, :foo, :name => "custom_index_name"
+
+        assert connection.index_exists?(:testings, :foo)
+        connection.remove_index :testings, :foo
+        assert !connection.index_exists?(:testings, :foo)
       end
 
       def test_add_index_attribute_length_limit