From d536ffd591d6a2363aaa1ad140f7b450e2e67ac6 Mon Sep 17 00:00:00 2001 From: Jess Bees Date: Fri, 29 Oct 2021 15:02:04 -0400 Subject: [PATCH] Raise an exception when using unrecognized options in change_table block In a database migration, the expressions `add_column`, `remove_index`, etc. accept as keyword options `if_exists: true`/`if_not_exists: true` which will skip that table alteration if the column or index does or does not already exist. This might lead some to think that within a change_table block, ``` change_table(:table) do |t| t.column :new_column, if_not_exists: true t.remove_index :old_column, if_exists: true end ``` also works, but it doesn't. Or rather, it is silently ignored when change_table is called with `bulk: true`, and it works accidentally otherwise. This commit raises an exception when these options are used in a change_table block, which suggests the similar syntax: `t.column :new_column unless t.column_exists?(:new_column)`. This suggestion is already made in the documentation to `ActiveRecord::ConnectionAdapters::Table`. https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/Table.html#method-i-column_exists-3F Do not raise these new exceptions on migrations before 7.0 --- .../abstract/schema_definitions.rb | 30 +++++++++++++++++-- .../active_record/migration/compatibility.rb | 12 ++++++++ .../test/cases/invertible_migration_test.rb | 2 +- .../test/cases/migration/change_table_test.rb | 18 ++++++++++- .../cases/migration/compatibility_test.rb | 14 +++++++++ 5 files changed, 72 insertions(+), 4 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 eccb49adb9..e88d1637f6 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -622,6 +622,7 @@ module ActiveRecord # # See TableDefinition#column for details of the options you can use. def column(column_name, type, index: nil, **options) + raise_on_if_exist_options(options) @base.add_column(name, column_name, type, **options) if index index_options = index.is_a?(Hash) ? index : {} @@ -647,6 +648,7 @@ module ActiveRecord # # See {connection.add_index}[rdoc-ref:SchemaStatements#add_index] for details of the options you can use. def index(column_name, **options) + raise_on_if_exist_options(options) @base.add_index(name, column_name, **options) end @@ -657,8 +659,8 @@ module ActiveRecord # end # # See {connection.index_exists?}[rdoc-ref:SchemaStatements#index_exists?] - def index_exists?(column_name, options = {}) - @base.index_exists?(name, column_name, options) + def index_exists?(column_name, **options) + @base.index_exists?(name, column_name, **options) end # Renames the given index on the table. @@ -676,6 +678,7 @@ module ActiveRecord # # See {connection.add_timestamps}[rdoc-ref:SchemaStatements#add_timestamps] def timestamps(**options) + raise_on_if_exist_options(options) @base.add_timestamps(name, **options) end @@ -686,6 +689,7 @@ module ActiveRecord # # See TableDefinition#column for details of the options you can use. def change(column_name, type, **options) + raise_on_if_exist_options(options) @base.change_column(name, column_name, type, **options) end @@ -717,6 +721,7 @@ module ActiveRecord # # See {connection.remove_columns}[rdoc-ref:SchemaStatements#remove_columns] def remove(*column_names, **options) + raise_on_if_exist_options(options) @base.remove_columns(name, *column_names, **options) end @@ -729,6 +734,7 @@ module ActiveRecord # # See {connection.remove_index}[rdoc-ref:SchemaStatements#remove_index] def remove_index(column_name = nil, **options) + raise_on_if_exist_options(options) @base.remove_index(name, column_name, **options) end @@ -757,6 +763,7 @@ module ActiveRecord # # See {connection.add_reference}[rdoc-ref:SchemaStatements#add_reference] for details of the options you can use. def references(*args, **options) + raise_on_if_exist_options(options) args.each do |ref_name| @base.add_reference(name, ref_name, **options) end @@ -770,6 +777,7 @@ module ActiveRecord # # See {connection.remove_reference}[rdoc-ref:SchemaStatements#remove_reference] def remove_references(*args, **options) + raise_on_if_exist_options(options) args.each do |ref_name| @base.remove_reference(name, ref_name, **options) end @@ -783,6 +791,7 @@ module ActiveRecord # # See {connection.add_foreign_key}[rdoc-ref:SchemaStatements#add_foreign_key] def foreign_key(*args, **options) + raise_on_if_exist_options(options) @base.add_foreign_key(name, *args, **options) end @@ -793,6 +802,7 @@ module ActiveRecord # # See {connection.remove_foreign_key}[rdoc-ref:SchemaStatements#remove_foreign_key] def remove_foreign_key(*args, **options) + raise_on_if_exist_options(options) @base.remove_foreign_key(name, *args, **options) end @@ -822,6 +832,22 @@ module ActiveRecord def remove_check_constraint(*args) @base.remove_check_constraint(name, *args) end + + private + def raise_on_if_exist_options(options) + unrecognized_option = options.keys.find do |key| + key == :if_exists || key == :if_not_exists + end + if unrecognized_option + conditional = unrecognized_option == :if_exists ? "if" : "unless" + message = <<~TXT + Option #{unrecognized_option} will be ignored. If you are calling an expression like + `t.column(.., #{unrecognized_option}: true)` from inside a change_table block, try a + conditional clause instead, as in `t.column(..) #{conditional} t.column_exists?(..)` + TXT + raise ArgumentError.new(message) + end + end end end end diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index 3e877ba725..524172a211 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -67,6 +67,14 @@ module ActiveRecord end end + def change_table(table_name, **options) + if block_given? + super { |t| yield compatible_table_definition(t) } + else + super + end + end + module TableDefinition def new_column_definition(name, type, **options) type = PostgreSQLCompat.compatible_timestamp_type(type, @conn) @@ -77,6 +85,10 @@ module ActiveRecord options[:precision] ||= nil super end + + private + def raise_on_if_exist_options(options) + end end private diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb index 9b2745f008..d35e4198f9 100644 --- a/activerecord/test/cases/invertible_migration_test.rb +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -96,7 +96,7 @@ module ActiveRecord def change change_table("horses") do |t| t.remove_index [:name, :color] - t.remove_index [:color], if_exists: true + t.remove_index [:color] if t.index_exists?(:color) end end end diff --git a/activerecord/test/cases/migration/change_table_test.rb b/activerecord/test/cases/migration/change_table_test.rb index 41f6146f9d..8930f0dea8 100644 --- a/activerecord/test/cases/migration/change_table_test.rb +++ b/activerecord/test/cases/migration/change_table_test.rb @@ -188,7 +188,7 @@ module ActiveRecord def test_index_exists with_change_table do |t| - @connection.expect :index_exists?, nil, [:delete_me, :bar, {}] + @connection.expect :index_exists?, nil, [:delete_me, :bar] t.index_exists?(:bar) end end @@ -289,6 +289,22 @@ module ActiveRecord t.remove_check_constraint name: "price_check" end end + + def test_remove_column_with_if_exists_raises_error + assert_raises(ArgumentError) do + with_change_table do |t| + t.remove :name, if_exists: true + end + end + end + + def test_add_column_with_if_not_exists_raises_error + assert_raises(ArgumentError) do + with_change_table do |t| + t.string :nickname, if_not_exists: true + end + end + end end end end diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb index 7ea05774d4..93adf8f64d 100644 --- a/activerecord/test/cases/migration/compatibility_test.rb +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -519,6 +519,20 @@ module ActiveRecord assert connection.column_exists?(:testings, :published_at, **precision_implicit_default) end + def test_change_table_allows_if_exists_option_on_6_1 + migration = Class.new(ActiveRecord::Migration[6.1]) { + def migrate(x) + change_table(:testings) do |t| + t.remove :foo, if_exists: true + end + end + }.new + + ActiveRecord::Migrator.new(:up, [migration], @schema_migration).migrate + + assert_not connection.column_exists?(:testings, :foo) + end + private def precision_implicit_default if current_adapter?(:Mysql2Adapter)