Merge branch 'backstage/gb/add-index-and-foreign-key-to-merge-requests' into 'master'

Add a foreign key to `merge_requests.head_pipeline_id`

Closes #34065

See merge request !12837

db/migrate/20170713104829_add_foreign_key_to_merge_requests.rb

+class AddForeignKeyToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+    include ::EachBatch
+  end
+
+  def up
+    scope = <<-SQL.strip_heredoc
+      head_pipeline_id IS NOT NULL
+        AND NOT EXISTS (
+          SELECT 1 FROM ci_pipelines
+            WHERE ci_pipelines.id = merge_requests.head_pipeline_id
+        )
+    SQL
+
+    MergeRequest.where(scope).each_batch(of: 1000) do |merge_requests|
+      merge_requests.update_all(head_pipeline_id: nil)
+    end
+
+    unless foreign_key_exists?(:merge_requests, :head_pipeline_id)
+      add_concurrent_foreign_key(:merge_requests, :ci_pipelines,
+                                 column: :head_pipeline_id, on_delete: :nullify)
+    end
+  end
+
+  def down
+    if foreign_key_exists?(:merge_requests, :head_pipeline_id)
+      remove_foreign_key(:merge_requests, column: :head_pipeline_id)
+    end
+  end
+
+  private
+
+  def foreign_key_exists?(table, column)
+    foreign_keys(table).any? do |key|
+      key.options[:column] == column.to_s
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170707184244) do
+ActiveRecord::Schema.define(version: 20170713104829) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1615,6 +1615,7 @@ ActiveRecord::Schema.define(version: 20170707184244) do
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
+  add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify
   add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade

lib/gitlab/database/migration_helpers.rb

@@ -140,6 +140,8 @@ module Gitlab
           return add_foreign_key(source, target,
                                  column: column,
                                  on_delete: on_delete)
+        else
+          on_delete = 'SET NULL' if on_delete == :nullify
         end
 
         disable_statement_timeout
@@ -155,7 +157,7 @@ module Gitlab
         ADD CONSTRAINT #{key_name}
         FOREIGN KEY (#{column})
         REFERENCES #{target} (id)
-        #{on_delete ? "ON DELETE #{on_delete}" : ''}
+        #{on_delete ? "ON DELETE #{on_delete.upcase}" : ''}
         NOT VALID;
         EOF
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -174,13 +174,23 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
           allow(Gitlab::Database).to receive(:mysql?).and_return(false)
         end
 
-        it 'creates a concurrent foreign key' do
+        it 'creates a concurrent foreign key and validates it' do
           expect(model).to receive(:disable_statement_timeout)
           expect(model).to receive(:execute).ordered.with(/NOT VALID/)
           expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
 
           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)
+          expect(model).to receive(:execute).with(/ON DELETE SET NULL/)
+          expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
+
+          model.add_concurrent_foreign_key(:projects, :users,
+                                           column: :user_id,
+                                           on_delete: :nullify)
+        end
       end
     end
   end

spec/migrations/add_foreign_key_to_merge_requests_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20170713104829_add_foreign_key_to_merge_requests.rb')
+
+describe AddForeignKeyToMergeRequests, :migration do
+  let(:projects) { table(:projects) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:pipelines) { table(:ci_pipelines) }
+
+  before do
+    projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce')
+    pipelines.create!(project_id: projects.first.id,
+                      ref: 'some-branch',
+                      sha: 'abc12345')
+
+    # merge request without a pipeline
+    create_merge_request(head_pipeline_id: nil)
+
+    # merge request with non-existent pipeline
+    create_merge_request(head_pipeline_id: 1234)
+
+    # merge reqeust with existing pipeline assigned
+    create_merge_request(head_pipeline_id: pipelines.first.id)
+  end
+
+  it 'correctly adds a foreign key to head_pipeline_id' do
+    migrate!
+
+    expect(merge_requests.first.head_pipeline_id).to be_nil
+    expect(merge_requests.second.head_pipeline_id).to be_nil
+    expect(merge_requests.third.head_pipeline_id).to eq pipelines.first.id
+  end
+
+  def create_merge_request(**opts)
+    merge_requests.create!(source_project_id: projects.first.id,
+                           target_project_id: projects.first.id,
+                           source_branch: 'some-branch',
+                           target_branch: 'master', **opts)
+  end
+end