Merge branch '198028-fj-improve-approval-merge-request-rules-relation' into 'master'

Refactor approval_rules association and caching some used methods

See merge request gitlab-org/gitlab!41965

ee/app/models/approval_merge_request_rule.rb

@@ -73,15 +73,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
     retry
   end
 
-  def self.applicable_to_branch(branch)
-    includes(:users, :groups, approval_project_rule: [:users, :groups, :protected_branches]).select do |rule|
-      next true unless rule.approval_project_rule.present?
-      next true if rule.overridden?
-
-      rule.approval_project_rule.applies_to_branch?(branch)
-    end
-  end
-
   def audit_add(_model)
     # no-op
     # only audit on project rule

ee/app/models/approval_project_rule.rb

@@ -20,14 +20,6 @@ class ApprovalProjectRule < ApplicationRecord
   validates :name, uniqueness: { scope: [:project_id, :rule_type] }
   validates :rule_type, uniqueness: { scope: :project_id, message: proc { _('any-approver for the project already exists') } }, if: :any_approver?
 
-  def self.applicable_to_branch(branch)
-    includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
-  end
-
-  def self.inapplicable_to_branch(branch)
-    includes(:protected_branches).reject { |rule| rule.applies_to_branch?(branch) }
-  end
-
   def applies_to_branch?(branch)
     return true if protected_branches.empty?
 

ee/app/models/approval_state.rb

@@ -207,7 +207,7 @@ class ApprovalState
     strong_memoize(:wrapped_rules) do
       merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch)
 
-      merge_request_rules.map do |rule|
+      merge_request_rules.map! do |rule|
         ApprovalWrappedRule.wrap(merge_request, rule)
       end
     end

ee/app/models/approval_wrapped_rule.rb

@@ -46,7 +46,9 @@ class ApprovalWrappedRule
   end
 
   def approvers
-    filter_approvers(@approval_rule.approvers)
+    strong_memoize(:approvers) do
+      filter_approvers(@approval_rule.approvers)
+    end
   end
 
   # @return [Array<User>] of users who have approved the merge request
@@ -61,13 +63,11 @@ class ApprovalWrappedRule
   # - Additional complexity to add update hooks
   # - DB updating many MRs for one project rule change is inefficient
   def approved_approvers
-    if merge_request.merged? && approval_rule.is_a?(ApprovalMergeRequestRule) && approval_rule.approved_approvers.present?
+    if merge_request.merged? && approval_rule.is_a?(ApprovalMergeRequestRule) && approval_rule.approved_approvers.any?
       return approval_rule.approved_approvers
     end
 
     strong_memoize(:approved_approvers) do
-      overall_approver_ids = merge_request.approvals.map(&:user_id).to_set
-
       approvers.select do |approver|
         overall_approver_ids.include?(approver.id)
       end
@@ -113,4 +113,14 @@ class ApprovalWrappedRule
 
     ApprovalState.filter_committers(filtered_approvers, merge_request)
   end
+
+  def overall_approver_ids
+    current_approvals = merge_request.approvals
+
+    if current_approvals.is_a?(ActiveRecord::Relation) && !current_approvals.loaded?
+      current_approvals.distinct.pluck(:user_id)
+    else
+      current_approvals.map(&:user_id).to_set
+    end
+  end
 end

ee/app/models/ee/merge_request.rb

@@ -19,7 +19,21 @@ module EE
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
-      has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
+      has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request do
+        def applicable_to_branch(branch)
+          ActiveRecord::Associations::Preloader.new.preload(
+            self,
+            [:users, :groups, approval_project_rule: [:users, :groups, :protected_branches]]
+          )
+
+          self.select do |rule|
+            next true unless rule.approval_project_rule.present?
+            next true if rule.overridden?
+
+            rule.approval_project_rule.applies_to_branch?(branch)
+          end
+        end
+      end
       has_one :merge_train, inverse_of: :merge_request, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
       has_many :blocks_as_blocker,

ee/app/models/ee/project.rb

@@ -51,7 +51,15 @@ module EE
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-      has_many :approval_rules, class_name: 'ApprovalProjectRule'
+      has_many :approval_rules, class_name: 'ApprovalProjectRule' do
+        def applicable_to_branch(branch)
+          includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
+        end
+
+        def inapplicable_to_branch(branch)
+          includes(:protected_branches).reject { |rule| rule.applies_to_branch?(branch) }
+        end
+      end
       has_many :approval_merge_request_rules, through: :merge_requests, source: :approval_rules
       has_many :audit_events, as: :entity
       has_many :path_locks
@@ -648,26 +656,32 @@ module EE
     end
 
     def disable_overriding_approvers_per_merge_request
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-      return super unless has_regulated_settings?
+      strong_memoize(:disable_overriding_approvers_per_merge_request) do
+        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+        next super unless has_regulated_settings?
 
-      ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
+        ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request?
+      end
     end
     alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
 
     def merge_requests_author_approval
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-      return super unless has_regulated_settings?
+      strong_memoize(:merge_requests_author_approval) do
+        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+        next super unless has_regulated_settings?
 
-      !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
+        !::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
+      end
     end
     alias_method :merge_requests_author_approval?, :merge_requests_author_approval
 
     def merge_requests_disable_committers_approval
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-      return super unless has_regulated_settings?
+      strong_memoize(:merge_requests_disable_committers_approval) do
+        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+        next super unless has_regulated_settings?
 
-      ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
+        ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval?
+      end
     end
     alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
 
@@ -737,7 +751,8 @@ module EE
 
     def user_defined_rules
       strong_memoize(:user_defined_rules) do
-        approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc)
+        # Loading the relation in order to memoize it loaded
+        approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc).load
       end
     end
 

ee/changelogs/unreleased/198028-fj-improve-approval-merge-request-rules-relation.yml

+---
+title: Refactor approval_rules association
+merge_request: 41965
+author:
+type: performance

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -59,6 +59,39 @@ RSpec.describe Projects::MergeRequestsController do
     sign_in(viewer)
   end
 
+  describe 'GET index' do
+    def get_merge_requests
+      get :index,
+        params: {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          state: 'opened'
+        }
+    end
+
+    context 'when filtering by opened state' do
+      context 'with opened merge requests' do
+        render_views
+        it 'avoids N+1' do
+          other_user = create(:user)
+          create(:merge_request, :unique_branches, target_project: project, source_project: project)
+          create_list(:approval_merge_request_rule, 5, merge_request: merge_request, users: [user, other_user], approvals_required: 2)
+
+          control_count = ActiveRecord::QueryRecorder.new { get_merge_requests }.count
+
+          create_list(:approval, 10)
+          create_list(:merge_request, 20, :unique_branches, target_project: project, source_project: project).each do |mr|
+            create(:approval_merge_request_rule, merge_request: merge_request, users: [user, other_user], approvals_required: 2)
+          end
+
+          expect do
+            get_merge_requests
+          end.not_to exceed_query_limit(control_count)
+        end
+      end
+    end
+  end
+
   describe 'PUT update' do
     before do
       project.update(approvals_before_merge: 2)
@@ -66,12 +99,12 @@ RSpec.describe Projects::MergeRequestsController do
 
     def update_merge_request(params = {})
       post :update,
-           params: {
-             namespace_id: merge_request.target_project.namespace.to_param,
-             project_id: merge_request.target_project.to_param,
-             id: merge_request.iid,
-             merge_request: params
-           }
+        params: {
+          namespace_id: merge_request.target_project.namespace.to_param,
+          project_id: merge_request.target_project.to_param,
+          id: merge_request.iid,
+          merge_request: params
+        }
     end
 
     context 'when the merge request requires approval' do

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -195,70 +195,6 @@ RSpec.describe ApprovalMergeRequestRule do
     end
   end
 
-  describe '.applicable_to_branch' do
-    let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
-    let(:branch) { 'stable' }
-
-    subject { described_class.applicable_to_branch(branch) }
-
-    shared_examples_for 'with applicable rules to specified branch' do
-      it { is_expected.to eq([rule]) }
-    end
-
-    context 'when there are no associated source rules' do
-      it_behaves_like 'with applicable rules to specified branch'
-    end
-
-    context 'when there are associated source rules' do
-      let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
-
-      before do
-        rule.update!(approval_project_rule: source_rule)
-      end
-
-      context 'and rule is not overridden' do
-        before do
-          rule.update!(
-            name: source_rule.name,
-            approvals_required: source_rule.approvals_required,
-            users: source_rule.users,
-            groups: source_rule.groups
-          )
-        end
-
-        context 'and there are no associated protected branches to source rule' do
-          it_behaves_like 'with applicable rules to specified branch'
-        end
-
-        context 'and there are associated protected branches to source rule' do
-          before do
-            source_rule.update!(protected_branches: protected_branches)
-          end
-
-          context 'and branch matches' do
-            let(:protected_branches) { [create(:protected_branch, name: branch)] }
-
-            it_behaves_like 'with applicable rules to specified branch'
-          end
-
-          context 'but branch does not match anything' do
-            let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
-
-            it { is_expected.to be_empty }
-          end
-        end
-      end
-
-      context 'but rule is overridden' do
-        before do
-          rule.update!(name: 'Overridden Rule')
-        end
-
-        it_behaves_like 'with applicable rules to specified branch'
-      end
-    end
-  end
-
   describe '#project' do
     it 'returns project of MergeRequest' do
       expect(subject.project).to be_present

ee/spec/models/approval_project_rule_spec.rb

@@ -152,64 +152,6 @@ RSpec.describe ApprovalProjectRule do
     end
   end
 
-  describe '.applicable_to_branch' do
-    let!(:rule) { create(:approval_project_rule) }
-    let(:branch) { 'stable' }
-
-    subject { described_class.applicable_to_branch(branch) }
-
-    context 'when there are no associated protected branches' do
-      it { is_expected.to eq([rule]) }
-    end
-
-    context 'when there are associated protected branches' do
-      before do
-        rule.update!(protected_branches: protected_branches)
-      end
-
-      context 'and branch matches' do
-        let(:protected_branches) { [create(:protected_branch, name: branch)] }
-
-        it { is_expected.to eq([rule]) }
-      end
-
-      context 'but branch does not match anything' do
-        let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
-
-        it { is_expected.to be_empty }
-      end
-    end
-  end
-
-  describe '.inapplicable_to_branch' do
-    let!(:rule) { create(:approval_project_rule) }
-    let(:branch) { 'stable' }
-
-    subject { described_class.inapplicable_to_branch(branch) }
-
-    context 'when there are no associated protected branches' do
-      it { is_expected.to be_empty }
-    end
-
-    context 'when there are associated protected branches' do
-      before do
-        rule.update!(protected_branches: protected_branches)
-      end
-
-      context 'and branch does not match anything' do
-        let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
-
-        it { is_expected.to eq([rule]) }
-      end
-
-      context 'but branch matches' do
-        let(:protected_branches) { [create(:protected_branch, name: branch)] }
-
-        it { is_expected.to be_empty }
-      end
-    end
-  end
-
   describe 'callbacks', :request_store do
     let_it_be(:user) { create(:user, name: 'Batman') }
     let_it_be(:group) { create(:group, name: 'Justice League') }

ee/spec/models/merge_request_spec.rb

@@ -24,6 +24,73 @@ RSpec.describe MergeRequest do
     it { is_expected.to have_many(:approver_groups).dependent(:delete_all) }
     it { is_expected.to have_many(:approved_by_users) }
     it { is_expected.to have_one(:merge_train) }
+    it { is_expected.to have_many(:approval_rules) }
+
+    describe 'approval_rules association' do
+      describe '#applicable_to_branch' do
+        let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
+        let(:branch) { 'stable' }
+
+        subject { merge_request.approval_rules.applicable_to_branch(branch) }
+
+        shared_examples_for 'with applicable rules to specified branch' do
+          it { is_expected.to eq([rule]) }
+        end
+
+        context 'when there are no associated source rules' do
+          it_behaves_like 'with applicable rules to specified branch'
+        end
+
+        context 'when there are associated source rules' do
+          let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
+
+          before do
+            rule.update!(approval_project_rule: source_rule)
+          end
+
+          context 'and rule is not overridden' do
+            before do
+              rule.update!(
+                name: source_rule.name,
+                approvals_required: source_rule.approvals_required,
+                users: source_rule.users,
+                groups: source_rule.groups
+              )
+            end
+
+            context 'and there are no associated protected branches to source rule' do
+              it_behaves_like 'with applicable rules to specified branch'
+            end
+
+            context 'and there are associated protected branches to source rule' do
+              before do
+                source_rule.update!(protected_branches: protected_branches)
+              end
+
+              context 'and branch matches' do
+                let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+                it_behaves_like 'with applicable rules to specified branch'
+              end
+
+              context 'and branch does not match anything' do
+                let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+                it { is_expected.to be_empty }
+              end
+            end
+          end
+
+          context 'and rule is overridden' do
+            before do
+              rule.update!(name: 'Overridden Rule')
+            end
+
+            it_behaves_like 'with applicable rules to specified branch'
+          end
+        end
+      end
+    end
   end
 
   it_behaves_like 'an editable mentionable with EE-specific mentions' do

ee/spec/models/project_spec.rb

@@ -49,6 +49,65 @@ RSpec.describe Project do
 
     it { is_expected.to have_one(:github_service) }
     it { is_expected.to have_many(:project_aliases) }
+    it { is_expected.to have_many(:approval_rules) }
+
+    describe 'approval_rules association' do
+      let_it_be(:rule, reload: true) { create(:approval_project_rule) }
+      let(:project) { rule.project }
+      let(:branch) { 'stable' }
+
+      describe '#applicable_to_branch' do
+        subject { project.approval_rules.applicable_to_branch(branch) }
+
+        context 'when there are no associated protected branches' do
+          it { is_expected.to eq([rule]) }
+        end
+
+        context 'when there are associated protected branches' do
+          before do
+            rule.update!(protected_branches: protected_branches)
+          end
+
+          context 'and branch matches' do
+            let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+            it { is_expected.to eq([rule]) }
+          end
+
+          context 'but branch does not match anything' do
+            let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+            it { is_expected.to be_empty }
+          end
+        end
+      end
+
+      describe '#inapplicable_to_branch' do
+        subject { project.approval_rules.inapplicable_to_branch(branch) }
+
+        context 'when there are no associated protected branches' do
+          it { is_expected.to be_empty }
+        end
+
+        context 'when there are associated protected branches' do
+          before do
+            rule.update!(protected_branches: protected_branches)
+          end
+
+          context 'and branch does not match anything' do
+            let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+            it { is_expected.to eq([rule]) }
+          end
+
+          context 'but branch matches' do
+            let(:protected_branches) { [create(:protected_branch, name: branch)] }
+
+            it { is_expected.to be_empty }
+          end
+        end
+      end
+    end
   end
 
   context 'scopes' do