Merge pull request #37161 from eugeneius/bulk_alter_recreate_index

Allow bulk alter to drop and recreate named index

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Allow bulk `ALTER` statements to drop and recreate indexes with the same name.
+
+    *Eugene Kenny*
+
 *   `insert`, `insert_all`, `upsert`, and `upsert_all` now clear the query cache.
 
     *Eugene Kenny*

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

@@ -1195,9 +1195,6 @@ def add_index_options(table_name, column_name, comment: nil, **options) # :nodoc
 
         validate_index_length!(table_name, index_name, options.fetch(:internal, false))
 
-        if data_source_exists?(table_name) && index_name_exists?(table_name, index_name)
-          raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' already exists"
-        end
         index_columns = quoted_columns_for_index(column_names, **options).join(", ")
 
         [index_name, index_type, index_columns, index_options, algorithm, using, comment]

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

@@ -27,10 +27,6 @@ def execute(sql, name = nil)
   end
 
   def test_add_index
-    # add_index calls data_source_exists? and index_name_exists? which can't work since execute is stubbed
-    def (ActiveRecord::Base.connection).data_source_exists?(*); true; end
-    def (ActiveRecord::Base.connection).index_name_exists?(*); false; end
-
     expected = "CREATE  INDEX `index_people_on_last_name`  ON `people` (`last_name`) "
     assert_equal expected, add_index(:people, :last_name, length: nil)
 
@@ -74,8 +70,6 @@ def (ActiveRecord::Base.connection).index_name_exists?(*); false; end
   end
 
   def test_index_in_create
-    def (ActiveRecord::Base.connection).data_source_exists?(*); false; end
-
     %w(SPATIAL FULLTEXT UNIQUE).each do |type|
       expected = /\ACREATE TABLE `people` \(#{type} INDEX `index_people_on_last_name`  \(`last_name`\)\)/
       actual = ActiveRecord::Base.connection.create_table(:people, id: false) do |t|
@@ -92,9 +86,6 @@ def (ActiveRecord::Base.connection).data_source_exists?(*); false; end
   end
 
   def test_index_in_bulk_change
-    def (ActiveRecord::Base.connection).data_source_exists?(*); true; end
-    def (ActiveRecord::Base.connection).index_name_exists?(*); false; end
-
     %w(SPATIAL FULLTEXT UNIQUE).each do |type|
       expected = "ALTER TABLE `people` ADD #{type} INDEX `index_people_on_last_name`  (`last_name`)"
       assert_sql(expected) do
@@ -170,19 +161,12 @@ def test_remove_timestamps
   end
 
   def test_indexes_in_create
-    assert_called_with(
-      ActiveRecord::Base.connection,
-      :data_source_exists?,
-      [:temp],
-      returns: false
-    ) do
-      expected = /\ACREATE TEMPORARY TABLE `temp` \( INDEX `index_temp_on_zip`  \(`zip`\)\)(?: ROW_FORMAT=DYNAMIC)? AS SELECT id, name, zip FROM a_really_complicated_query/
-      actual = ActiveRecord::Base.connection.create_table(:temp, temporary: true, as: "SELECT id, name, zip FROM a_really_complicated_query") do |t|
-        t.index :zip
-      end
-
-      assert_match expected, actual
+    expected = /\ACREATE TEMPORARY TABLE `temp` \( INDEX `index_temp_on_zip`  \(`zip`\)\)(?: ROW_FORMAT=DYNAMIC)? AS SELECT id, name, zip FROM a_really_complicated_query/
+    actual = ActiveRecord::Base.connection.create_table(:temp, temporary: true, as: "SELECT id, name, zip FROM a_really_complicated_query") do |t|
+      t.index :zip
     end
+
+    assert_match expected, actual
   end
 
   private

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

@@ -28,9 +28,6 @@ def test_create_database_with_collation_and_ctype
   end
 
   def test_add_index
-    # add_index calls index_name_exists? which can't work since execute is stubbed
-    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.define_method(:index_name_exists?) { |*| false }
-
     expected = %(CREATE UNIQUE INDEX  "index_people_on_last_name" ON "people"  ("last_name") WHERE state = 'active')
     assert_equal expected, add_index(:people, :last_name, unique: true, where: "state = 'active'")
 
@@ -73,8 +70,6 @@ def test_add_index
     assert_raise ArgumentError do
       add_index(:people, :last_name, algorithm: :copy)
     end
-
-    ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.remove_method :index_name_exists?
   end
 
   def test_remove_index

activerecord/test/cases/migration/index_test.rb

@@ -49,13 +49,6 @@ def test_rename_index_too_long
         assert connection.index_name_exists?(table_name, "old_idx")
       end
 
-      def test_double_add_index
-        connection.add_index(table_name, [:foo], name: "some_idx")
-        assert_raises(ArgumentError) {
-          connection.add_index(table_name, [:foo], name: "some_idx")
-        }
-      end
-
       def test_remove_nonexistent_index
         assert_raise(ArgumentError) { connection.remove_index(table_name, "no_such_index") }
       end

activerecord/test/cases/migration_test.rb

@@ -938,6 +938,34 @@ def test_changing_columns
       assert_equal "This is a comment", column(:birthdate).comment
     end
 
+    def test_changing_index
+      with_bulk_change_table do |t|
+        t.string :username
+        t.index :username, name: :username_index
+      end
+
+      assert index(:username_index)
+      assert_not index(:username_index).unique
+
+      classname = ActiveRecord::Base.connection.class.name[/[^:]*$/]
+      expected_query_count = {
+        "Mysql2Adapter"     => 1, # mysql2 supports dropping and creating two indexes using one statement
+        "PostgreSQLAdapter" => 2,
+      }.fetch(classname) {
+        raise "need an expected query count for #{classname}"
+      }
+
+      assert_queries(expected_query_count) do
+        with_bulk_change_table do |t|
+          t.remove_index name: :username_index
+          t.index :username, name: :username_index, unique: true
+        end
+      end
+
+      assert index(:username_index)
+      assert index(:username_index).unique
+    end
+
     private
       def with_bulk_change_table
         # Reset columns/indexes cache as we're changing the table