Commit 664bc371 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'sh-optimize-mr-api-emojis-and-labels' into 'master'

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

Closes #34159

See merge request !12732
parents b347749a 42f10974
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
......
---
title: Remove remaining N+1 queries in merge requests API with emojis and labels
merge_request:
author:
......@@ -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
......
......@@ -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
......
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
require 'spec_helper'
describe Gitlab::IssuableMetadata, lib: true do
let(:user) { create(:user) }
let!(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
subject { Class.new { include Gitlab::IssuableMetadata }.new }
it 'returns an empty Hash if an empty collection is provided' do
expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({})
end
context 'issues' do
let!(:issue) { create(:issue, author: user, project: project) }
let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) }
let!(:downvote) { create(:award_emoji, :downvote, awardable: closed_issue) }
let!(:upvote) { create(:award_emoji, :upvote, awardable: issue) }
let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
it 'aggregates stats on issues' do
data = subject.issuable_meta_data(Issue.all, 'Issue')
expect(data.count).to eq(2)
expect(data[issue.id].upvotes).to eq(1)
expect(data[issue.id].downvotes).to eq(0)
expect(data[issue.id].notes_count).to eq(0)
expect(data[issue.id].merge_requests_count).to eq(1)
expect(data[closed_issue.id].upvotes).to eq(0)
expect(data[closed_issue.id].downvotes).to eq(1)
expect(data[closed_issue.id].notes_count).to eq(0)
expect(data[closed_issue.id].merge_requests_count).to eq(0)
end
end
context 'merge requests' do
let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") }
let!(:merge_request_closed) { create(:merge_request, state: "closed", source_project: project, target_project: project, title: "Closed Test") }
let!(:downvote) { create(:award_emoji, :downvote, awardable: merge_request) }
let!(:upvote) { create(:award_emoji, :upvote, awardable: merge_request) }
let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
it 'aggregates stats on merge requests' do
data = subject.issuable_meta_data(MergeRequest.all, 'MergeRequest')
expect(data.count).to eq(2)
expect(data[merge_request.id].upvotes).to eq(1)
expect(data[merge_request.id].downvotes).to eq(1)
expect(data[merge_request.id].notes_count).to eq(1)
expect(data[merge_request.id].merge_requests_count).to eq(0)
expect(data[merge_request_closed.id].upvotes).to eq(0)
expect(data[merge_request_closed.id].downvotes).to eq(0)
expect(data[merge_request_closed.id].notes_count).to eq(0)
expect(data[merge_request_closed.id].merge_requests_count).to eq(0)
end
end
end
......@@ -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]
......@@ -32,6 +36,18 @@ describe API::MergeRequests do
end
context "when authenticated" do
it 'avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new do
get api("/projects/#{project.id}/merge_requests", user)
end.count
create(:merge_request, state: 'closed', milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time)
expect do
get api("/projects/#{project.id}/merge_requests", user)
end.not_to exceed_query_limit(control_count)
end
it "returns an array of all merge_requests" do
get api("/projects/#{project.id}/merge_requests", user)
......@@ -44,6 +60,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 +164,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 +255,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
......
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