Add explicit options

lib/gitlab/database/migration_helpers.rb

@@ -155,6 +155,7 @@ module Gitlab
       # column - The name of the column to create the foreign key on.
       # on_delete - The action to perform when associated data is removed,
       #             defaults to "CASCADE".
+      # name - The name of the foreign key.
       #
       # rubocop:disable Gitlab/RailsLogger
       def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, name: nil)
@@ -164,21 +165,17 @@ 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
-          options[:name] = name
-        else
-          key_name = concurrent_foreign_key_name(source, column)
-          options[:column] = column
-        end
+        options = {
+          column: column,
+          on_delete: on_delete,
+          name: name.presence || concurrent_foreign_key_name(source, column)
+        }
 
         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}, name: #{name}, on_delete: #{on_delete}"
+            "source: #{source}, target: #{target}, column: #{options[:column]}, "\
+            "name: #{options[:name]}, on_delete: #{options[:on_delete]}"
 
           Rails.logger.warn warning_message
         else
@@ -187,14 +184,12 @@ module Gitlab
           # 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}
-          FOREIGN KEY (#{column})
+          ADD CONSTRAINT #{options[:name]}
+          FOREIGN KEY (#{options[:column]})
           REFERENCES #{target} (id)
-          #{on_delete ? "ON DELETE #{on_delete.upcase}" : ''}
+          #{on_delete_statement(options[:on_delete])}
           NOT VALID;
           EOF
         end
@@ -205,15 +200,15 @@ module Gitlab
         #
         # Note this is a no-op in case the constraint is VALID already
         disable_statement_timeout do
-          execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{key_name};")
+          execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{options[:name]};")
         end
       end
       # rubocop:enable Gitlab/RailsLogger
 
       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 }
+          tables_match?(target.to_s, foreign_key.to_table.to_s) &&
+            options_match?(foreign_key.options, options)
         end
       end
 
@@ -1059,6 +1054,21 @@ into similar problems in the future (e.g. when new tables are created).
 
       private
 
+      def tables_match?(target_table, foreign_key_table)
+        target_table.blank? || foreign_key_table == target_table
+      end
+
+      def options_match?(foreign_key_options, options)
+        options.all? { |k, v| foreign_key_options[k].to_s == v.to_s }
+      end
+
+      def on_delete_statement(on_delete)
+        return '' if on_delete.blank?
+        return 'ON DELETE SET NULL' if on_delete == :nullify
+
+        "ON DELETE #{on_delete.upcase}"
+      end
+
       def create_column_from(table, old, new, type: nil)
         old_col = column_for(table, old)
         new_type = type || old_col.type

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -212,6 +212,53 @@ describe Gitlab::Database::MigrationHelpers do
         allow(model).to receive(:transaction_open?).and_return(false)
       end
 
+      context 'ON DELETE statements' do
+        context 'on_delete: :nullify' do
+          it 'appends ON DELETE SET NULL 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).ordered.with(/VALIDATE CONSTRAINT/)
+            expect(model).to receive(:execute).with(/RESET ALL/)
+
+            expect(model).to receive(:execute).with(/ON DELETE SET NULL/)
+
+            model.add_concurrent_foreign_key(:projects, :users,
+                                             column: :user_id,
+                                             on_delete: :nullify)
+          end
+        end
+
+        context 'on_delete: :cascade' do
+          it 'appends ON DELETE CASCADE 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).ordered.with(/VALIDATE CONSTRAINT/)
+            expect(model).to receive(:execute).with(/RESET ALL/)
+
+            expect(model).to receive(:execute).with(/ON DELETE CASCADE/)
+
+            model.add_concurrent_foreign_key(:projects, :users,
+                                             column: :user_id,
+                                             on_delete: :cascade)
+          end
+        end
+
+        context 'on_delete: nil' do
+          it 'appends no 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).ordered.with(/VALIDATE CONSTRAINT/)
+            expect(model).to receive(:execute).with(/RESET ALL/)
+
+            expect(model).not_to receive(:execute).with(/ON DELETE/)
+
+            model.add_concurrent_foreign_key(:projects, :users,
+                                             column: :user_id,
+                                             on_delete: nil)
+          end
+        end
+      end
+
       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
@@ -223,20 +270,13 @@ describe Gitlab::Database::MigrationHelpers do
           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/)
-
-          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, on_delete: :cascade).and_return(true)
+          name = model.concurrent_foreign_key_name(:projects, :user_id)
+          expect(model).to receive(:foreign_key_exists?).with(:projects, :users,
+                                                              column: :user_id,
+                                                              on_delete: :cascade,
+                                                              name: name).and_return(true)
+
           expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/)
           expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
 
@@ -260,7 +300,10 @@ 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, on_delete: :cascade).and_return(true)
+              expect(model).to receive(:foreign_key_exists?).with(:projects, :users,
+                                                                  name: :foo,
+                                                                  on_delete: :cascade,
+                                                                  column: :user_id).and_return(true)
 
               expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/)
               expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
@@ -301,43 +344,57 @@ describe Gitlab::Database::MigrationHelpers do
       allow(model).to receive(:foreign_keys).with(:projects).and_return([key])
     end
 
-    it 'finds existing foreign keys by column' do
-      expect(model.foreign_key_exists?(:projects, :users, column: :non_standard_id)).to be_truthy
-    end
+    shared_examples_for 'foreign key checks' do
+      it 'finds existing foreign keys by column' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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 name' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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 and column' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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 name, column and on_delete' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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
+      it 'finds existing foreign keys by target table only' do
+        expect(model.foreign_key_exists?(:projects, target_table)).to be_truthy
+      end
 
-    it 'compares by column name if given' do
-      expect(model.foreign_key_exists?(:projects, :users, column: :user_id)).to be_falsey
-    end
+      it 'compares by column name if given' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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
+      it 'compares by foreign key name if given' do
+        expect(model.foreign_key_exists?(:projects, target_table, 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, target_table, 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, target_table, name: :fk_projects_users_non_standard_id, column: :non_standard_id, on_delete: :nullify)).to be_falsey
+      end
     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
+    context 'without specifying a target table' do
+      let(:target_table) { nil }
+
+      it_behaves_like 'foreign key checks'
     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
+    context 'specifying a target table' do
+      let(:target_table) { :users }
+
+      it_behaves_like 'foreign key checks'
     end
 
-    it 'compares by target if no column given' do
+    it 'compares by target table if no column given' do
       expect(model.foreign_key_exists?(:projects, :other_table)).to be_falsey
     end
   end