Merge branch 'bvl-optimize-mr-approvals' into 'master'

Optimize number of queries for approvals

Closes #4903

See merge request gitlab-org/gitlab-ee!4492

changelogs/unreleased-ee/bvl-optimize-mr-approvals.yml

+---
+title: Speed up approvals calculations
+merge_request: 4492
+author:
+type: performance

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -3,6 +3,8 @@ module EE
     module MergeRequestsController
       extend ActiveSupport::Concern
 
+      APPROVAL_RENDERING_ACTIONS = [:approve, :approvals, :unapprove].freeze
+
       def approve
         unless merge_request.can_approve?(current_user)
           return render_404
@@ -31,6 +33,23 @@ module EE
 
       protected
 
+      # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      # Assigning both @merge_request and @issuable like in
+      # `Projects::MergeRequests::ApplicationController`, and calling super if
+      # we don't need the extra includes requires us to disable this cop.
+      def merge_request
+        return super unless APPROVAL_RENDERING_ACTIONS.include?(action_name.to_sym)
+
+        @issuable = @merge_request ||= project.merge_requests
+                                         .includes(
+                                           :approved_by_users,
+                                           approvers: :user
+                                         )
+                                         .find_by!(iid: params[:id])
+        super
+      end
+      # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
       def define_edit_vars
         super
 

ee/app/models/concerns/approvable.rb

 module Approvable
+  include Gitlab::Utils::StrongMemoize
+
   def requires_approve?
     approvals_required.nonzero?
   end
@@ -13,7 +15,7 @@ module Approvable
   #
   def approvals_left
     [
-      [approvals_required - approvals.count, number_of_potential_approvers].min,
+      [approvals_required - approvals.size, number_of_potential_approvers].min,
       0
     ].max
   end
@@ -66,7 +68,9 @@ module Approvable
   # Users in the list of approvers who have not already approved this MR.
   #
   def approvers_left
-    User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id))
+    strong_memoize(:approvers_left) do
+      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approved_by_users.select(:id))
+    end
   end
 
   # The list of approvers from either this MR (if they've been set on the MR) or the
@@ -79,7 +83,7 @@ module Approvable
     approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
     approvers_relation = approvers_relation.where.not(user_id: author.id) if author
 
-    approvers_relation
+    approvers_relation.includes(:user)
   end
 
   def overall_approver_groups
@@ -87,14 +91,16 @@ module Approvable
   end
 
   def all_approvers_including_groups
-    approvers = []
+    strong_memoize(:all_approvers_including_groups) do
+      approvers = []
 
-    # Approvers from direct assignment
-    approvers << approvers_from_users
+      # Approvers from direct assignment
+      approvers << approvers_from_users
 
-    approvers << approvers_from_groups
+      approvers << approvers_from_groups
 
-    approvers.flatten
+      approvers.flatten
+    end
   end
 
   def approvers_from_users
@@ -144,10 +150,6 @@ module Approvable
     remaining_approvals.zero? || remaining_approvals > approvers_left.count
   end
 
-  def approved_by_users
-    approvals.map(&:user)
-  end
-
   def approver_ids=(value)
     value.split(",").map(&:strip).each do |user_id|
       next if author && user_id == author.id
@@ -161,4 +163,12 @@ module Approvable
       approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
     end
   end
+
+  def reset_approval_cache!
+    approvals(true)
+    approved_by_users(true)
+
+    clear_memoization(:approvers_left)
+    clear_memoization(:all_approvers_including_groups)
+  end
 end

ee/app/models/ee/merge_request.rb

@@ -6,6 +6,7 @@ module EE
 
     included do
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 

ee/app/services/merge_requests/approval_service.rb

@@ -4,6 +4,8 @@ module MergeRequests
       approval = merge_request.approvals.new(user: current_user)
 
       if approval.save
+        merge_request.reset_approval_cache!
+
         create_approval_note(merge_request)
         mark_pending_todos_as_done(merge_request)
 

ee/app/services/merge_requests/remove_approval_service.rb

@@ -9,9 +9,7 @@ module MergeRequests
       currently_approved = merge_request.approved?
 
       if approval.destroy_all
-        # bust the cache here, otherwise will show results from
-        # before the deletion
-        merge_request.approvals(true)
+        merge_request.reset_approval_cache!
 
         create_note(merge_request)
 

ee/app/views/shared/issuable/_approvals.html.haml

@@ -12,7 +12,7 @@
     Approvers
   .col-sm-10
     - if can_update_approvers
-      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups << ineligible_approver, project: issuable.target_project)
+      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups + [ineligible_approver], project: issuable.target_project)
       .help-block
         This merge request must be approved by these users.
         You can override the project settings by setting your own list of approvers.

ee/spec/models/approvable_spec.rb

+require 'spec_helper'
+
+describe Approvable do
+  let(:merge_request) { create(:merge_request, :with_approver) }
+
+  describe '#approvers_left' do
+    it 'only queries once' do
+      merge_request
+
+      expect(User).to receive(:where).and_call_original.once
+
+      3.times { merge_request.approvers_left }
+    end
+  end
+
+  describe '#reset_approval_cache!' do
+    it 'clears the cache of approvers left' do
+      user_can_approve = merge_request.approvers_left.first
+      merge_request.approvals.create!(user: user_can_approve)
+
+      merge_request.reset_approval_cache!
+
+      expect(merge_request.approvers_left).to be_empty
+    end
+  end
+end

ee/spec/models/merge_request_spec.rb

@@ -11,6 +11,7 @@ describe MergeRequest do
     it { is_expected.to have_many(:approvals).dependent(:delete_all) }
     it { is_expected.to have_many(:approvers).dependent(:delete_all) }
     it { is_expected.to have_many(:approver_groups).dependent(:delete_all) }
+    it { is_expected.to have_many(:approved_by_users) }
   end
 
   describe '#squash_in_progress?' do

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -44,6 +44,12 @@ describe MergeRequests::ApprovalService do
         expect(todo.reload).to be_done
       end
 
+      it 'resets the cache for approvals' do
+        expect(merge_request).to receive(:reset_approval_cache!)
+
+        service.execute(merge_request)
+      end
+
       context 'with remaining approvals' do
         it 'does not fire a webhook' do
           expect(merge_request).to receive(:approvals_left).and_return(5)

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -34,6 +34,12 @@ describe MergeRequests::RemoveApprovalService do
 
         execute!
       end
+
+      it 'resets the cache for approvals' do
+        expect(merge_request).to receive(:reset_approval_cache!)
+
+        execute!
+      end
     end
 
     context 'with an approved merge request' do

spec/lib/gitlab/import_export/all_models.yml

@@ -95,6 +95,7 @@ merge_requests:
 - timelogs
 - head_pipeline
 - latest_merge_request_diff
+- approved_by_users
 merge_request_diff:
 - merge_request
 - merge_request_diff_commits

spec/models/merge_request_spec.rb

@@ -692,10 +692,17 @@ describe MergeRequest do
     let(:author) { create(:user) }
     let(:merge_request) { create(:merge_request, source_project: project, author: author) }
 
+    def reloaded_merge_request
+      merge_request.reload
+      merge_request.reset_approval_cache!
+
+      merge_request
+    end
+
     it "includes approvers set on the MR" do
       expect do
         create(:approver, user: create(:user), target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "includes approvers from group" do
@@ -703,7 +710,7 @@ describe MergeRequest do
 
       expect do
         create(:approver_group, group: group, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "includes project members with developer access and up" do
@@ -718,7 +725,7 @@ describe MergeRequest do
         # Add this user as both someone with access, and an explicit approver,
         # to ensure they aren't double-counted.
         create(:approver, user: developer, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
     end
 
     it "excludes users who have already approved the MR" do
@@ -726,14 +733,14 @@ describe MergeRequest do
         approver = create(:user)
         create(:approver, user: approver, target: merge_request)
         create(:approval, user: approver, merge_request: merge_request)
-      end.not_to change { merge_request.reload.number_of_potential_approvers }
+      end.not_to change { reloaded_merge_request.number_of_potential_approvers }
     end
 
     it "excludes the MR author" do
       expect do
         create(:approver, user: create(:user), target: merge_request)
         create(:approver, user: author, target: merge_request)
-      end.to change { merge_request.reload.number_of_potential_approvers }.by(1)
+      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "excludes blocked users" do
@@ -742,7 +749,7 @@ describe MergeRequest do
       project.add_developer(developer)
       project.add_developer(blocked_developer)
 
-      expect(merge_request.reload.number_of_potential_approvers).to eq(2)
+      expect(reloaded_merge_request.number_of_potential_approvers).to eq(2)
     end
 
     context "when the project is part of a group" do
@@ -760,7 +767,7 @@ describe MergeRequest do
           group.add_master(create(:user))
           blocked_developer = create(:user).tap { |u| u.block! }
           group.add_developer(blocked_developer)
-        end.to change { merge_request.reload.number_of_potential_approvers }.by(2)
+        end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
       end
     end
   end