From fd18b98dd90d738af265cf5b8f0d66ca11010132 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 6 Mar 2019 23:30:11 +0900
Subject: Allow `remove_foreign_key` with both `to_table` and `options`
Foreign keys could be created to the same table.
So `remove_foreign_key :from_table, :to_table` is sometimes ambiguous.
This allows `remove_foreign_key` to remove the select one on the same
table with giving both `to_table` and `options`.
---
.../connection_adapters/abstract/schema_definitions.rb | 10 +++-------
.../connection_adapters/abstract/schema_statements.rb | 18 +++++++++---------
.../postgresql/schema_statements.rb | 4 ++--
.../connection_adapters/sqlite3/schema_statements.rb | 2 +-
activerecord/lib/active_record/migration.rb | 2 +-
.../lib/active_record/migration/command_recorder.rb | 17 ++++-------------
activerecord/test/cases/migration/foreign_key_test.rb | 12 ++++++++++++
7 files changed, 32 insertions(+), 33 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 11d4b4a503..4861872129 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -105,13 +105,9 @@ module ActiveRecord
!ActiveRecord::SchemaDumper.fk_ignore_pattern.match?(name) if name
end
- def defined_for?(to_table_ord = nil, to_table: nil, **options)
- if to_table_ord
- self.to_table == to_table_ord.to_s
- else
- (to_table.nil? || to_table.to_s == self.to_table) &&
- options.all? { |k, v| self.options[k].to_s == v.to_s }
- end
+ def defined_for?(to_table: nil, **options)
+ (to_table.nil? || to_table.to_s == self.to_table) &&
+ options.all? { |k, v| self.options[k].to_s == v.to_s }
end
private
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 b2cc60f363..3dc81ffccf 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1002,10 +1002,10 @@ module ActiveRecord
# with an addition of
# [<tt>:to_table</tt>]
# The name of the table that contains the referenced primary key.
- def remove_foreign_key(from_table, options_or_to_table = {})
+ def remove_foreign_key(from_table, to_table = nil, **options)
return unless supports_foreign_keys?
- fk_name_to_delete = foreign_key_for!(from_table, options_or_to_table).name
+ fk_name_to_delete = foreign_key_for!(from_table, to_table: to_table, **options).name
at = create_alter_table from_table
at.drop_foreign_key fk_name_to_delete
@@ -1024,8 +1024,8 @@ module ActiveRecord
# # Checks to see if a foreign key with a custom name exists.
# foreign_key_exists?(:accounts, name: "special_fk_name")
#
- def foreign_key_exists?(from_table, options_or_to_table = {})
- foreign_key_for(from_table, options_or_to_table).present?
+ def foreign_key_exists?(from_table, to_table = nil, **options)
+ foreign_key_for(from_table, to_table: to_table, **options).present?
end
def foreign_key_column_for(table_name) # :nodoc:
@@ -1341,14 +1341,14 @@ module ActiveRecord
end
end
- def foreign_key_for(from_table, options_or_to_table = {})
+ def foreign_key_for(from_table, **options)
return unless supports_foreign_keys?
- foreign_keys(from_table).detect { |fk| fk.defined_for? options_or_to_table }
+ foreign_keys(from_table).detect { |fk| fk.defined_for?(options) }
end
- def foreign_key_for!(from_table, options_or_to_table = {})
- foreign_key_for(from_table, options_or_to_table) || \
- raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{options_or_to_table}")
+ def foreign_key_for!(from_table, to_table: nil, **options)
+ foreign_key_for(from_table, to_table: to_table, **options) ||
+ raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table || options}")
end
def extract_foreign_key_action(specifier)
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 d694a4f47d..a38c1325c0 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -623,10 +623,10 @@ module ActiveRecord
# validate_foreign_key :accounts, name: :special_fk_name
#
# The +options+ hash accepts the same keys as SchemaStatements#add_foreign_key.
- def validate_foreign_key(from_table, options_or_to_table = {})
+ def validate_foreign_key(from_table, to_table = nil, **options)
return unless supports_validate_constraints?
- fk_name_to_validate = foreign_key_for!(from_table, options_or_to_table).name
+ fk_name_to_validate = foreign_key_for!(from_table, to_table: to_table, **options).name
validate_constraint from_table, fk_name_to_validate
end
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
index bd649451d6..e64e995e1a 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
@@ -72,7 +72,7 @@ module ActiveRecord
table = strip_table_name_prefix_and_suffix(table)
fk_to_table = strip_table_name_prefix_and_suffix(fk.to_table)
fk_to_table == table && options.all? { |k, v| fk.options[k].to_s == v.to_s }
- end || raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table}")
+ end || raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table || options}")
foreign_keys.delete(fkey)
alter_table(from_table, foreign_keys)
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index c20274420f..997b7f763a 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -308,7 +308,7 @@ module ActiveRecord
# named +column_name+ from the table called +table_name+.
# * <tt>remove_columns(table_name, *column_names)</tt>: Removes the given
# columns from the table definition.
- # * <tt>remove_foreign_key(from_table, options_or_to_table)</tt>: Removes the
+ # * <tt>remove_foreign_key(from_table, to_table = nil, **options)</tt>: Removes the
# given foreign key from the table called +table_name+.
# * <tt>remove_index(table_name, column: column_names)</tt>: Removes the index
# specified by +column_names+.
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index 82f5121d94..8e7f596076 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -231,24 +231,15 @@ module ActiveRecord
end
def invert_remove_foreign_key(args)
- from_table, options_or_to_table, options_or_nil = args
+ options = args.extract_options!
+ from_table, to_table = args
- to_table = if options_or_to_table.is_a?(Hash)
- options_or_to_table[:to_table]
- else
- options_or_to_table
- end
-
- remove_options = if options_or_to_table.is_a?(Hash)
- options_or_to_table.except(:to_table)
- else
- options_or_nil
- end
+ to_table ||= options.delete(:to_table)
raise ActiveRecord::IrreversibleMigration, "remove_foreign_key is only reversible if given a second table" if to_table.nil?
reversed_args = [from_table, to_table]
- reversed_args << remove_options if remove_options.present?
+ reversed_args << options unless options.empty?
[:add_foreign_key, reversed_args]
end
diff --git a/activerecord/test/cases/migration/foreign_key_test.rb b/activerecord/test/cases/migration/foreign_key_test.rb
index 6547ebb5d1..da82f62406 100644
--- a/activerecord/test/cases/migration/foreign_key_test.rb
+++ b/activerecord/test/cases/migration/foreign_key_test.rb
@@ -385,6 +385,18 @@ if ActiveRecord::Base.connection.supports_foreign_keys?
assert_equal "Table 'astronauts' has no foreign key for rockets", e.message
end
+ def test_remove_foreign_key_by_the_select_one_on_the_same_table
+ @connection.add_foreign_key :astronauts, :rockets
+ @connection.add_reference :astronauts, :myrocket, foreign_key: { to_table: :rockets }
+
+ assert_equal 2, @connection.foreign_keys("astronauts").size
+
+ @connection.remove_foreign_key :astronauts, :rockets, column: "myrocket_id"
+
+ assert_equal [["astronauts", "rockets", "rocket_id"]],
+ @connection.foreign_keys("astronauts").map { |fk| [fk.from_table, fk.to_table, fk.column] }
+ end
+
if ActiveRecord::Base.connection.supports_validate_constraints?
def test_add_invalid_foreign_key
@connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", validate: false
--
cgit v1.2.3