Pare back default index option for the migration generator

- Using `references` or `belongs_to` in migrations will always add index
  for the referenced column by default, without adding `index:true` option
  to generated migration file.
- Users can opt out of this by passing `index: false`.
- Legacy migrations won't be affected by this change. They will continue
  to run as they were before.
- Fixes #18146

activerecord/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Using `references` or `belongs_to` in migrations will always add index
+    for the referenced column by default, without adding `index: true` option
+    to generated migration file. Users can opt out of this by passing
+    `index: false`.
+
+    Fixes #18146.
+
+    *Matthew Draper*, *Prathamesh Sonpatki*
+
 *   Run `type` attributes through attributes API type-casting before
     instantiating the corresponding subclass. This makes it possible to define
     custom STI mappings.

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

@@ -69,7 +69,7 @@ class ReferenceDefinition # :nodoc:
       def initialize(
         name,
         polymorphic: false,
-        index: false,
+        index: true,
         foreign_key: false,
         type: :integer,
         **options

activerecord/lib/active_record/migration/compatibility.rb

@@ -5,6 +5,12 @@ module Compatibility # :nodoc: all
 
       module FourTwoShared
         module TableDefinition
+          def references(*, **options)
+            options[:index] ||= false
+            super
+          end
+          alias :belongs_to :references
+
           def timestamps(*, **options)
             options[:null] = true if options[:null].nil?
             super
@@ -24,6 +30,12 @@ class << t
           end
         end
 
+        def add_reference(*, **options)
+          options[:index] ||= false
+          super
+        end
+        alias :add_belongs_to :add_reference
+
         def add_timestamps(*, **options)
           options[:null] = true if options[:null].nil?
           super

activerecord/test/cases/migration/compatibility_test.rb

@@ -53,6 +53,24 @@ def migrate(x)
         ActiveRecord::Migrator.new(:up, [migration]).migrate
         assert_not connection.index_exists?(:testings, :bar)
       end
+
+      def test_references_does_not_add_index_by_default
+        migration = Class.new(ActiveRecord::Migration) {
+          def migrate(x)
+            create_table :more_testings do |t|
+              t.references :foo
+              t.belongs_to :bar, index: false
+            end
+          end
+        }.new
+
+        ActiveRecord::Migrator.new(:up, [migration]).migrate
+
+        assert_not connection.index_exists?(:more_testings, :foo_id)
+        assert_not connection.index_exists?(:more_testings, :bar_id)
+      ensure
+        connection.drop_table :more_testings rescue nil
+      end
     end
   end
 end

activerecord/test/cases/migration/references_foreign_key_test.rb

@@ -32,10 +32,10 @@ class ReferencesForeignKeyTest < ActiveRecord::TestCase
         assert_equal [], @connection.foreign_keys("testings")
       end
 
-      test "foreign keys can be created in one query" do
+      test "foreign keys can be created in one query when index is not added" do
         assert_queries(1) do
           @connection.create_table :testings do |t|
-            t.references :testing_parent, foreign_key: true
+            t.references :testing_parent, foreign_key: true, index: false
           end
         end
       end

activerecord/test/cases/migration/references_index_test.rb

@@ -23,12 +23,12 @@ def test_creates_index
         assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
       end
 
-      def test_does_not_create_index
+      def test_creates_index_by_default_even_if_index_option_is_not_passed
         connection.create_table table_name do |t|
           t.references :foo
         end
 
-        assert_not connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+        assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
       end
 
       def test_does_not_create_index_explicit
@@ -68,13 +68,13 @@ def test_creates_index_for_existing_table
         assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
       end
 
-      def test_does_not_create_index_for_existing_table
+      def test_creates_index_for_existing_table_even_if_index_option_is_not_passed
         connection.create_table table_name
         connection.change_table table_name do |t|
           t.references :foo
         end
 
-        assert_not connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
+        assert connection.index_exists?(table_name, :foo_id, :name => :index_testings_on_foo_id)
       end
 
       def test_does_not_create_index_for_existing_table_explicit

activerecord/test/cases/migration/references_statements_test.rb

@@ -30,14 +30,14 @@ def test_creates_reference_type_column
         assert column_exists?(table_name, :taggable_type, :string)
       end
 
-      def test_creates_reference_id_index
-        add_reference table_name, :user, index: true
-        assert index_exists?(table_name, :user_id)
+      def test_does_not_create_reference_id_index_if_index_is_false
+        add_reference table_name, :user, index: false
+        assert_not index_exists?(table_name, :user_id)
       end
 
-      def test_does_not_create_reference_id_index
+      def test_create_reference_id_index_even_if_index_option_is_passed
         add_reference table_name, :user
-        assert_not index_exists?(table_name, :user_id)
+        assert index_exists?(table_name, :user_id)
       end
 
       def test_creates_polymorphic_index

railties/lib/rails/generators/generated_attribute.rb

@@ -23,8 +23,9 @@ def parse(column_definition)
           type = type.to_sym if type
 
           if type && reference?(type)
-            references_index = UNIQ_INDEX_OPTIONS.include?(has_index) ? { unique: true } : true
-            attr_options[:index] = references_index
+            if UNIQ_INDEX_OPTIONS.include?(has_index)
+              attr_options[:index] = { unique: true }
+            end
           end
 
           new(name, type, has_index, attr_options)

railties/test/generators/migration_generator_test.rb

@@ -79,8 +79,8 @@ def test_remove_migration_with_references_options
 
     assert_migration "db/migrate/#{migration}.rb" do |content|
       assert_method :change, content do |change|
-        assert_match(/remove_reference :books, :author, index: true/, change)
-        assert_match(/remove_reference :books, :distributor, polymorphic: true, index: true/, change)
+        assert_match(/remove_reference :books, :author/, change)
+        assert_match(/remove_reference :books, :distributor, polymorphic: true/, change)
       end
     end
   end
@@ -166,8 +166,8 @@ def test_add_migration_with_references_options
 
     assert_migration "db/migrate/#{migration}.rb" do |content|
       assert_method :change, content do |change|
-        assert_match(/add_reference :books, :author, index: true/, change)
-        assert_match(/add_reference :books, :distributor, polymorphic: true, index: true/, change)
+        assert_match(/add_reference :books, :author/, change)
+        assert_match(/add_reference :books, :distributor, polymorphic: true/, change)
       end
     end
   end
@@ -178,8 +178,8 @@ def test_add_migration_with_required_references
 
     assert_migration "db/migrate/#{migration}.rb" do |content|
       assert_method :change, content do |change|
-        assert_match(/add_reference :books, :author, index: true, null: false/, change)
-        assert_match(/add_reference :books, :distributor, polymorphic: true, index: true, null: false/, change)
+        assert_match(/add_reference :books, :author, null: false/, change)
+        assert_match(/add_reference :books, :distributor, polymorphic: true, null: false/, change)
       end
     end
   end

railties/test/generators/model_generator_test.rb

@@ -345,26 +345,6 @@ def test_check_class_collision
     assert_match(/The name 'Object' is either already used in your application or reserved/, content)
   end
 
-  def test_index_is_added_for_belongs_to_association
-    run_generator ["account", "supplier:belongs_to"]
-
-    assert_migration "db/migrate/create_accounts.rb" do |m|
-      assert_method :change, m do |up|
-        assert_match(/index: true/, up)
-      end
-    end
-  end
-
-  def test_index_is_added_for_references_association
-    run_generator ["account", "supplier:references"]
-
-    assert_migration "db/migrate/create_accounts.rb" do |m|
-      assert_method :change, m do |up|
-        assert_match(/index: true/, up)
-      end
-    end
-  end
-
   def test_index_is_skipped_for_belongs_to_association
     run_generator ["account", "supplier:belongs_to", "--no-indexes"]
 

railties/test/generators/scaffold_generator_test.rb

@@ -14,8 +14,8 @@ def test_scaffold_on_invoke
     assert_file "app/models/product_line.rb", /class ProductLine < ActiveRecord::Base/
     assert_file "test/models/product_line_test.rb", /class ProductLineTest < ActiveSupport::TestCase/
     assert_file "test/fixtures/product_lines.yml"
-    assert_migration "db/migrate/create_product_lines.rb", /belongs_to :product, index: true/
-    assert_migration "db/migrate/create_product_lines.rb", /references :user, index: true/
+    assert_migration "db/migrate/create_product_lines.rb", /belongs_to :product/
+    assert_migration "db/migrate/create_product_lines.rb", /references :user/
 
     # Route
     assert_file "config/routes.rb" do |route|
@@ -94,8 +94,8 @@ def test_api_scaffold_on_invoke
     assert_file "app/models/product_line.rb", /class ProductLine < ActiveRecord::Base/
     assert_file "test/models/product_line_test.rb", /class ProductLineTest < ActiveSupport::TestCase/
     assert_file "test/fixtures/product_lines.yml"
-    assert_migration "db/migrate/create_product_lines.rb", /belongs_to :product, index: true/
-    assert_migration "db/migrate/create_product_lines.rb", /references :user, index: true/
+    assert_migration "db/migrate/create_product_lines.rb", /belongs_to :product/
+    assert_migration "db/migrate/create_product_lines.rb", /references :user/
 
     # Route
     assert_file "config/routes.rb" do |route|