Merge branch 'osw-multi-assignees-merge-requests-migration' into 'master'

Add multiple MR assignees migration and background table population

See merge request gitlab-org/gitlab-ce!26496

app/models/merge_request.rb

@@ -67,6 +67,8 @@ class MergeRequest < ActiveRecord::Base
   has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
   has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
+  has_many :merge_request_assignees
+  # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457
   belongs_to :assignee, class_name: "User"
   serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
@@ -76,6 +78,10 @@ class MergeRequest < ActiveRecord::Base
   after_update :reload_diff_if_branch_changed
   after_save :ensure_metrics
+  # Required until the codebase starts using this relation for single or multiple assignees.
+  # TODO: Remove at gitlab-ee#2004 implementation.
+  after_save :refresh_merge_request_assignees, if: :assignee_id_changed?
+
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken
@@ -675,6 +681,15 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff || create_merge_request_diff
   end
+  def refresh_merge_request_assignees
+    transaction do
+      # Using it instead relation.delete_all in order to avoid adding a
+      # dependent: :delete_all (we already have foreign key cascade deletion).
+      MergeRequestAssignee.where(merge_request_id: self).delete_all
+      merge_request_assignees.create(user_id: assignee_id) if assignee_id
+    end
+  end
+
   def create_merge_request_diff
     fetch_ref!

app/models/merge_request_assignee.rb

+# frozen_string_literal: true
+
+class MergeRequestAssignee < ApplicationRecord
+  belongs_to :merge_request
+  belongs_to :assignee, class_name: "User", foreign_key: :user_id
+end

db/migrate/20190315191339_create_merge_request_assignees_table.rb

+# frozen_string_literal: true
+
+class CreateMergeRequestAssigneesTable < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  INDEX_NAME = 'index_merge_request_assignees_on_merge_request_id_and_user_id'
+
+  def up
+    create_table :merge_request_assignees do |t|
+      t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false
+      t.references :merge_request, foreign_key: { on_delete: :cascade }, null: false
+    end
+
+    add_index :merge_request_assignees, [:merge_request_id, :user_id], unique: true, name: INDEX_NAME
+  end
+
+  def down
+    drop_table :merge_request_assignees
+  end
+end

db/post_migrate/20190322132835_schedule_populate_merge_request_assignees_table.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class SchedulePopulateMergeRequestAssigneesTable < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 10_000
+  MIGRATION = 'PopulateMergeRequestAssigneesTable'
+  DELAY_INTERVAL = 8.minutes.to_i
+
+  disable_ddl_transaction!
+
+  def up
+    say 'Scheduling `PopulateMergeRequestAssigneesTable` jobs'
+    # We currently have ~4_500_000 merge request records on GitLab.com.
+    # This means it'll schedule ~450 jobs (10k MRs each) with a 8 minutes gap,
+    # so this should take ~60 hours for all background migrations to complete.
+    queue_background_migration_jobs_by_range_at_intervals(MergeRequest, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20190301182457) do
+ActiveRecord::Schema.define(version: 20190322132835) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1201,6 +1201,14 @@ ActiveRecord::Schema.define(version: 20190301182457) do
     t.index ["user_id"], name: "index_members_on_user_id", using: :btree
   end
+  create_table "merge_request_assignees", force: :cascade do |t|
+    t.integer "user_id", null: false
+    t.integer "merge_request_id", null: false
+    t.index ["merge_request_id", "user_id"], name: "index_merge_request_assignees_on_merge_request_id_and_user_id", unique: true, using: :btree
+    t.index ["merge_request_id"], name: "index_merge_request_assignees_on_merge_request_id", using: :btree
+    t.index ["user_id"], name: "index_merge_request_assignees_on_user_id", using: :btree
+  end
+
   create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
     t.datetime_with_timezone "authored_date"
     t.datetime_with_timezone "committed_date"
@@ -2454,6 +2462,8 @@ ActiveRecord::Schema.define(version: 20190301182457) do
   add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade
   add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade
   add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade
+  add_foreign_key "merge_request_assignees", "merge_requests", on_delete: :cascade
+  add_foreign_key "merge_request_assignees", "users", on_delete: :cascade
   add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade

lib/gitlab/background_migration/populate_merge_request_assignees_table.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This background migration creates records on merge_request_assignees according
+    # to the given merge request IDs range. A _single_ INSERT is issued for the given range.
+    # This is required for supporting multiple assignees on merge requests.
+    class PopulateMergeRequestAssigneesTable
+      def perform(from_id, to_id)
+        select_sql =
+          MergeRequest
+            .where(merge_request_assignees_not_exists_clause)
+            .where(id: from_id..to_id)
+            .where('assignee_id IS NOT NULL')
+            .select(:id, :assignee_id)
+            .to_sql
+
+        execute("INSERT INTO merge_request_assignees (merge_request_id, user_id) #{select_sql}")
+      end
+
+      private
+
+      def merge_request_assignees_not_exists_clause
+        <<~SQL
+            NOT EXISTS (SELECT 1 FROM merge_request_assignees
+                        WHERE merge_request_assignees.merge_request_id = merge_requests.id)
+        SQL
+      end
+
+      def execute(sql)
+        @connection ||= ActiveRecord::Base.connection
+        @connection.execute(sql)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Gitlab::BackgroundMigration::PopulateMergeRequestAssigneesTable, :migration, schema: 20190315191339 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:users) { table(:users) }
+
+  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+  let(:user_2) { users.create!(email: 'test2@example.com', projects_limit: 100, username: 'test') }
+  let(:user_3) { users.create!(email: 'test3@example.com', projects_limit: 100, username: 'test') }
+
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:merge_request_assignees) { table(:merge_request_assignees) }
+
+  def create_merge_request(id, params = {})
+    params.merge!(id: id,
+                  target_project_id: project.id,
+                  target_branch: 'master',
+                  source_project_id: project.id,
+                  source_branch: 'mr name',
+                  title: "mr name#{id}")
+
+    merge_requests.create(params)
+  end
+
+  describe '#perform' do
+    it 'creates merge_request_assignees rows according to merge_requests' do
+      create_merge_request(2, assignee_id: user.id)
+      create_merge_request(3, assignee_id: user_2.id)
+      create_merge_request(4, assignee_id: user_3.id)
+      # Test filtering already migrated row
+      merge_request_assignees.create!(merge_request_id: 2, user_id: user_3.id)
+
+      subject.perform(1, 4)
+
+      rows = merge_request_assignees.order(:id).map { |row| row.attributes.slice('merge_request_id', 'user_id') }
+      existing_rows = [
+        { 'merge_request_id' => 2, 'user_id' => user_3.id }
+      ]
+      created_rows = [
+        { 'merge_request_id' => 3, 'user_id' => user_2.id },
+        { 'merge_request_id' => 4, 'user_id' => user_3.id }
+      ]
+      expected_rows = existing_rows + created_rows
+
+      expect(rows.size).to eq(expected_rows.size)
+      expected_rows.each do |expected_row|
+        expect(rows).to include(expected_row)
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -100,6 +100,7 @@ merge_requests:
 - head_pipeline
 - latest_merge_request_diff
 - merge_request_pipelines
+- merge_request_assignees
 merge_request_diff:
 - merge_request
 - merge_request_diff_commits

spec/migrations/schedule_populate_merge_request_assignees_table_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190322132835_schedule_populate_merge_request_assignees_table.rb')
+
+describe SchedulePopulateMergeRequestAssigneesTable, :migration, :sidekiq do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
+  let(:merge_requests) { table(:merge_requests) }
+
+  def create_merge_request(id)
+    params = {
+      id: id,
+      target_project_id: project.id,
+      target_branch: 'master',
+      source_project_id: project.id,
+      source_branch: 'mr name',
+      title: "mr name#{id}"
+    }
+
+    merge_requests.create!(params)
+  end
+
+  it 'correctly schedules background migrations' do
+    create_merge_request(1)
+    create_merge_request(2)
+    create_merge_request(3)
+
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(8.minutes, 1, 2)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(16.minutes, 3, 3)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -179,6 +179,31 @@ describe MergeRequest do
         expect(MergeRequest::Metrics.count).to eq(1)
       end
     end
+
+    describe '#refresh_merge_request_assignees' do
+      set(:user) { create(:user) }
+
+      it 'creates merge request assignees relation upon MR creation' do
+        merge_request = create(:merge_request, assignee: nil)
+
+        expect(merge_request.merge_request_assignees).to be_empty
+
+        expect { merge_request.update!(assignee: user) }
+          .to change { merge_request.reload.merge_request_assignees.count }
+          .from(0).to(1)
+      end
+
+      it 'updates merge request assignees relation upon MR assignee change' do
+        another_user = create(:user)
+        merge_request = create(:merge_request, assignee: user)
+
+        expect { merge_request.update!(assignee: another_user) }
+          .to change { merge_request.reload.merge_request_assignees.first.assignee }
+          .from(user).to(another_user)
+
+        expect(merge_request.merge_request_assignees.count).to eq(1)
+      end
+    end
   end
   describe 'respond to' do