diff options
| -rw-r--r-- | app/controllers/concerns/issuable_collections.rb | 34 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml | 4 | ||||
| -rw-r--r-- | lib/api/entities.rb | 20 | ||||
| -rw-r--r-- | lib/api/merge_requests.rb | 8 | ||||
| -rw-r--r-- | lib/gitlab/issuable_metadata.rb | 36 | ||||
| -rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 13 |
6 files changed, 74 insertions, 41 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 693e2f6365c..e18778cdf80 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -1,6 +1,7 @@ module IssuableCollections extend ActiveSupport::Concern include SortingHelper + include Gitlab::IssuableMetadata included do helper_method :issues_finder @@ -9,39 +10,6 @@ module IssuableCollections 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 issues_finder.execute.preload(:project, :author, :assignees, :labels, :milestone, project: :namespace) end diff --git a/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml b/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml new file mode 100644 index 00000000000..9589659cdc2 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-mr-api-emojis-and-labels.yml @@ -0,0 +1,4 @@ +--- +title: Remove remaining N+1 queries in merge requests API with emojis and labels +merge_request: +author: diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f4796f311a5..945f2821d72 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -316,10 +316,26 @@ module API class MergeRequestBasic < ProjectEntity 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 :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 :milestone, using: Entities::Milestone expose :merge_when_pipeline_succeeds diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index d419d345ec5..4ad1eef4ff1 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -10,6 +10,8 @@ module API resource :projects, requirements: { id: %r{[^/]+} } do include TimeTrackingEndpoints + helpers ::Gitlab::IssuableMetadata + helpers do def handle_merge_request_errors!(errors) if errors[:project_access].any? @@ -42,8 +44,7 @@ module API args[:label_name] = args.delete(:labels) merge_requests = MergeRequestsFinder.new(current_user, args).execute - .inc_notes_with_associations - .preload(:target_project, :author, :assignee, :milestone, :merge_request_diff) + .preload(:notes, :target_project, :author, :assignee, :milestone, :merge_request_diff, :labels) merge_requests.reorder(args[:order_by] => args[:sort]) end @@ -82,8 +83,9 @@ module API authorize! :read_merge_request, user_project 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 desc 'Create a merge request' do diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb new file mode 100644 index 00000000000..977c05910d3 --- /dev/null +++ b/lib/gitlab/issuable_metadata.rb @@ -0,0 +1,36 @@ +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 diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4d0bd67c571..6b67523981c 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -16,7 +16,11 @@ describe API::MergeRequests do let!(:label) do create(:label, title: 'label', color: '#FFAABB', project: project) 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_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 project.team << [user, :reporter] @@ -44,6 +48,9 @@ describe API::MergeRequests do 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 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['sha']).to eq(merge_request_merged.diff_head_sha) expect(json_response.first['merge_commit_sha']).not_to be_nil @@ -145,7 +152,7 @@ describe API::MergeRequests do expect(response).to have_http_status(200) expect(json_response).to be_an Array 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 it 'returns an array of labeled merge requests where all labels match' do @@ -236,8 +243,8 @@ describe API::MergeRequests do expect(json_response['author']).to be_a Hash 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['upvotes']).to eq(0) - expect(json_response['downvotes']).to eq(0) + expect(json_response['upvotes']).to eq(1) + expect(json_response['downvotes']).to eq(1) 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['work_in_progress']).to be_falsy |