Merge branch 'sh-add-simple-mode-mr-api-9-4-stable' into '9-4-stable-preparing-rc3'

Cherry pick performance improvements and simple merge request API changes to 9-4-stable

See merge request !12799

9 files changed, 189 insertions, 42 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-add-mr-simple-mode.yml b/changelogs/unreleased/sh-add-mr-simple-mode.yml
new file mode 100644
index 00000000000..0033ca28444
--- /dev/null
+++ b/changelogs/unreleased/sh-add-mr-simple-mode.yml
@@ -0,0 +1,4 @@
+---
+title: Add a simple mode to merge request API
+merge_request:
+author:
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/doc/api/merge_requests.md b/doc/api/merge_requests.md
index 3dc808c196d..c90d95e4dd0 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -25,6 +25,7 @@ Parameters:
 | `order_by`| string  | no    | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` |
 | `sort`    | string  | no    | Return requests sorted in `asc` or `desc` order. Default is `desc`  |
 | `milestone`  | string  | no | Return merge requests for a specific milestone |
+| `view` | string | no | If `simple`, returns the `iid`, URL, title, description, and basic state of merge request |
 | `labels`  | string  | no | Return merge requests matching a comma separated list of labels |
 | `created_after` | datetime | no | Return merge requests created after the given time (inclusive) |
 | `created_before` | datetime | no | Return merge requests created before the given time (inclusive) |
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index f4796f311a5..47742a164fc 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -314,12 +314,35 @@ module API
       expose :id
     end
 
+    class MergeRequestSimple < ProjectEntity
+      expose :title
+      expose :web_url do |merge_request, options|
+        Gitlab::UrlBuilder.build(merge_request)
+      end
+    end
+
     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..ac33b2b801c 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,10 +44,14 @@ 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)
+                             .reorder(args[:order_by] => args[:sort])
+          merge_requests = paginate(merge_requests)
+                             .preload(:target_project)
+
+          return merge_requests if args[:view] == 'simple'
 
-          merge_requests.reorder(args[:order_by] => args[:sort])
+          merge_requests
+            .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels)
         end
 
         params :optional_params_ce do
@@ -76,6 +82,7 @@ module API
         optional :labels, type: String, desc: 'Comma-separated list of label names'
         optional :created_after, type: DateTime, desc: 'Return merge requests created after the specified time'
         optional :created_before, type: DateTime, desc: 'Return merge requests created before the specified time'
+        optional :view, type: String, values: %w[simple], desc: 'If simple, returns the `iid`, URL, title, description, and basic state of merge request'
         use :pagination
       end
       get ":id/merge_requests" do
@@ -83,7 +90,17 @@ module API
 
         merge_requests = find_merge_requests(project_id: user_project.id)
 
-        present paginate(merge_requests), with: Entities::MergeRequestBasic, current_user: current_user, project: user_project
+        options = { with: Entities::MergeRequestBasic,
+                    current_user: current_user,
+                    project: user_project }
+
+        if params[:view] == 'simple'
+          options[:with] = Entities::MergeRequestSimple
+        else
+          options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest')
+        end
+
+        present merge_requests, options
       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/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb
new file mode 100644
index 00000000000..f9f4b290dbf
--- /dev/null
+++ b/spec/lib/gitlab/issuable_metadata_spec.rb
@@ -0,0 +1,59 @@
+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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 4d0bd67c571..9098ae6bcda 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]
@@ -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,12 +60,31 @@ 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
         expect(json_response.first['merge_commit_sha']).to eq(merge_request_merged.merge_commit_sha)
       end
 
+      it "returns an array of all merge_requests using simple mode" do
+        get api("/projects/#{project.id}/merge_requests?view=simple", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response.last.keys).to match_array(%w(id iid title web_url created_at description project_id state updated_at))
+        expect(json_response).to be_an Array
+        expect(json_response.length).to eq(3)
+        expect(json_response.last['iid']).to eq(merge_request.iid)
+        expect(json_response.last['title']).to eq(merge_request.title)
+        expect(json_response.last).to have_key('web_url')
+        expect(json_response.first['iid']).to eq(merge_request_merged.iid)
+        expect(json_response.first['title']).to eq(merge_request_merged.title)
+        expect(json_response.first).to have_key('web_url')
+      end
+
       it "returns an array of all merge_requests" do
         get api("/projects/#{project.id}/merge_requests?state", user)
 
@@ -145,7 +180,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 +271,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