Commit 88c4248a authored by Stan Hu's avatar Stan Hu

Remove remaining N+1 queries in merge requests API with emojis and labels

Closes #34159
parent 420f6b54
module IssuableCollections module IssuableCollections
extend ActiveSupport::Concern extend ActiveSupport::Concern
include SortingHelper include SortingHelper
include Gitlab::IssuableMetadata
included do included do
helper_method :issues_finder helper_method :issues_finder
...@@ -9,39 +10,6 @@ module IssuableCollections ...@@ -9,39 +10,6 @@ module IssuableCollections
private private
def issuable_meta_data(issuable_collection, collection_type)
# map has to be used here since using pluck or select will
# throw an error when ordering issuables by priority which inserts
# a new order into the collection.
# We cannot use reorder to not mess up the paginated collection.
issuable_ids = issuable_collection.map(&:id)
return {} if issuable_ids.empty?
issuable_note_count = Note.count_for_collection(issuable_ids, @collection_type)
issuable_votes_count = AwardEmoji.votes_for_collection(issuable_ids, @collection_type)
issuable_merge_requests_count =
if collection_type == 'Issue'
MergeRequestsClosingIssues.count_for_collection(issuable_ids)
else
[]
end
issuable_ids.each_with_object({}) do |id, issuable_meta|
downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
upvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
notes = issuable_note_count.find { |notes| notes.noteable_id == id }
merge_requests = issuable_merge_requests_count.find { |mr| mr.first == id }
issuable_meta[id] = Issuable::IssuableMeta.new(
upvotes.try(:count).to_i,
downvotes.try(:count).to_i,
notes.try(:count).to_i,
merge_requests.try(:last).to_i
)
end
end
def issues_collection def issues_collection
issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace) issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace)
end end
......
---
title: Remove remaining N+1 queries in merge requests API with emojis and labels
merge_request:
author:
...@@ -316,10 +316,26 @@ module API ...@@ -316,10 +316,26 @@ module API
class MergeRequestBasic < ProjectEntity class MergeRequestBasic < ProjectEntity
expose :target_branch, :source_branch expose :target_branch, :source_branch
expose :upvotes, :downvotes expose :upvotes do |merge_request, options|
if options[:issuable_metadata]
options[:issuable_metadata][merge_request.id].upvotes
else
merge_request.upvotes
end
end
expose :downvotes do |merge_request, options|
if options[:issuable_metadata]
options[:issuable_metadata][merge_request.id].downvotes
else
merge_request.downvotes
end
end
expose :author, :assignee, using: Entities::UserBasic expose :author, :assignee, using: Entities::UserBasic
expose :source_project_id, :target_project_id expose :source_project_id, :target_project_id
expose :label_names, as: :labels expose :labels do |merge_request, options|
# Avoids an N+1 query since labels are preloaded
merge_request.labels.map(&:title).sort
end
expose :work_in_progress?, as: :work_in_progress expose :work_in_progress?, as: :work_in_progress
expose :milestone, using: Entities::Milestone expose :milestone, using: Entities::Milestone
expose :merge_when_pipeline_succeeds expose :merge_when_pipeline_succeeds
......
...@@ -10,6 +10,8 @@ module API ...@@ -10,6 +10,8 @@ module API
resource :projects, requirements: { id: %r{[^/]+} } do resource :projects, requirements: { id: %r{[^/]+} } do
include TimeTrackingEndpoints include TimeTrackingEndpoints
helpers ::Gitlab::IssuableMetadata
helpers do helpers do
def handle_merge_request_errors!(errors) def handle_merge_request_errors!(errors)
if errors[:project_access].any? if errors[:project_access].any?
...@@ -42,8 +44,7 @@ module API ...@@ -42,8 +44,7 @@ module API
args[:label_name] = args.delete(:labels) args[:label_name] = args.delete(:labels)
merge_requests = MergeRequestsFinder.new(current_user, args).execute merge_requests = MergeRequestsFinder.new(current_user, args).execute
.inc_notes_with_associations .preload(:notes, :target_project, :author, :assignee, :milestone, :merge_request_diff, :labels)
.preload(:target_project, :author, :assignee, :milestone, :merge_request_diff)
merge_requests.reorder(args[:order_by] => args[:sort]) merge_requests.reorder(args[:order_by] => args[:sort])
end end
...@@ -82,8 +83,9 @@ module API ...@@ -82,8 +83,9 @@ module API
authorize! :read_merge_request, user_project authorize! :read_merge_request, user_project
merge_requests = find_merge_requests(project_id: user_project.id) merge_requests = find_merge_requests(project_id: user_project.id)
issuable_metadata = issuable_meta_data(merge_requests, 'MergeRequest')
present paginate(merge_requests), with: Entities::MergeRequestBasic, current_user: current_user, project: user_project present paginate(merge_requests), with: Entities::MergeRequestBasic, current_user: current_user, project: user_project, issuable_metadata: issuable_metadata
end end
desc 'Create a merge request' do desc 'Create a merge request' do
......
module Gitlab
module IssuableMetadata
def issuable_meta_data(issuable_collection, collection_type)
# map has to be used here since using pluck or select will
# throw an error when ordering issuables by priority which inserts
# a new order into the collection.
# We cannot use reorder to not mess up the paginated collection.
issuable_ids = issuable_collection.map(&:id)
return {} if issuable_ids.empty?
issuable_note_count = ::Note.count_for_collection(issuable_ids, collection_type)
issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
issuable_merge_requests_count =
if collection_type == 'Issue'
::MergeRequestsClosingIssues.count_for_collection(issuable_ids)
else
[]
end
issuable_ids.each_with_object({}) do |id, issuable_meta|
downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
upvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
notes = issuable_note_count.find { |notes| notes.noteable_id == id }
merge_requests = issuable_merge_requests_count.find { |mr| mr.first == id }
issuable_meta[id] = ::Issuable::IssuableMeta.new(
upvotes.try(:count).to_i,
downvotes.try(:count).to_i,
notes.try(:count).to_i,
merge_requests.try(:last).to_i
)
end
end
end
end
...@@ -16,7 +16,11 @@ describe API::MergeRequests do ...@@ -16,7 +16,11 @@ describe API::MergeRequests do
let!(:label) do let!(:label) do
create(:label, title: 'label', color: '#FFAABB', project: project) create(:label, title: 'label', color: '#FFAABB', project: project)
end end
let!(:label2) { create(:label, title: 'a-test', color: '#FFFFFF', project: project) }
let!(:label_link) { create(:label_link, label: label, target: merge_request) } let!(:label_link) { create(:label_link, label: label, target: merge_request) }
let!(:label_link2) { create(:label_link, label: label2, target: merge_request) }
let!(:downvote) { create(:award_emoji, :downvote, awardable: merge_request) }
let!(:upvote) { create(:award_emoji, :upvote, awardable: merge_request) }
before do before do
project.team << [user, :reporter] project.team << [user, :reporter]
...@@ -44,6 +48,9 @@ describe API::MergeRequests do ...@@ -44,6 +48,9 @@ describe API::MergeRequests do
expect(json_response.last['sha']).to eq(merge_request.diff_head_sha) expect(json_response.last['sha']).to eq(merge_request.diff_head_sha)
expect(json_response.last['merge_commit_sha']).to be_nil expect(json_response.last['merge_commit_sha']).to be_nil
expect(json_response.last['merge_commit_sha']).to eq(merge_request.merge_commit_sha) expect(json_response.last['merge_commit_sha']).to eq(merge_request.merge_commit_sha)
expect(json_response.last['downvotes']).to eq(1)
expect(json_response.last['upvotes']).to eq(1)
expect(json_response.last['labels']).to eq([label2.title, label.title])
expect(json_response.first['title']).to eq(merge_request_merged.title) expect(json_response.first['title']).to eq(merge_request_merged.title)
expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha) expect(json_response.first['sha']).to eq(merge_request_merged.diff_head_sha)
expect(json_response.first['merge_commit_sha']).not_to be_nil expect(json_response.first['merge_commit_sha']).not_to be_nil
...@@ -145,7 +152,7 @@ describe API::MergeRequests do ...@@ -145,7 +152,7 @@ describe API::MergeRequests do
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(1) expect(json_response.length).to eq(1)
expect(json_response.first['labels']).to eq([label.title]) expect(json_response.first['labels']).to eq([label2.title, label.title])
end end
it 'returns an array of labeled merge requests where all labels match' do it 'returns an array of labeled merge requests where all labels match' do
...@@ -236,8 +243,8 @@ describe API::MergeRequests do ...@@ -236,8 +243,8 @@ describe API::MergeRequests do
expect(json_response['author']).to be_a Hash expect(json_response['author']).to be_a Hash
expect(json_response['target_branch']).to eq(merge_request.target_branch) expect(json_response['target_branch']).to eq(merge_request.target_branch)
expect(json_response['source_branch']).to eq(merge_request.source_branch) expect(json_response['source_branch']).to eq(merge_request.source_branch)
expect(json_response['upvotes']).to eq(0) expect(json_response['upvotes']).to eq(1)
expect(json_response['downvotes']).to eq(0) expect(json_response['downvotes']).to eq(1)
expect(json_response['source_project_id']).to eq(merge_request.source_project.id) expect(json_response['source_project_id']).to eq(merge_request.source_project.id)
expect(json_response['target_project_id']).to eq(merge_request.target_project.id) expect(json_response['target_project_id']).to eq(merge_request.target_project.id)
expect(json_response['work_in_progress']).to be_falsy expect(json_response['work_in_progress']).to be_falsy
......
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