Clean up schema of the "merge_requests" table

This adds various foreign keys and indexes to the "merge_requests" table
as outlined in https://gitlab.com/gitlab-org/gitlab-ce/issues/31825.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/31825

11 files changed, 276 insertions, 5 deletions

diff --git a/changelogs/unreleased/merge-requests-schema-cleanup.yml b/changelogs/unreleased/merge-requests-schema-cleanup.yml
new file mode 100644
index 00000000000..ccce9b1436c
--- /dev/null
+++ b/changelogs/unreleased/merge-requests-schema-cleanup.yml
@@ -0,0 +1,5 @@
+---
+title: Clean up schema of the "merge_requests" table
+merge_request:
+author:
+type: other
diff --git a/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb b/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb
new file mode 100644
index 00000000000..021eaa04a0c
--- /dev/null
+++ b/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb
@@ -0,0 +1,43 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsAuthorIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_authors
+      where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.author_id = users.id)')
+        .where('author_id IS NOT NULL')
+    end
+  end
+
+  def up
+    # Replacing the ghost user ID logic would be too complex, hence we don't
+    # redefine the User model here.
+    ghost_id = User.select(:id).ghost.id
+
+    MergeRequest.with_orphaned_authors.each_batch(of: 100) do |batch|
+      batch.update_all(author_id: ghost_id)
+    end
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :users,
+      column: :author_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key(:merge_requests, column: :author_id)
+  end
+end
diff --git a/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb b/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb
new file mode 100644
index 00000000000..1a242f01051
--- /dev/null
+++ b/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb
@@ -0,0 +1,39 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsAssigneeIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_assignees
+      where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.assignee_id = users.id)')
+        .where('assignee_id IS NOT NULL')
+    end
+  end
+
+  def up
+    MergeRequest.with_orphaned_assignees.each_batch(of: 100) do |batch|
+      batch.update_all(assignee_id: nil)
+    end
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :users,
+      column: :assignee_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key(:merge_requests, column: :assignee_id)
+  end
+end
diff --git a/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb b/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb
new file mode 100644
index 00000000000..eb3872e38da
--- /dev/null
+++ b/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb
@@ -0,0 +1,46 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsUpdatedByIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_updaters
+      where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.updated_by_id = users.id)')
+        .where('updated_by_id IS NOT NULL')
+    end
+  end
+
+  def up
+    MergeRequest.with_orphaned_updaters.each_batch(of: 100) do |batch|
+      batch.update_all(updated_by_id: nil)
+    end
+
+    add_concurrent_index(
+      :merge_requests,
+      :updated_by_id,
+      where: 'updated_by_id IS NOT NULL'
+    )
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :users,
+      column: :updated_by_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key_without_error(:merge_requests, column: :updated_by_id)
+    remove_concurrent_index(:merge_requests, :updated_by_id)
+  end
+end
diff --git a/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb b/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb
new file mode 100644
index 00000000000..925b3e537d7
--- /dev/null
+++ b/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb
@@ -0,0 +1,46 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsMergeUserIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_mergers
+      where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.merge_user_id = users.id)')
+        .where('merge_user_id IS NOT NULL')
+    end
+  end
+
+  def up
+    MergeRequest.with_orphaned_mergers.each_batch(of: 100) do |batch|
+      batch.update_all(merge_user_id: nil)
+    end
+
+    add_concurrent_index(
+      :merge_requests,
+      :merge_user_id,
+      where: 'merge_user_id IS NOT NULL'
+    )
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :users,
+      column: :merge_user_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key_without_error(:merge_requests, column: :merge_user_id)
+    remove_concurrent_index(:merge_requests, :merge_user_id)
+  end
+end
diff --git a/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb b/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb
new file mode 100644
index 00000000000..2965e580c84
--- /dev/null
+++ b/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb
@@ -0,0 +1,45 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsSourceProjectIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_source_projects
+      where('NOT EXISTS (SELECT true FROM projects WHERE merge_requests.source_project_id = projects.id)')
+        .where('source_project_id IS NOT NULL')
+    end
+  end
+
+  def up
+    MergeRequest.with_orphaned_source_projects.each_batch(of: 100) do |batch|
+      batch.update_all(source_project_id: nil)
+    end
+
+    # We need to allow NULL values so we can nullify the column when the source
+    # project is removed. We _don't_ want to remove the merge request, instead
+    # the application will keep them but close them.
+    change_column_null(:merge_requests, :source_project_id, true)
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :projects,
+      column: :source_project_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key_without_error(:merge_requests, column: :source_project_id)
+    change_column_null(:merge_requests, :source_project_id, false)
+  end
+end
diff --git a/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb b/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb
new file mode 100644
index 00000000000..c005cf7d173
--- /dev/null
+++ b/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb
@@ -0,0 +1,39 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MergeRequestsMilestoneIdForeignKey < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    def self.with_orphaned_milestones
+      where('NOT EXISTS (SELECT true FROM milestones WHERE merge_requests.milestone_id = milestones.id)')
+        .where('milestone_id IS NOT NULL')
+    end
+  end
+
+  def up
+    MergeRequest.with_orphaned_milestones.each_batch(of: 100) do |batch|
+      batch.update_all(milestone_id: nil)
+    end
+
+    add_concurrent_foreign_key(
+      :merge_requests,
+      :milestones,
+      column: :milestone_id,
+      on_delete: :nullify
+    )
+  end
+
+  def down
+    remove_foreign_key_without_error(:merge_requests, column: :milestone_id)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 25b4fa8b888..7afab18df08 100644
--- a/db/schema.rb
+++ b/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: 20171114104051) do
+ActiveRecord::Schema.define(version: 20171114162227) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1040,7 +1040,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do
   create_table "merge_requests", force: :cascade do |t|
     t.string "target_branch", null: false
     t.string "source_branch", null: false
-    t.integer "source_project_id", null: false
+    t.integer "source_project_id"
     t.integer "author_id"
     t.integer "assignee_id"
     t.string "title"
@@ -1080,6 +1080,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do
   add_index "merge_requests", ["description"], name: "index_merge_requests_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "merge_requests", ["head_pipeline_id"], name: "index_merge_requests_on_head_pipeline_id", using: :btree
   add_index "merge_requests", ["latest_merge_request_diff_id"], name: "index_merge_requests_on_latest_merge_request_diff_id", using: :btree
+  add_index "merge_requests", ["merge_user_id"], name: "index_merge_requests_on_merge_user_id", where: "(merge_user_id IS NOT NULL)", using: :btree
   add_index "merge_requests", ["milestone_id"], name: "index_merge_requests_on_milestone_id", using: :btree
   add_index "merge_requests", ["source_branch"], name: "index_merge_requests_on_source_branch", using: :btree
   add_index "merge_requests", ["source_project_id", "source_branch"], name: "index_merge_requests_on_source_project_id_and_source_branch", using: :btree
@@ -1088,6 +1089,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do
   add_index "merge_requests", ["target_project_id", "merge_commit_sha", "id"], name: "index_merge_requests_on_tp_id_and_merge_commit_sha_and_id", using: :btree
   add_index "merge_requests", ["title"], name: "index_merge_requests_on_title", using: :btree
   add_index "merge_requests", ["title"], name: "index_merge_requests_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
+  add_index "merge_requests", ["updated_by_id"], name: "index_merge_requests_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree
 
   create_table "merge_requests_closing_issues", force: :cascade do |t|
     t.integer "merge_request_id", null: false
@@ -1965,7 +1967,13 @@ ActiveRecord::Schema.define(version: 20171114104051) do
   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", "merge_request_diffs", column: "latest_merge_request_diff_id", name: "fk_06067f5644", on_delete: :nullify
+  add_foreign_key "merge_requests", "milestones", name: "fk_6a5165a692", on_delete: :nullify
+  add_foreign_key "merge_requests", "projects", column: "source_project_id", name: "fk_3308fe130c", on_delete: :nullify
   add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade
+  add_foreign_key "merge_requests", "users", column: "assignee_id", name: "fk_6149611a04", on_delete: :nullify
+  add_foreign_key "merge_requests", "users", column: "author_id", name: "fk_e719a85f8a", on_delete: :nullify
+  add_foreign_key "merge_requests", "users", column: "merge_user_id", name: "fk_ad525e1f87", on_delete: :nullify
+  add_foreign_key "merge_requests", "users", column: "updated_by_id", name: "fk_641731faff", on_delete: :nullify
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade
   add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade
diff --git a/lib/gitlab/import_export/merge_request_parser.rb b/lib/gitlab/import_export/merge_request_parser.rb
index 61db4bd9ccc..f3d7407383c 100644
--- a/lib/gitlab/import_export/merge_request_parser.rb
+++ b/lib/gitlab/import_export/merge_request_parser.rb
@@ -1,7 +1,7 @@
 module Gitlab
   module ImportExport
     class MergeRequestParser
-      FORKED_PROJECT_ID = -1
+      FORKED_PROJECT_ID = nil
 
       def initialize(project, diff_head_sha, merge_request, relation_hash)
         @project = project
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index e4b4cf5ba85..c2bda6f8821 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -155,7 +155,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         end
 
         it 'has no source if source/target differ' do
-          expect(MergeRequest.find_by_title('MR2').source_project_id).to eq(-1)
+          expect(MergeRequest.find_by_title('MR2').source_project_id).to be_nil
         end
       end
 
diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb
index 16e288b3148..af35e17bfa7 100644
--- a/spec/services/milestones/destroy_service_spec.rb
+++ b/spec/services/milestones/destroy_service_spec.rb
@@ -5,7 +5,7 @@ describe Milestones::DestroyService do
   let(:project) { create(:project) }
   let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) }
   let!(:issue) { create(:issue, project: project, milestone: milestone) }
-  let(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) }
+  let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) }
 
   before do
     project.team << [user, :master]