Check for all options

lib/gitlab/database/migration_helpers.rb

@@ -164,23 +164,29 @@ module Gitlab
           raise 'add_concurrent_foreign_key can not be run inside a transaction'
         end
 
+        options = {}
+        options[:on_delete] = on_delete
+
         if name
           key_name = name
-          foreign_key_exists = foreign_key_exists?(source, target, name: name)
+          options[:name] = name
         else
           key_name = concurrent_foreign_key_name(source, column)
-          foreign_key_exists = foreign_key_exists?(source, target, column: column)
+          options[:column] = column
         end
 
-        if foreign_key_exists
-          Rails.logger.warn "Foreign key not created because it exists already " \
+        if foreign_key_exists?(source, target, options)
+          warning_message = "Foreign key not created because it exists already " \
             "(this may be due to an aborted migration or similar): " \
-            "source: #{source}, target: #{target}, column: #{column}"
+            "source: #{source}, target: #{target}, column: #{column}, name: #{name}, on_delete: #{on_delete}"
+
+          Rails.logger.warn warning_message
         else
           # Using NOT VALID allows us to create a key without immediately
           # validating it. This means we keep the ALTER TABLE lock only for a
           # short period of time. The key _is_ enforced for any newly created
           # data.
+
           on_delete = 'SET NULL' if on_delete == :nullify
 
           execute <<-EOF.strip_heredoc
@@ -204,15 +210,10 @@ module Gitlab
       end
       # rubocop:enable Gitlab/RailsLogger
 
-      def foreign_key_exists?(source, target = nil, column: nil, name: nil)
-        foreign_keys(source).any? do |key|
-          if column
-            key.options[:column].to_s == column.to_s
-          elsif name
-            key.options[:name].to_s == name.to_s
-          else
-            key.to_table.to_s == target.to_s
-          end
+      def foreign_key_exists?(source, target = nil, **options)
+        foreign_keys(source).any? do |foreign_key|
+          (target.nil? || foreign_key.to_table.to_s == target.to_s) &&
+            options.all? { |k, v| foreign_key.options[k].to_s == v.to_s }
         end
       end
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -236,7 +236,7 @@ describe Gitlab::Database::MigrationHelpers do
         end
 
         it 'does not create a foreign key if it exists already' do
-          expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id).and_return(true)
+          expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id, on_delete: :cascade).and_return(true)
           expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/)
           expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
 
@@ -260,7 +260,7 @@ describe Gitlab::Database::MigrationHelpers do
         context 'for creating a duplicate foreign key for a column that presently exists' do
           context 'when the supplied key name is the same as the existing foreign key name' do
             it 'does not create a new foreign key' do
-              expect(model).to receive(:foreign_key_exists?).with(:projects, :users, name: :foo).and_return(true)
+              expect(model).to receive(:foreign_key_exists?).with(:projects, :users, name: :foo, on_delete: :cascade).and_return(true)
 
               expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/)
               expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
@@ -297,7 +297,7 @@ describe Gitlab::Database::MigrationHelpers do
 
   describe '#foreign_key_exists?' do
     before do
-      key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(:projects, :users, { column: :non_standard_id, name: :fk_projects_users_non_standard_id })
+      key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(:projects, :users, { column: :non_standard_id, name: :fk_projects_users_non_standard_id, on_delete: :cascade })
       allow(model).to receive(:foreign_keys).with(:projects).and_return([key])
     end
 
@@ -309,6 +309,14 @@ describe Gitlab::Database::MigrationHelpers do
       expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_non_standard_id)).to be_truthy
     end
 
+    it 'finds existing foreign_keys by name and column' do
+      expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_non_standard_id, column: :non_standard_id)).to be_truthy
+    end
+
+    it 'finds existing foreign_keys by name, column and on_delete' do
+      expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_non_standard_id, column: :non_standard_id, on_delete: :cascade)).to be_truthy
+    end
+
     it 'finds existing foreign keys by target table only' do
       expect(model.foreign_key_exists?(:projects, :users)).to be_truthy
     end
@@ -321,6 +329,14 @@ describe Gitlab::Database::MigrationHelpers do
       expect(model.foreign_key_exists?(:projects, :users, name: :non_existent_foreign_key_name)).to be_falsey
     end
 
+    it 'compares by foreign key name and column if given' do
+      expect(model.foreign_key_exists?(:projects, :users, name: :non_existent_foreign_key_name, column: :non_standard_id)).to be_falsey
+    end
+
+    it 'compares by foreign key name, column and on_delete if given' do
+      expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_non_standard_id, column: :non_standard_id, on_delete: :nullify)).to be_falsey
+    end
+
     it 'compares by target if no column given' do
       expect(model.foreign_key_exists?(:projects, :other_table)).to be_falsey
     end