diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 7ea7565f758a04ed12d464b0654651c8bd28147c..3df91459545950ed162b8d9deee5c1257badf936 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/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
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 449eee7a371b318a06dd203f87d04122d574033a..56e75fdbcb2fc0cb6ffb32cd2974674c226c66f6 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/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