From b9e5859c6f956d25a73bea19133ab65aad41be62 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Sun, 10 May 2020 07:53:44 +0900
Subject: [PATCH] Make index options to kwargs

---
 .../abstract/schema_definitions.rb            | 30 +++++++++++--------
 .../abstract/schema_statements.rb             | 17 ++++-------
 .../abstract_mysql_adapter.rb                 | 11 +++----
 .../postgresql/schema_statements.rb           | 11 ++-----
 .../connection_adapters/sqlite3_adapter.rb    | 12 ++++----
 .../migration/command_recorder.rb             | 27 +++++++----------
 .../active_record/migration/compatibility.rb  | 18 +++++------
 .../adapters/postgresql/active_schema_test.rb |  1 +
 .../test/cases/invertible_migration_test.rb   |  2 ++
 .../test/cases/migration/change_table_test.rb |  9 ++++--
 .../cases/migration/command_recorder_test.rb  | 10 +++----
 11 files changed, 72 insertions(+), 76 deletions(-)

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
index 7e3b225c91..2957fb3a2e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -155,7 +155,7 @@ module ActiveRecord
         end
 
         if index
-          table.index(column_names, index_options)
+          table.index(column_names, **index_options)
         end
 
         if foreign_key
@@ -373,7 +373,7 @@ module ActiveRecord
       #     t.references :tagger, polymorphic: true
       #     t.references :taggable, polymorphic: { default: 'Photo' }, index: false
       #   end
-      def column(name, type, **options)
+      def column(name, type, index: nil, **options)
         name = name.to_s
         type = type.to_sym if type
 
@@ -385,9 +385,13 @@ module ActiveRecord
           end
         end
 
-        index_options = options.delete(:index)
-        index(name, index_options.is_a?(Hash) ? index_options : {}) if index_options
         @columns_hash[name] = new_column_definition(name, type, **options)
+
+        if index
+          index_options = index.is_a?(Hash) ? index : {}
+          index(name, **index_options)
+        end
+
         self
       end
 
@@ -401,7 +405,7 @@ module ActiveRecord
       # This is primarily used to track indexes that need to be created after the table
       #
       #   index(:account_id, name: 'index_projects_on_account_id')
-      def index(column_name, options = {})
+      def index(column_name, **options)
         indexes << [column_name, options]
       end
 
@@ -551,10 +555,12 @@ module ActiveRecord
       #  t.column(:name, :string)
       #
       # See TableDefinition#column for details of the options you can use.
-      def column(column_name, type, **options)
-        index_options = options.delete(:index)
+      def column(column_name, type, index: nil, **options)
         @base.add_column(name, column_name, type, **options)
-        index(column_name, index_options.is_a?(Hash) ? index_options : {}) if index_options
+        if index
+          index_options = index.is_a?(Hash) ? index : {}
+          index(column_name, **index_options)
+        end
       end
 
       # Checks to see if a column exists.
@@ -574,8 +580,8 @@ module ActiveRecord
       #  t.index([:branch_id, :party_id], unique: true, name: 'by_branch_party')
       #
       # See {connection.add_index}[rdoc-ref:SchemaStatements#add_index] for details of the options you can use.
-      def index(column_name, options = {})
-        @base.add_index(name, column_name, options)
+      def index(column_name, **options)
+        @base.add_index(name, column_name, **options)
       end
 
       # Checks to see if an index exists.
@@ -656,8 +662,8 @@ module ActiveRecord
       #   t.remove_index(:branch_id, name: :by_branch_party)
       #
       # See {connection.remove_index}[rdoc-ref:SchemaStatements#remove_index]
-      def remove_index(column_name = nil, options = {})
-        @base.remove_index(name, column_name, options)
+      def remove_index(column_name = nil, **options)
+        @base.remove_index(name, column_name, **options)
       end
 
       # Removes the timestamp columns (+created_at+ and +updated_at+) from the table.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index 1192cbc58b..4776fc56d6 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -97,7 +97,7 @@ module ActiveRecord
       #   # Check an index with a custom name exists
       #   index_exists?(:suppliers, :company_id, name: "idx_company_id")
       #
-      def index_exists?(table_name, column_name, options = {})
+      def index_exists?(table_name, column_name, **options)
         checks = []
 
         if column_name.present?
@@ -324,7 +324,7 @@ module ActiveRecord
 
         unless supports_indexes_in_create?
           td.indexes.each do |column_name, index_options|
-            add_index(table_name, column_name, index_options.merge!(if_not_exists: td.if_not_exists))
+            add_index(table_name, column_name, **index_options, if_not_exists: td.if_not_exists)
           end
         end
 
@@ -833,7 +833,7 @@ module ActiveRecord
       # Concurrently adding an index is not supported in a transaction.
       #
       # For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration].
-      def add_index(table_name, column_name, options = {})
+      def add_index(table_name, column_name, **options)
         index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
 
         create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
@@ -876,8 +876,8 @@ module ActiveRecord
       # Concurrently removing an index is not supported in a transaction.
       #
       # For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration].
-      def remove_index(table_name, column_name = nil, options = {})
-        return if options[:if_exists] && !index_exists?(table_name, column_name, options)
+      def remove_index(table_name, column_name = nil, **options)
+        return if options[:if_exists] && !index_exists?(table_name, column_name, **options)
 
         index_name = index_name_for_remove(table_name, column_name, options)
 
@@ -1334,17 +1334,12 @@ module ActiveRecord
         end
 
         def index_name_for_remove(table_name, column_name, options)
-          if column_name.is_a?(Hash)
-            options = column_name.dup
-            column_name = options.delete(:column)
-          end
-
           return options[:name] if can_remove_index_by_name?(column_name, options)
 
           checks = []
 
           checks << lambda { |i| i.name == options[:name].to_s } if options.key?(:name)
-          column_names = index_column_names(column_name)
+          column_names = index_column_names(column_name || options[:column])
 
           if column_names.present?
             checks << lambda { |i| index_name(table_name, i.columns) == index_name(table_name, column_names) }
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 697c6e5de7..5e683abd52 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -364,10 +364,11 @@ module ActiveRecord
         rename_column_indexes(table_name, column_name, new_column_name)
       end
 
-      def add_index(table_name, column_name, options = {}) #:nodoc:
-        index, algorithm, _ = add_index_options(table_name, column_name, **options)
+      def add_index(table_name, column_name, **options) #:nodoc:
+        index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
+
+        return if if_not_exists && index_exists?(table_name, column_name, name: index.name)
 
-        return if options[:if_not_exists] && index_exists?(table_name, column_name, name: index.name)
         create_index = CreateIndexDefinition.new(index, algorithm)
         execute schema_creation.accept(create_index)
       end
@@ -680,14 +681,14 @@ module ActiveRecord
           schema_creation.accept(ChangeColumnDefinition.new(cd, column.name))
         end
 
-        def add_index_for_alter(table_name, column_name, options = {})
+        def add_index_for_alter(table_name, column_name, **options)
           index, algorithm, _ = add_index_options(table_name, column_name, **options)
           algorithm = ", #{algorithm}" if algorithm
 
           "ADD #{schema_creation.accept(index)}#{algorithm}"
         end
 
-        def remove_index_for_alter(table_name, column_name = nil, options = {})
+        def remove_index_for_alter(table_name, column_name = nil, **options)
           index_name = index_name_for_remove(table_name, column_name, options)
           "DROP INDEX #{quote_column_name(index_name)}"
         end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index ba60ab1d67..6194a3bc75 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -441,7 +441,7 @@ module ActiveRecord
           rename_column_indexes(table_name, column_name, new_column_name)
         end
 
-        def add_index(table_name, column_name, options = {}) #:nodoc:
+        def add_index(table_name, column_name, **options) #:nodoc:
           index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
 
           create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
@@ -451,14 +451,9 @@ module ActiveRecord
           result
         end
 
-        def remove_index(table_name, column_name = nil, options = {}) # :nodoc:
+        def remove_index(table_name, column_name = nil, **options) # :nodoc:
           table = Utils.extract_schema_qualified_name(table_name.to_s)
 
-          if column_name.is_a?(Hash)
-            options = column_name.dup
-            column_name = options.delete(:column)
-          end
-
           if options.key?(:name)
             provided_index = Utils.extract_schema_qualified_name(options[:name].to_s)
 
@@ -470,7 +465,7 @@ module ActiveRecord
             end
           end
 
-          return if options[:if_exists] && !index_exists?(table_name, column_name, options)
+          return if options[:if_exists] && !index_exists?(table_name, column_name, **options)
 
           index_to_remove = PostgreSQL::Name.new(table.schema, index_name_for_remove(table.to_s, column_name, options))
 
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index b2548dae9a..d27fefcd99 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -219,8 +219,8 @@ module ActiveRecord
         pks.sort_by { |f| f["pk"] }.map { |f| f["name"] }
       end
 
-      def remove_index(table_name, column_name, options = {}) # :nodoc:
-        return if options[:if_exists] && !index_exists?(table_name, column_name, options)
+      def remove_index(table_name, column_name = nil, **options) # :nodoc:
+        return if options[:if_exists] && !index_exists?(table_name, column_name, **options)
 
         index_name = index_name_for_remove(table_name, column_name, options)
 
@@ -445,10 +445,10 @@ module ActiveRecord
 
             unless columns.empty?
               # index name can't be the same
-              opts = { name: name.gsub(/(^|_)(#{from})_/, "\\1#{to}_"), internal: true }
-              opts[:unique] = true if index.unique
-              opts[:where] = index.where if index.where
-              add_index(to, columns, opts)
+              options = { name: name.gsub(/(^|_)(#{from})_/, "\\1#{to}_"), internal: true }
+              options[:unique] = true if index.unique
+              options[:where] = index.where if index.where
+              add_index(to, columns, **options)
             end
           end
         end
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index f71f77883f..a2aede95af 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -132,6 +132,7 @@ module ActiveRecord
               create_table:      :drop_table,
               create_join_table: :drop_join_table,
               add_column:        :remove_column,
+              add_index:         :remove_index,
               add_timestamps:    :remove_timestamps,
               add_reference:     :remove_reference,
               add_foreign_key:   :remove_foreign_key,
@@ -192,30 +193,22 @@ module ActiveRecord
           [:rename_column, [args.first] + args.last(2).reverse]
         end
 
-        def invert_add_index(args)
-          table, columns, options = *args
-          options ||= {}
-
-          options_hash = options.slice(:name, :algorithm)
-          options_hash[:column] = columns if !options_hash[:name]
-
-          [:remove_index, [table, options_hash]]
-        end
-
         def invert_remove_index(args)
-          table, columns, options = *args
-          options ||= {}
+          options = args.extract_options!
+          table, columns = args
 
-          if columns.is_a?(Hash)
-            options = columns.dup
-            columns = options.delete(:column)
-          end
+          columns ||= options.delete(:column)
 
           unless columns
             raise ActiveRecord::IrreversibleMigration, "remove_index is only reversible if given a :column option."
           end
 
-          [:add_index, [table, columns, options]]
+          options.delete(:if_exists)
+
+          args = [table, columns]
+          args << options unless options.empty?
+
+          [:add_index, args]
         end
 
         alias :invert_add_belongs_to :invert_add_reference
diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb
index fb2fceeaf7..58063c7d53 100644
--- a/activerecord/lib/active_record/migration/compatibility.rb
+++ b/activerecord/lib/active_record/migration/compatibility.rb
@@ -197,7 +197,7 @@ module ActiveRecord
           super
         end
 
-        def index_exists?(table_name, column_name, options = {})
+        def index_exists?(table_name, column_name, **options)
           column_names = Array(column_name).map(&:to_s)
           options[:name] =
             if options[:name].present?
@@ -208,10 +208,9 @@ module ActiveRecord
           super
         end
 
-        def remove_index(table_name, options = {})
-          options = { column: options } unless options.is_a?(Hash)
-          options[:name] = index_name_for_remove(table_name, options)
-          super(table_name, options)
+        def remove_index(table_name, column_name = nil, **options)
+          options[:name] = index_name_for_remove(table_name, column_name, options)
+          super
         end
 
         private
@@ -222,13 +221,12 @@ module ActiveRecord
             super
           end
 
-          def index_name_for_remove(table_name, options = {})
-            index_name = connection.index_name(table_name, options)
+          def index_name_for_remove(table_name, column_name, options)
+            index_name = connection.index_name(table_name, column_name || options)
 
             unless connection.index_name_exists?(table_name, index_name)
-              if options.is_a?(Hash) && options.has_key?(:name)
-                options_without_column = options.dup
-                options_without_column.delete :column
+              if options.key?(:name)
+                options_without_column = options.except(:column)
                 index_name_without_column = connection.index_name(table_name, options_without_column)
 
                 if connection.index_name_exists?(table_name, index_name_without_column)
diff --git a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
index af23908acd..418acb5bc1 100644
--- a/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/active_schema_test.rb
@@ -106,4 +106,5 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase
     def method_missing(method_symbol, *arguments)
       ActiveRecord::Base.connection.send(method_symbol, *arguments)
     end
+    ruby2_keywords(:method_missing) if respond_to?(:ruby2_keywords, true)
 end
diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb
index 2f071e7488..6e783a5a92 100644
--- a/activerecord/test/cases/invertible_migration_test.rb
+++ b/activerecord/test/cases/invertible_migration_test.rb
@@ -86,6 +86,7 @@ module ActiveRecord
           t.column :name, :string
           t.column :color, :string
           t.index [:name, :color]
+          t.index [:color]
         end
       end
     end
@@ -94,6 +95,7 @@ module ActiveRecord
       def change
         change_table("horses") do |t|
           t.remove_index [:name, :color]
+          t.remove_index [:color], if_exists: true
         end
       end
     end
diff --git a/activerecord/test/cases/migration/change_table_test.rb b/activerecord/test/cases/migration/change_table_test.rb
index 370f3f3d54..b3bb55de60 100644
--- a/activerecord/test/cases/migration/change_table_test.rb
+++ b/activerecord/test/cases/migration/change_table_test.rb
@@ -217,17 +217,22 @@ module ActiveRecord
         with_change_table do |t|
           if RUBY_VERSION < "2.7"
             @connection.expect :add_column, nil, [:delete_me, :bar, :integer, {}]
+            @connection.expect :add_index, nil, [:delete_me, :bar, {}]
           else
             @connection.expect :add_column, nil, [:delete_me, :bar, :integer]
+            @connection.expect :add_index, nil, [:delete_me, :bar]
           end
-          @connection.expect :add_index, nil, [:delete_me, :bar, {}]
           t.column :bar, :integer, index: true
         end
       end
 
       def test_index_creates_index
         with_change_table do |t|
-          @connection.expect :add_index, nil, [:delete_me, :bar, {}]
+          if RUBY_VERSION < "2.7"
+            @connection.expect :add_index, nil, [:delete_me, :bar, {}]
+          else
+            @connection.expect :add_index, nil, [:delete_me, :bar]
+          end
           t.index :bar
         end
       end
diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb
index 03b641df60..392b9d031a 100644
--- a/activerecord/test/cases/migration/command_recorder_test.rb
+++ b/activerecord/test/cases/migration/command_recorder_test.rb
@@ -251,22 +251,22 @@ module ActiveRecord
 
       def test_invert_add_index
         remove = @recorder.inverse_of :add_index, [:table, [:one, :two]]
-        assert_equal [:remove_index, [:table, { column: [:one, :two] }]], remove
+        assert_equal [:remove_index, [:table, [:one, :two]], nil], 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, { name: "new_index" }]], remove
+        assert_equal [:remove_index, [:table, [:one, :two], name: "new_index"], nil], remove
       end
 
       def test_invert_add_index_with_algorithm_option
         remove = @recorder.inverse_of :add_index, [:table, :one, algorithm: :concurrently]
-        assert_equal [:remove_index, [:table, { column: :one, algorithm: :concurrently }]], remove
+        assert_equal [:remove_index, [:table, :one, algorithm: :concurrently], nil], remove
       end
 
       def test_invert_remove_index
         add = @recorder.inverse_of :remove_index, [:table, :one]
-        assert_equal [:add_index, [:table, :one, {}]], add
+        assert_equal [:add_index, [:table, :one]], add
       end
 
       def test_invert_remove_index_with_positional_column
@@ -286,7 +286,7 @@ module ActiveRecord
 
       def test_invert_remove_index_with_no_special_options
         add = @recorder.inverse_of :remove_index, [:table, { column: [:one, :two] }]
-        assert_equal [:add_index, [:table, [:one, :two], {}]], add
+        assert_equal [:add_index, [:table, [:one, :two]]], add
       end
 
       def test_invert_remove_index_with_no_column