Skip to content

Commit

Permalink
Fix CI pipelines N+1 in Issues::ReferencedMergeRequestsService
Browse files Browse the repository at this point in the history
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 rails/rails#32140.
  • Loading branch information
smcgivern committed Aug 16, 2018
1 parent cae147c commit 0c5a9e7
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 4 deletions.
12 changes: 8 additions & 4 deletions app/services/issues/referenced_merge_requests_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
module Issues
class ReferencedMergeRequestsService < Issues::BaseService
def execute(issue)
[
sort_by_iid(referenced_merge_requests(issue)),
sort_by_iid(closed_by_merge_requests(issue))
]
referenced = referenced_merge_requests(issue)
closed_by = 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

def referenced_merge_requests(issue)
Expand Down
19 changes: 19 additions & 0 deletions spec/services/issues/referenced_merge_requests_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,25 @@ def create_closing_mr(attributes = {})

expect { service.execute(issue) }.not_to exceed_query_limit(control_count)
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
issue_id = issue.id
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(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

Expand Down

0 comments on commit 0c5a9e7

Please sign in to comment.