kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Fix authors N+1 in Issues::ReferencedMergeRequestsService

Browse source 
`#referenced_merge_requests` preloaded too many associations. Award emoji, for
instance, are completely unnecessary here.

`#closed_by_merge_requests` had the opposite problem: `#all_references` needs
each item's author, and these weren't preloaded.
This commit is contained in:
Sean McGivern 2018-08-16 12:08:00 +01:00
parent c73da6c1e7
commit 22d8fbacaf
2 changed files with 37 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

26
app/services/issues/referenced_merge_requests_service.rb
View file

@ -10,13 +10,7 @@ module Issues
end
def referenced_merge_requests(issue)
ext = issue.all_references(current_user)
issue.notes_with_associations.each do |object|
object.all_references(current_user, extractor: ext)
end
merge_requests = ext.merge_requests.sort_by(&:iid)
merge_requests = extract_merge_requests(issue, issue.notes)
cross_project_filter = -> (merge_requests) do
merge_requests.select { |mr| mr.target_project == project }
@ -34,13 +28,7 @@ module Issues
def closed_by_merge_requests(issue)
return [] unless issue.open?
ext = issue.all_references(current_user)
issue.notes.system.each do |note|
note.all_references(current_user, extractor: ext)
end
merge_requests = ext.merge_requests.select(&:open?)
merge_requests = extract_merge_requests(issue, issue.notes.system).select(&:open?)
return [] if merge_requests.empty?
@ -50,6 +38,16 @@ module Issues
private
def extract_merge_requests(issue, notes)
ext = issue.all_references(current_user)
notes.includes(:author).each do |note|
note.all_references(current_user, extractor: ext)
end
ext.merge_requests
end
def sort_by_iid(merge_requests)
Gitlab::IssuableSorter.sort(project, merge_requests) { |mr| mr.iid.to_s }
end

26
spec/services/issues/referenced_merge_requests_service_spec.rb
View file

@ -5,7 +5,7 @@ require 'spec_helper.rb'
describe Issues::ReferencedMergeRequestsService do
def create_referencing_mr(attributes = {})
create(:merge_request, attributes).tap do |merge_request|
create(:note, :system, project: project, noteable: issue, note: merge_request.to_reference(full: true))
create(:note, :system, project: project, noteable: issue, author: user, note: merge_request.to_reference(full: true))
end
end
@ -54,6 +54,18 @@ describe Issues::ReferencedMergeRequestsService do
expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project)
expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project)
end
context 'performance' do
it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
service.referenced_merge_requests(issue) # warm cache
control_count = ActiveRecord::QueryRecorder.new { service.referenced_merge_requests(issue) }.count
create(:note, project: project, noteable: issue, author: create(:user))
service.referenced_merge_requests(issue) # warm cache
expect { service.referenced_merge_requests(issue) }.not_to exceed_query_limit(control_count)
end
end
end
describe '#closed_by_merge_requests' do
@ -68,5 +80,17 @@ describe Issues::ReferencedMergeRequestsService do
it 'returns an empty array when the current issue is closed already' do
expect(service.closed_by_merge_requests(closed_issue)).to eq([])
end
context 'performance' do
it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
service.closed_by_merge_requests(issue) # warm cache
control_count = ActiveRecord::QueryRecorder.new { service.closed_by_merge_requests(issue) }.count
create(:note, :system, project: project, noteable: issue, author: create(:user))
service.closed_by_merge_requests(issue) # warm cache
expect { service.closed_by_merge_requests(issue) }.not_to exceed_query_limit(control_count)
end
end
end
end