Commit e4d23a5b authored by Jan Provaznik's avatar Jan Provaznik

Fix N+1 issue in approval rules

We can cache the list of approver IDs instead of
always getting the list of IDs.
parent 3ee0a7e1
......@@ -115,12 +115,14 @@ class ApprovalWrappedRule
end
def overall_approver_ids
current_approvals = merge_request.approvals
strong_memoize(:overall_approver_ids) do
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
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
end
---
title: Optimize merge request approvals checking
merge_request: 45009
author:
type: performance
......@@ -136,6 +136,22 @@ RSpec.describe ApprovalWrappedRule do
expect(subject.approved_approvers).to contain_exactly(approver1)
end
end
it 'avoids N+1 queries' do
rule = create(:approval_project_rule, project: merge_request.project, approvals_required: approvals_required)
rule.users = [approver1]
approved_approvers_for_rule_id(rule.id) # warm up the cache
control_count = ActiveRecord::QueryRecorder.new { approved_approvers_for_rule_id(rule.id) }.count
rule.users += [approver2, approver3]
expect { approved_approvers_for_rule_id(rule.id) }.not_to exceed_query_limit(control_count)
end
def approved_approvers_for_rule_id(rule_id)
described_class.new(merge_request, ApprovalProjectRule.find(rule_id)).approved_approvers
end
end
describe "#commented_approvers" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment