From 909818b93b8f1bd4d7053a1c5d8135b9b0cbe865 Mon Sep 17 00:00:00 2001 From: Prathamesh Sonpatki Date: Sun, 24 Jan 2016 16:46:12 +0530 Subject: 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 | 9 +++++++++ .../abstract/schema_definitions.rb | 2 +- .../lib/active_record/migration/compatibility.rb | 12 ++++++++++++ .../test/cases/migration/compatibility_test.rb | 18 ++++++++++++++++++ .../cases/migration/references_foreign_key_test.rb | 4 ++-- .../test/cases/migration/references_index_test.rb | 8 ++++---- .../cases/migration/references_statements_test.rb | 10 +++++----- railties/lib/rails/generators/generated_attribute.rb | 5 +++-- railties/test/generators/migration_generator_test.rb | 12 ++++++------ railties/test/generators/model_generator_test.rb | 20 -------------------- railties/test/generators/scaffold_generator_test.rb | 8 ++++---- 11 files changed, 64 insertions(+), 44 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 81799b65d6..f3dc26ddb3 100644 --- a/activerecord/CHANGELOG.md +++ b/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. 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 1cda23dc1d..690e0ba957 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -69,7 +69,7 @@ module ActiveRecord def initialize( name, polymorphic: false, - index: false, + index: true, foreign_key: false, type: :integer, **options diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index 1b94573870..5d742b523b 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -5,6 +5,12 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb index b1e1d72944..6a9cdd9d29 100644 --- a/activerecord/test/cases/migration/compatibility_test.rb +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -53,6 +53,24 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/references_foreign_key_test.rb b/activerecord/test/cases/migration/references_foreign_key_test.rb index edbc8abe4d..b01415afb2 100644 --- a/activerecord/test/cases/migration/references_foreign_key_test.rb +++ b/activerecord/test/cases/migration/references_foreign_key_test.rb @@ -32,10 +32,10 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/references_index_test.rb b/activerecord/test/cases/migration/references_index_test.rb index ad6b828d0b..a9a7f0f4c4 100644 --- a/activerecord/test/cases/migration/references_index_test.rb +++ b/activerecord/test/cases/migration/references_index_test.rb @@ -23,12 +23,12 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/migration/references_statements_test.rb b/activerecord/test/cases/migration/references_statements_test.rb index f613fd66c3..b9ce6bbc55 100644 --- a/activerecord/test/cases/migration/references_statements_test.rb +++ b/activerecord/test/cases/migration/references_statements_test.rb @@ -30,14 +30,14 @@ module ActiveRecord 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 diff --git a/railties/lib/rails/generators/generated_attribute.rb b/railties/lib/rails/generators/generated_attribute.rb index 8145a26e22..7e437e7344 100644 --- a/railties/lib/rails/generators/generated_attribute.rb +++ b/railties/lib/rails/generators/generated_attribute.rb @@ -23,8 +23,9 @@ module Rails 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) diff --git a/railties/test/generators/migration_generator_test.rb b/railties/test/generators/migration_generator_test.rb index 80f284674d..46154b7db2 100644 --- a/railties/test/generators/migration_generator_test.rb +++ b/railties/test/generators/migration_generator_test.rb @@ -79,8 +79,8 @@ class MigrationGeneratorTest < Rails::Generators::TestCase 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 @@ class MigrationGeneratorTest < Rails::Generators::TestCase 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 @@ class MigrationGeneratorTest < Rails::Generators::TestCase 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 diff --git a/railties/test/generators/model_generator_test.rb b/railties/test/generators/model_generator_test.rb index fb502ec0c5..814f4c050e 100644 --- a/railties/test/generators/model_generator_test.rb +++ b/railties/test/generators/model_generator_test.rb @@ -345,26 +345,6 @@ class ModelGeneratorTest < Rails::Generators::TestCase 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"] diff --git a/railties/test/generators/scaffold_generator_test.rb b/railties/test/generators/scaffold_generator_test.rb index eb81ea3d0e..6f7a83cae0 100644 --- a/railties/test/generators/scaffold_generator_test.rb +++ b/railties/test/generators/scaffold_generator_test.rb @@ -14,8 +14,8 @@ class ScaffoldGeneratorTest < Rails::Generators::TestCase 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 @@ class ScaffoldGeneratorTest < Rails::Generators::TestCase 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| -- cgit v1.2.3