Commit fd072f7d authored by Sean McGivern's avatar Sean McGivern

Fix CI pipelines N+1 in Issues::ReferencedMergeRequestsService

Whether the preloading belongs in the service or the controller is arguable,
here. As the service is only used for one controller action, it seems reasonable
to put it in the service, but that is not a definitive answer.

Adding the preloads for MR project routes here doesn't seem to work, perhaps
because of https://github.com/rails/rails/issues/32140.
parent 2017c5c6
...@@ -3,10 +3,14 @@ ...@@ -3,10 +3,14 @@
module Issues module Issues
class ReferencedMergeRequestsService < Issues::BaseService class ReferencedMergeRequestsService < Issues::BaseService
def execute(issue) def execute(issue)
[ referenced = referenced_merge_requests(issue)
sort_by_iid(referenced_merge_requests(issue)), closed_by = closed_by_merge_requests(issue)
sort_by_iid(closed_by_merge_requests(issue)) preloader = ActiveRecord::Associations::Preloader.new
]
preloader.preload(referenced + closed_by,
head_pipeline: { project: [:route, { namespace: :route }] })
[sort_by_iid(referenced), sort_by_iid(closed_by)]
end end
def referenced_merge_requests(issue) def referenced_merge_requests(issue)
......
...@@ -47,6 +47,24 @@ describe Issues::ReferencedMergeRequestsService do ...@@ -47,6 +47,24 @@ describe Issues::ReferencedMergeRequestsService do
expect { service.execute(issue) }.not_to exceed_query_limit(control_count) expect { service.execute(issue) }.not_to exceed_query_limit(control_count)
end end
it 'preloads the head pipeline for each merge request, and its routes' do
# Hack to ensure no data is preserved on issue before starting the spec,
# to avoid false negatives
reloaded_issue = Issue.find(issue.id)
pipeline_routes = lambda do |merge_requests|
merge_requests.map { |mr| mr.head_pipeline&.project&.full_path }
end
closing_mr_other_project.update!(head_pipeline: create(:ci_pipeline))
control_count = ActiveRecord::QueryRecorder.new { service.execute(reloaded_issue).each(&pipeline_routes) }
closing_mr.update!(head_pipeline: create(:ci_pipeline))
expect { service.execute(issue).each(&pipeline_routes) }
.not_to exceed_query_limit(control_count)
end
end end
end end
......
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