Merge branch 'id-any-approver-rules-migration' into 'master'

Migrate approvals_before_merge column to a rule

See merge request gitlab-org/gitlab!16121

db/migrate/20190910212256_add_any_approver_rule_unique_indexes.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 AddAnyApproverRuleUniqueIndexes < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  PROJECT_RULE_UNIQUE_INDEX = 'any_approver_project_rule_type_unique_index'
+  MERGE_REQUEST_RULE_UNIQUE_INDEX = 'any_approver_merge_request_rule_type_unique_index'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:approval_project_rules, [:project_id],
+      where: "rule_type = 3",
+      name: PROJECT_RULE_UNIQUE_INDEX, unique: true)
+
+    add_concurrent_index(:approval_merge_request_rules, [:merge_request_id, :rule_type],
+      where: "rule_type = 4",
+      name: MERGE_REQUEST_RULE_UNIQUE_INDEX, unique: true)
+  end
+
+  def down
+    remove_concurrent_index_by_name(:approval_project_rules, PROJECT_RULE_UNIQUE_INDEX)
+    remove_concurrent_index_by_name(:approval_merge_request_rules, MERGE_REQUEST_RULE_UNIQUE_INDEX)
+  end
+end

db/post_migrate/20190905091812_schedule_project_any_approval_rule_migration.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 ScheduleProjectAnyApprovalRuleMigration < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 5_000
+  MIGRATION = 'PopulateAnyApprovalRuleForProjects'
+  DELAY_INTERVAL = 8.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'projects'
+
+    scope :with_approvals_before_merge, -> { where('approvals_before_merge <> 0') }
+  end
+
+  def up
+    add_concurrent_index :projects, :id,
+      name: 'tmp_projects_with_approvals_before_merge',
+      where: 'approvals_before_merge <> 0'
+
+    say "Scheduling `#{MIGRATION}` jobs"
+
+    # We currently have ~43k project records with non-zero approvals_before_merge on GitLab.com.
+    # This means it'll schedule ~9 jobs (5k projects each) with a 8 minutes gap,
+    # so this should take ~1 hour for all background migrations to complete.
+    #
+    # The approximate expected number of affected rows is: 18k
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      ScheduleProjectAnyApprovalRuleMigration::Project.with_approvals_before_merge,
+      MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+
+    remove_concurrent_index_by_name(:projects, 'tmp_projects_with_approvals_before_merge')
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20190905091831_schedule_merge_request_any_approval_rule_migration.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 ScheduleMergeRequestAnyApprovalRuleMigration < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 5_000
+  MIGRATION = 'PopulateAnyApprovalRuleForMergeRequests'
+  DELAY_INTERVAL = 8.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+
+    scope :with_approvals_before_merge, -> { where('approvals_before_merge <> 0') }
+  end
+
+  def up
+    add_concurrent_index :merge_requests, :id,
+      name: 'tmp_merge_requests_with_approvals_before_merge',
+      where: 'approvals_before_merge <> 0'
+
+    say "Scheduling `#{MIGRATION}` jobs"
+
+    # We currently have ~440_000 merge request records with non-zero approvals_before_merge on GitLab.com.
+    # This means it'll schedule ~88 jobs (5k merge requests each) with a 8 minutes gap,
+    # so this should take ~12 hours for all background migrations to complete.
+    #
+    # The approximate expected number of affected rows is: 190k
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      ScheduleMergeRequestAnyApprovalRuleMigration::MergeRequest.with_approvals_before_merge,
+      MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+
+    remove_concurrent_index_by_name(:merge_requests, 'tmp_merge_requests_with_approvals_before_merge')
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema.rb

@@ -319,8 +319,9 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do
     t.integer "report_type", limit: 2
     t.index ["merge_request_id", "code_owner", "name"], name: "approval_rule_name_index_for_code_owners", unique: true, where: "(code_owner = true)"
     t.index ["merge_request_id", "code_owner"], name: "index_approval_merge_request_rules_1"
-    t.index ["merge_request_id", "rule_type", "name"], name: "index_approval_rule_name_for_code_owners_rule_type", unique: true, where: "(rule_type = 2)"
-    t.index ["merge_request_id", "rule_type"], name: "index_approval_rules_code_owners_rule_type", where: "(rule_type = 2)"
+    t.index ["merge_request_id", "name"], name: "index_approval_rule_name_for_code_owners_rule_type", unique: true, where: "(rule_type = 2)"
+    t.index ["merge_request_id", "rule_type"], name: "any_approver_merge_request_rule_type_unique_index", unique: true, where: "(rule_type = 4)"
+    t.index ["merge_request_id"], name: "index_approval_rules_code_owners_rule_type", where: "(rule_type = 2)"
   end
 
   create_table "approval_merge_request_rules_approved_approvers", force: :cascade do |t|
@@ -351,6 +352,7 @@ ActiveRecord::Schema.define(version: 2019_09_12_061145) do
     t.integer "approvals_required", limit: 2, default: 0, null: false
     t.string "name", null: false
     t.integer "rule_type", limit: 2, default: 0, null: false
+    t.index ["project_id"], name: "any_approver_project_rule_type_unique_index", unique: true, where: "(rule_type = 3)"
     t.index ["project_id"], name: "index_approval_project_rules_on_project_id"
     t.index ["rule_type"], name: "index_approval_project_rules_on_rule_type"
   end

ee/app/models/approval_merge_request_rule.rb

@@ -43,7 +43,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
   enum rule_type: {
     regular: 1,
     code_owner: 2,
-    report_approver: 3
+    report_approver: 3,
+    any_approver: 4
   }
 
   enum report_type: {

ee/app/models/approval_project_rule.rb

@@ -8,7 +8,8 @@ class ApprovalProjectRule < ApplicationRecord
   enum rule_type: {
     regular: 0,
     code_owner: 1, # currently unused
-    report_approver: 2
+    report_approver: 2,
+    any_approver: 3
   }
 
   alias_method :code_owner, :code_owner?

ee/app/models/concerns/approval_rule_like.rb

@@ -11,6 +11,7 @@ module ApprovalRuleLike
     DEFAULT_NAME_FOR_SECURITY_REPORT => :security
   }.freeze
   APPROVALS_REQUIRED_MAX = 100
+  ALL_MEMBERS = 'All Members'
 
   included do
     has_and_belongs_to_many :users

ee/app/models/concerns/deprecated_approvals_before_merge.rb

+# frozen_string_literal: true
+
+# This module handles backward compatibility for approvals_before_merge column
+module DeprecatedApprovalsBeforeMerge
+  extend ActiveSupport::Concern
+
+  include AfterCommitQueue
+
+  included do
+    after_save do
+      next unless saved_changes['approvals_before_merge']
+
+      run_after_commit do
+        update_any_approver_rule
+      end
+    end
+  end
+
+  private
+
+  def any_approver_rule
+    strong_memoize(:any_approver_rule) do
+      approval_rules.any_approver.safe_find_or_create_by(name: ApprovalRuleLike::ALL_MEMBERS)
+    end
+  end
+
+  def update_any_approver_rule
+    return if any_approver_rule.approvals_required == approvals_before_merge.to_i
+
+    any_approver_rule.update_column(:approvals_required, approvals_before_merge.to_i)
+  end
+end

ee/app/models/ee/merge_request.rb

@@ -12,6 +12,7 @@ module EE
 
     prepended do
       include Elastic::ApplicationVersionedSearch
+      include DeprecatedApprovalsBeforeMerge
 
       has_many :reviews, inverse_of: :merge_request
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent

ee/app/models/ee/project.rb

@@ -20,6 +20,7 @@ module EE
       include EachBatch
       include InsightsFeature
       include Vulnerable
+      include DeprecatedApprovalsBeforeMerge
 
       self.ignored_columns += %i[
         mirror_last_update_at

ee/lib/gitlab/background_migration/populate_any_approval_rule_for_merge_requests.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This background migration creates any approver rule records according
+    # to the given merge request IDs range. A _single_ INSERT is issued for the given range.
+    class PopulateAnyApprovalRuleForMergeRequests
+      def perform(from_id, to_id)
+        select_sql =
+          MergeRequest
+            .where(merge_request_approval_rules_not_exists_clause)
+            .where(id: from_id..to_id)
+            .where('approvals_before_merge <> 0')
+            .select("id, approvals_before_merge, created_at, updated_at, 4, '#{ApprovalRuleLike::ALL_MEMBERS}'")
+            .to_sql
+
+        execute("INSERT INTO approval_merge_request_rules (merge_request_id, approvals_required, created_at, updated_at, rule_type, name) #{select_sql}")
+      end
+
+      private
+
+      def merge_request_approval_rules_not_exists_clause
+        <<~SQL
+            NOT EXISTS (SELECT 1 FROM approval_merge_request_rules
+                        WHERE approval_merge_request_rules.merge_request_id = merge_requests.id)
+        SQL
+      end
+
+      def execute(sql)
+        @connection ||= ActiveRecord::Base.connection
+        @connection.execute(sql)
+      end
+    end
+  end
+end

ee/lib/gitlab/background_migration/populate_any_approval_rule_for_projects.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This background migration creates any approver rule records according
+    # to the given project IDs range. A _single_ INSERT is issued for the given range.
+    class PopulateAnyApprovalRuleForProjects
+      def perform(from_id, to_id)
+        select_sql =
+          Project
+            .where(project_approval_rules_not_exists_clause)
+            .where(id: from_id..to_id)
+            .where('approvals_before_merge <> 0')
+            .select("id, approvals_before_merge, created_at, updated_at, 4, '#{ApprovalRuleLike::ALL_MEMBERS}'")
+            .to_sql
+
+        execute("INSERT INTO approval_project_rules (project_id, approvals_required, created_at, updated_at, rule_type, name) #{select_sql}")
+      end
+
+      private
+
+      def project_approval_rules_not_exists_clause
+        <<~SQL
+            NOT EXISTS (SELECT 1 FROM approval_project_rules
+                        WHERE approval_project_rules.project_id = projects.id)
+        SQL
+      end
+
+      def execute(sql)
+        @connection ||= ActiveRecord::Base.connection
+        @connection.execute(sql)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb

@@ -117,7 +117,15 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
     end
 
     context 'merge request' do
-      let(:target) { create(:merge_request, approvals_before_merge: 2) }
+      let(:target) do
+        merge_request = build(:merge_request, approvals_before_merge: 2)
+
+        allow(merge_request).to receive(:update_any_approver_rule)
+
+        merge_request.save!
+        merge_request
+      end
+
       let(:target_type) { 'MergeRequest' }
       let(:approval_rule) { create(:approval_merge_request_rule, merge_request: target) }
 
@@ -243,7 +251,15 @@ describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
     end
 
     context 'project' do
-      let(:target) { create(:project, approvals_before_merge: 2) }
+      let(:target) do
+        project = build(:project, approvals_before_merge: 2)
+
+        allow(project).to receive(:update_any_approver_rule)
+
+        project.save!
+        project
+      end
+
       let(:target_type) { 'Project' }
       let(:approval_rule) { create(:approval_project_rule, project: target) }
 

ee/spec/lib/gitlab/background_migration/populate_any_approval_rule_for_merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::PopulateAnyApprovalRuleForMergeRequests, :migration, schema: 2019_09_05_091831 do
+  let(:namespaces) { table(:namespaces) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:projects) { table(:projects) }
+  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:approval_merge_request_rules) { table(:approval_merge_request_rules) }
+
+  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
+
+  before do
+    create_merge_request(2, approvals_before_merge: 2)
+
+    # Test filtering rows with empty approvals_before_merge column
+    create_merge_request(3, approvals_before_merge: nil)
+
+    # Test filtering already migrated rows
+    create_merge_request(4, approvals_before_merge: 3)
+    approval_merge_request_rules.create(id: 3,
+      merge_request_id: 4, approvals_required: 3, rule_type: 4, name: ApprovalRuleLike::ALL_MEMBERS)
+
+    # Test filtering MRs with existing rules
+    create_merge_request(5, approvals_before_merge: 3)
+    approval_merge_request_rules.create(id: 4,
+      merge_request_id: 5, approvals_required: 3, rule_type: 1, name: 'Regular rules')
+
+    create_merge_request(6, approvals_before_merge: 5)
+
+    # Test filtering rows with zero approvals_before_merge column
+    create_merge_request(7, approvals_before_merge: 0)
+  end
+
+  describe '#perform' do
+    it 'creates approval_merge_request_rules rows according to merge_requests' do
+      expect { subject.perform(1, 7) }.to change(ApprovalMergeRequestRule, :count).by(2)
+
+      created_rows = [
+        { 'merge_request_id' => 2, 'approvals_required' => 2 },
+        { 'merge_request_id' => 6, 'approvals_required' => 5 }
+      ]
+      existing_rows = [
+        { 'merge_request_id' => 4, 'approvals_required' => 3 }
+      ]
+
+      rows = approval_merge_request_rules.where(rule_type: 4).order(:id).map do |row|
+        row.attributes.slice('merge_request_id', 'approvals_required')
+      end
+
+      expect(rows).to match_array(created_rows + existing_rows)
+    end
+  end
+end

ee/spec/lib/gitlab/background_migration/populate_any_approval_rule_for_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::PopulateAnyApprovalRuleForProjects, :migration, schema: 2019_09_05_091812 do
+  let(:namespaces) { table(:namespaces) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:projects) { table(:projects) }
+  let(:approval_project_rules) { table(:approval_project_rules) }
+
+  def create_project(id, params = {})
+    params.merge!(id: id, namespace_id: namespace.id)
+
+    projects.create(params)
+  end
+
+  before do
+    create_project(2, approvals_before_merge: 2)
+
+    # Test filtering rows with empty approvals_before_merge column
+    create_project(3, approvals_before_merge: 0)
+
+    # Test filtering already migrated rows
+    create_project(4, approvals_before_merge: 3)
+    approval_project_rules.create(id: 3,
+      project_id: 4, approvals_required: 3, rule_type: 4, name: ApprovalRuleLike::ALL_MEMBERS)
+
+    # Test filtering MRs with existing rules
+    create_project(5, approvals_before_merge: 3)
+    approval_project_rules.create(id: 4,
+      project_id: 5, approvals_required: 3, rule_type: 1, name: 'Regular rules')
+
+    create_project(6, approvals_before_merge: 5)
+  end
+
+  describe '#perform' do
+    it 'creates approval_project_rules rows according to projects' do
+      expect { subject.perform(1, 6) }.to change(ApprovalProjectRule, :count).by(2)
+
+      created_rows = [
+        { 'project_id' => 2, 'approvals_required' => 2 },
+        { 'project_id' => 6, 'approvals_required' => 5 }
+      ]
+      existing_rows = [
+        { 'project_id' => 4, 'approvals_required' => 3 }
+      ]
+
+      rows = approval_project_rules.where(rule_type: 4).order(:id).map do |row|
+        row.attributes.slice('project_id', 'approvals_required')
+      end
+
+      expect(rows).to match_array(created_rows + existing_rows)
+    end
+  end
+end

ee/spec/migrations/schedule_merge_request_any_approval_rule_migration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190905091831_schedule_merge_request_any_approval_rule_migration.rb')
+
+describe ScheduleMergeRequestAnyApprovalRuleMigration, :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, options = {})
+    default_options = {
+      id: id,
+      target_project_id: project.id,
+      target_branch: 'master',
+      source_project_id: project.id,
+      source_branch: 'mr name',
+      title: "mr name#{id}",
+      approvals_before_merge: 2
+    }
+
+    merge_requests.create!(default_options.merge(options))
+  end
+
+  it 'correctly schedules background migrations' do
+    create_merge_request(1, approvals_before_merge: nil)
+    create_merge_request(2)
+    create_merge_request(3, approvals_before_merge: 0)
+    create_merge_request(4)
+    create_merge_request(5, approvals_before_merge: 0)
+    create_merge_request(6)
+
+    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, 2, 4)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(16.minutes, 6, 6)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

ee/spec/migrations/schedule_project_any_approval_rule_migration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190905091812_schedule_project_any_approval_rule_migration.rb')
+
+describe ScheduleProjectAnyApprovalRuleMigration, :migration, :sidekiq do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+
+  def create_project(id, options = {})
+    default_options = {
+      id: id,
+      namespace_id: namespace.id,
+      name: 'foo',
+      approvals_before_merge: 2
+    }
+
+    projects.create(default_options.merge(options))
+  end
+
+  it 'correctly schedules background migrations' do
+    create_project(1, approvals_before_merge: 0)
+    create_project(2)
+    create_project(3, approvals_before_merge: 0)
+    create_project(4)
+    create_project(5, approvals_before_merge: 0)
+    create_project(6)
+
+    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, 2, 4)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(16.minutes, 6, 6)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -88,6 +88,20 @@ describe ApprovalMergeRequestRule do
         expect(rule).not_to be_valid
       end
     end
+
+    context 'any_approver rules' do
+      let(:rule) { build(:approval_merge_request_rule, merge_request: merge_request, rule_type: :any_approver) }
+
+      it 'is valid' do
+        expect(rule).to be_valid
+      end
+
+      it 'creating more than one any_approver rule raises an error' do
+        create(:approval_merge_request_rule, merge_request: merge_request, rule_type: :any_approver)
+
+        expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique)
+      end
+    end
   end
 
   context 'scopes'  do

ee/spec/models/approval_project_rule_spec.rb

@@ -123,4 +123,15 @@ describe ApprovalProjectRule do
       end
     end
   end
+
+  context 'any_approver rules' do
+    let(:project) { create(:project) }
+    let(:rule) { build(:approval_project_rule, project: project, rule_type: :any_approver) }
+
+    it 'creating more than one any_approver rule raises an error' do
+      create(:approval_project_rule, project: project, rule_type: :any_approver)
+
+      expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique)
+    end
+  end
 end

ee/spec/models/concerns/deprecated_approvals_before_merge_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeprecatedApprovalsBeforeMerge do
+  shared_examples 'with approvals before merge deprecated' do
+    context 'updating approvals_before_merge' do
+      it 'creates any_approver rule' do
+        subject.update(approvals_before_merge: 3)
+
+        expect_approvals_before_merge_to_be_updated(3)
+
+        subject.update(approvals_before_merge: 5)
+
+        expect_approvals_before_merge_to_be_updated(5)
+      end
+    end
+  end
+
+  context 'merge request' do
+    subject { create(:merge_request, approvals_before_merge: 2) }
+
+    it_behaves_like 'with approvals before merge deprecated'
+  end
+
+  context 'project' do
+    subject { create(:project, approvals_before_merge: 2) }
+
+    it_behaves_like 'with approvals before merge deprecated'
+  end
+
+  def expect_approvals_before_merge_to_be_updated(value)
+    expect(subject.approval_rules.any_approver.size).to eq(1)
+
+    rule = subject.approval_rules.any_approver.first
+    expect(rule.approvals_required).to eq(value)
+  end
+end