Merge pull request rails#55086 from zzak/re-55078

Fix `remove_foreign_key` when multiple columns are found

Author: byroot

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

@@ -1815,7 +1815,20 @@ def foreign_key_name(table_name, options)
 
         def foreign_key_for(from_table, **options)
           return unless use_foreign_keys?
-          foreign_keys(from_table).detect { |fk| fk.defined_for?(**options) }
+
+          keys = foreign_keys(from_table)
+
+          if options[:_skip_column_match]
+            return keys.find { |fk| fk.defined_for?(**options) }
+          end
+
+          if options[:column].nil?
+            default_column = foreign_key_column_for(options[:to_table], "id")
+            matches = keys.select { |fk| fk.column == default_column }
+            keys = matches if matches.any?
+          end
+
+          keys.find { |fk| fk.defined_for?(**options) }
         end
 
         def foreign_key_for!(from_table, to_table: nil, **options)

activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb

@@ -63,23 +63,13 @@ def add_foreign_key(from_table, to_table, **options)
         end
 
         def remove_foreign_key(from_table, to_table = nil, **options)
-          return if options.delete(:if_exists) == true && !foreign_key_exists?(from_table, to_table, **options.slice(:column))
+          return if options.delete(:if_exists) && !foreign_key_exists?(from_table, to_table, **options.slice(:column))
 
           to_table ||= options[:to_table]
           options = options.except(:name, :to_table, :validate)
-          foreign_keys = foreign_keys(from_table)
-
-          fkey = foreign_keys.detect do |fk|
-            table = to_table || begin
-              table = options[:column].to_s.delete_suffix("_id")
-              Base.pluralize_table_names ? table.pluralize : table
-            end
-            table = strip_table_name_prefix_and_suffix(table)
-            options = options.slice(*fk.options.keys)
-            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 || options}")
+          fkey = foreign_key_for!(from_table, to_table: to_table, **options)
 
+          foreign_keys = foreign_keys(from_table)
           foreign_keys.delete(fkey)
           alter_table(from_table, foreign_keys)
         end

activerecord/lib/active_record/migration/compatibility.rb

@@ -32,6 +32,27 @@ def self.find(version)
       V8_1 = Current
 
       class V8_0 < V8_1
+        module RemoveForeignKeyColumnMatch
+          def remove_foreign_key(from_table, to_table = nil, **options)
+            options[:_skip_column_match] = true
+            super
+          end
+        end
+
+        module TableDefinition
+          def remove_foreign_key(to_table = nil, **options)
+            options[:_skip_column_match] = true
+            super
+          end
+        end
+
+        include RemoveForeignKeyColumnMatch
+
+        private
+          def compatible_table_definition(t)
+            t.singleton_class.prepend(TableDefinition)
+            super
+          end
       end
 
       class V7_2 < V8_0
@@ -157,9 +178,7 @@ def add_foreign_key(from_table, to_table, **options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
       end
@@ -220,9 +239,7 @@ def raise_on_if_exist_options(options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
       end
@@ -263,9 +280,7 @@ def add_reference(table_name, ref_name, **options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
       end
@@ -311,17 +326,13 @@ def add_timestamps(table_name, **options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
 
           def command_recorder
             recorder = super
-            class << recorder
-              prepend CommandRecorder
-            end
+            recorder.singleton_class.prepend(CommandRecorder)
             recorder
           end
       end
@@ -409,9 +420,7 @@ def add_reference(table_name, ref_name, **options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
       end
@@ -463,9 +472,7 @@ def remove_index(table_name, column_name = nil, **options)
 
         private
           def compatible_table_definition(t)
-            class << t
-              prepend TableDefinition
-            end
+            t.singleton_class.prepend(TableDefinition)
             super
           end
 

activerecord/test/cases/migration/compatibility_test.rb

@@ -664,6 +664,60 @@ def up
         end
       end
 
+      def test_remove_foreign_key_on_8_1
+        connection.create_table(:sub_testings) do |t|
+          t.references :testing, foreign_key: true, type: :bigint
+          t.references :experiment, foreign_key: { to_table: :testings }, type: :bigint
+        end
+
+        migration = Class.new(ActiveRecord::Migration[8.1]) do
+          def up
+            change_table(:sub_testings) do |t|
+              t.remove_foreign_key :testings
+              t.remove_foreign_key :testings, column: :experiment_id
+            end
+          end
+        end
+
+        ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
+
+        foreign_keys = @connection.foreign_keys("sub_testings")
+        assert_equal 0, foreign_keys.size
+      ensure
+        connection.drop_table(:sub_testings, if_exists: true)
+        ActiveRecord::Base.clear_cache!
+      end
+
+      def test_remove_foreign_key_on_8_0
+        connection.create_table(:sub_testings) do |t|
+          t.references :testing, foreign_key: true, type: :bigint
+          t.references :experiment, foreign_key: { to_table: :testings }, type: :bigint
+        end
+
+        migration = Class.new(ActiveRecord::Migration[8.0]) do
+          def up
+            change_table(:sub_testings) do |t|
+              t.remove_foreign_key :testings
+              t.remove_foreign_key :testings, column: :experiment_id
+            end
+          end
+        end
+
+        assert_raise(StandardError, match: /Table 'sub_testings' has no foreign key for testings$/) {
+          ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
+        }
+
+        foreign_keys = @connection.foreign_keys("sub_testings")
+        if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter)
+          assert_equal 2, foreign_keys.size
+        else
+          assert_equal 1, foreign_keys.size
+        end
+      ensure
+        connection.drop_table(:sub_testings, if_exists: true)
+        ActiveRecord::Base.clear_cache!
+      end
+
       private
         def precision_implicit_default
           if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)

activerecord/test/cases/migration/foreign_key_test.rb

@@ -260,6 +260,22 @@ def test_add_foreign_key_with_if_not_exists_considers_primary_key_option
           assert_equal ["id", "id_for_type_change"], foreign_keys.map(&:primary_key).sort
         end
 
+        def test_remove_foreign_key_with_multiple_keys_between_tables
+          @connection.add_column :astronauts, :backup_rocket_id, :bigint
+
+          @connection.add_foreign_key :astronauts, :rockets, column: :rocket_id
+          @connection.add_foreign_key :astronauts, :rockets, column: :backup_rocket_id
+
+          @connection.remove_foreign_key :astronauts, :rockets
+
+          foreign_keys = @connection.foreign_keys(:astronauts)
+          assert_equal 1, foreign_keys.size
+          assert_equal "backup_rocket_id", foreign_keys.first.column
+
+          @connection.remove_foreign_key :astronauts, :rockets, column: :backup_rocket_id
+          assert_empty @connection.foreign_keys(:astronauts)
+        end
+
         def test_add_foreign_key_with_non_standard_primary_key
           @connection.create_table :space_shuttles, id: false, force: true do |t|
             t.bigint :pk, primary_key: true
@@ -771,6 +787,38 @@ def test_add_foreign_key_with_suffix
           ActiveRecord::Base.table_name_suffix = nil
         end
 
+        def test_remove_foreign_key_with_prefix
+          ActiveRecord::Base.table_name_prefix = "p_"
+
+          migration = CreateSchoolsAndClassesMigration.new
+          silence_stream($stdout) { migration.migrate(:up) }
+          assert_equal 1, @connection.foreign_keys("p_classes").size
+
+          @connection.remove_foreign_key :p_classes
+
+          assert_empty @connection.foreign_keys("p_classes")
+        ensure
+          @connection.drop_table :p_classes
+          @connection.drop_table :p_schools
+          ActiveRecord::Base.table_name_prefix = nil
+        end
+
+        def test_remove_foreign_key_with_suffix
+          ActiveRecord::Base.table_name_suffix = "_s"
+
+          migration = CreateSchoolsAndClassesMigration.new
+          silence_stream($stdout) { migration.migrate(:up) }
+          assert_equal 1, @connection.foreign_keys("classes_s").size
+
+          @connection.remove_foreign_key :classes_s
+
+          assert_empty @connection.foreign_keys("classes_s")
+        ensure
+          @connection.drop_table :classes_s
+          @connection.drop_table :schools_s
+          ActiveRecord::Base.table_name_suffix = nil
+        end
+
         def test_remove_foreign_key_with_if_exists_not_set
           @connection.add_foreign_key :astronauts, :rockets
           assert_equal 1, @connection.foreign_keys("astronauts").size