Allow adding foreign key with a different name for the same column

lib/gitlab/database/migration_helpers.rb

@@ -164,19 +164,25 @@ module Gitlab
           raise 'add_concurrent_foreign_key can not be run inside a transaction'
         end
 
-        on_delete = 'SET NULL' if on_delete == :nullify
-
-        key_name = name || concurrent_foreign_key_name(source, column)
+        if name
+          key_name = name
+          foreign_key_exists = foreign_key_exists?(source, target, name: name)
+        else
+          key_name = concurrent_foreign_key_name(source, column)
+          foreign_key_exists = foreign_key_exists?(source, target, column: column)
+        end
 
-        unless foreign_key_exists?(source, target, column: column)
+        if foreign_key_exists
           Rails.logger.warn "Foreign key not created because it exists already " \
             "(this may be due to an aborted migration or similar): " \
             "source: #{source}, target: #{target}, column: #{column}"
-
+        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
           ALTER TABLE #{source}
           ADD CONSTRAINT #{key_name}
@@ -198,10 +204,12 @@ module Gitlab
       end
       # rubocop:enable Gitlab/RailsLogger
 
-      def foreign_key_exists?(source, target = nil, column: nil)
+      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

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -212,44 +212,75 @@ describe Gitlab::Database::MigrationHelpers do
         allow(model).to receive(:transaction_open?).and_return(false)
       end
 
-      it 'creates a concurrent foreign key and validates it' do
-        expect(model).to receive(:disable_statement_timeout).and_call_original
-        expect(model).to receive(:execute).with(/statement_timeout/)
-        expect(model).to receive(:execute).ordered.with(/NOT VALID/)
-        expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-        expect(model).to receive(:execute).with(/RESET ALL/)
+      context 'when no custom key name is supplied' do
+        it 'creates a concurrent foreign key and validates it' do
+          expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:execute).with(/statement_timeout/)
+          expect(model).to receive(:execute).ordered.with(/NOT VALID/)
+          expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
+          expect(model).to receive(:execute).with(/RESET ALL/)
 
-        model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
-      end
+          model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+        end
 
-      it 'appends a valid ON DELETE statement' do
-        expect(model).to receive(:disable_statement_timeout).and_call_original
-        expect(model).to receive(:execute).with(/statement_timeout/)
-        expect(model).to receive(:execute).with(/ON DELETE SET NULL/)
-        expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-        expect(model).to receive(:execute).with(/RESET ALL/)
+        it 'appends a valid ON DELETE statement' do
+          expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:execute).with(/statement_timeout/)
+          expect(model).to receive(:execute).with(/ON DELETE SET NULL/)
+          expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
+          expect(model).to receive(:execute).with(/RESET ALL/)
 
-        model.add_concurrent_foreign_key(:projects, :users,
-                                         column: :user_id,
-                                         on_delete: :nullify)
-      end
+          model.add_concurrent_foreign_key(:projects, :users,
+                                           column: :user_id,
+                                           on_delete: :nullify)
+        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).not_to receive(:execute).with(/ADD CONSTRAINT/)
-        expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
+        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).not_to receive(:execute).with(/ADD CONSTRAINT/)
+          expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
 
-        model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+          model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+        end
       end
 
-      it 'allows the use of a custom key name' do
-        expect(model).to receive(:disable_statement_timeout).and_call_original
-        expect(model).to receive(:execute).with(/statement_timeout/)
-        expect(model).to receive(:execute).ordered.with(/NOT VALID/)
-        expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+foo/)
-        expect(model).to receive(:execute).with(/RESET ALL/)
+      context 'when a custom key name is supplied' do
+        context 'for creating a new foreign key for a column that does not presently exist' do
+          it 'creates a new foreign key' do
+            expect(model).to receive(:disable_statement_timeout).and_call_original
+            expect(model).to receive(:execute).with(/statement_timeout/)
+            expect(model).to receive(:execute).ordered.with(/NOT VALID/)
+            expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+foo/)
+            expect(model).to receive(:execute).with(/RESET ALL/)
+
+            model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :foo)
+          end
+        end
+
+        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).not_to receive(:execute).with(/ADD CONSTRAINT/)
+              expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
 
-        model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :foo)
+              model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :foo)
+            end
+          end
+
+          context 'when the supplied key name is different from the existing foreign key name' do
+            it 'creates a new foreign key' do
+              expect(model).to receive(:disable_statement_timeout).and_call_original
+              expect(model).to receive(:execute).with(/statement_timeout/)
+              expect(model).to receive(:execute).ordered.with(/NOT VALID/)
+              expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+bar/)
+              expect(model).to receive(:execute).with(/RESET ALL/)
+
+              model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :bar)
+            end
+          end
+        end
       end
     end
   end
@@ -266,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 })
+      key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(:projects, :users, { column: :non_standard_id, name: :fk_projects_users_non_standard_id })
       allow(model).to receive(:foreign_keys).with(:projects).and_return([key])
     end
 
@@ -274,6 +305,10 @@ describe Gitlab::Database::MigrationHelpers do
       expect(model.foreign_key_exists?(:projects, :users, column: :non_standard_id)).to be_truthy
     end
 
+    it 'finds existing foreign keys by name' 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 target table only' do
       expect(model.foreign_key_exists?(:projects, :users)).to be_truthy
     end
@@ -282,6 +317,10 @@ describe Gitlab::Database::MigrationHelpers do
       expect(model.foreign_key_exists?(:projects, :users, column: :user_id)).to be_falsey
     end
 
+    it 'compares by foreign key name if given' do
+      expect(model.foreign_key_exists?(:projects, :users, name: :non_existent_foreign_key_name)).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