Gather issuable metadata to avoid n+ queries on index viewissue_25900

17 files changed, 199 insertions, 37 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 6247934f81e..a6e158ebae6 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -9,6 +9,28 @@ module IssuableCollections
 
   private
 
+  def issuable_meta_data(issuable_collection)
+    # 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)
+    issuable_note_count  = Note.count_for_collection(issuable_ids, @collection_type)
+    issuable_votes_count = AwardEmoji.votes_for_collection(issuable_ids, @collection_type)
+
+    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 }
+
+      issuable_meta[id] = Issuable::IssuableMeta.new(
+        upvotes.try(:count).to_i,
+        downvotes.try(:count).to_i,
+        notes.try(:count).to_i
+      )
+    end
+  end
+
   def issues_collection
     issues_finder.execute.preload(:project, :author, :assignee, :labels, :milestone, project: :namespace)
   end
diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb
index b46adcceb60..fb5edb34370 100644
--- a/app/controllers/concerns/issues_action.rb
+++ b/app/controllers/concerns/issues_action.rb
@@ -9,6 +9,9 @@ module IssuesAction
               .non_archived
               .page(params[:page])
 
+    @collection_type    = "Issue"
+    @issuable_meta_data = issuable_meta_data(@issues)
+
     respond_to do |format|
       format.html
       format.atom { render layout: false }
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
index fdb05bb3228..6229759dcf1 100644
--- a/app/controllers/concerns/merge_requests_action.rb
+++ b/app/controllers/concerns/merge_requests_action.rb
@@ -7,6 +7,9 @@ module MergeRequestsAction
 
     @merge_requests = merge_requests_collection
                       .page(params[:page])
+
+    @collection_type    = "MergeRequest"
+    @issuable_meta_data = issuable_meta_data(@merge_requests)
   end
 
   private
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index c75b8987a4b..744a4af1c51 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -23,8 +23,11 @@ class Projects::IssuesController < Projects::ApplicationController
   respond_to :html
 
   def index
-    @issues = issues_collection
-    @issues = @issues.page(params[:page])
+    @collection_type    = "Issue"
+    @issues             = issues_collection
+    @issues             = @issues.page(params[:page])
+    @issuable_meta_data = issuable_meta_data(@issues)
+
     if @issues.out_of_range? && @issues.total_pages != 0
       return redirect_to url_for(params.merge(page: @issues.total_pages))
     end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index fbad66c5c40..c3e1760f168 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -36,8 +36,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
 
   def index
-    @merge_requests = merge_requests_collection
-    @merge_requests = @merge_requests.page(params[:page])
+    @collection_type    = "MergeRequest"
+    @merge_requests     = merge_requests_collection
+    @merge_requests     = @merge_requests.page(params[:page])
+    @issuable_meta_data = issuable_meta_data(@merge_requests)
+
     if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
       return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
     end
diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb
index 46b17479d6d..eabe68f0deb 100644
--- a/app/models/award_emoji.rb
+++ b/app/models/award_emoji.rb
@@ -16,6 +16,14 @@ class AwardEmoji < ActiveRecord::Base
   scope :downvotes, -> { where(name: DOWNVOTE_NAME) }
   scope :upvotes,   -> { where(name: UPVOTE_NAME) }
 
+  class << self
+    def votes_for_collection(ids, type)
+      select('name', 'awardable_id', 'COUNT(*)').
+        where('name IN (?) AND awardable_type = ? AND awardable_id IN (?)', [DOWNVOTE_NAME, UPVOTE_NAME], type, ids).
+        group('name', 'awardable_id')
+    end
+  end
+
   def downvote?
     self.name == DOWNVOTE_NAME
   end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 3517969eabc..bfb54e878fc 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -15,6 +15,11 @@ module Issuable
   include Taskable
   include TimeTrackable
 
+  # This object is used to gather issuable meta data for displaying
+  # upvotes, downvotes and notes count for issues and merge requests
+  # lists avoiding n+1 queries and improving performance.
+  IssuableMeta = Struct.new(:upvotes, :downvotes, :notes_count)
+
   included do
     cache_markdown_field :title, pipeline: :single_line
     cache_markdown_field :description
diff --git a/app/models/note.rb b/app/models/note.rb
index bf090a0438c..b4bdc42a788 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -108,6 +108,12 @@ class Note < ActiveRecord::Base
       Discussion.for_diff_notes(active_notes).
         map { |d| [d.line_code, d] }.to_h
     end
+
+    def count_for_collection(ids, type)
+      user.select('noteable_id', 'COUNT(*)').
+        group(:noteable_id).
+        where(noteable_type: type, noteable_id: ids)
+    end
   end
 
   def cross_reference?
diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml
index 5c9839cb330..52c372312e4 100644
--- a/app/views/projects/issues/_issue.html.haml
+++ b/app/views/projects/issues/_issue.html.haml
@@ -17,22 +17,7 @@
             %li
               = link_to_member(@project, issue.assignee, name: false, title: "Assigned to :name")
 
-          - upvotes, downvotes = issue.upvotes, issue.downvotes
-          - if upvotes > 0
-            %li
-              = icon('thumbs-up')
-              = upvotes
-
-          - if downvotes > 0
-            %li
-              = icon('thumbs-down')
-              = downvotes
-
-          - note_count = issue.notes.user.count
-          %li
-            = link_to issue_path(issue, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
-              = icon('comments')
-              = note_count
+          = render 'shared/issuable_meta_data', issuable: issue
 
       .issue-info
         #{issuable_reference(issue)} &middot;
@@ -57,5 +42,6 @@
           %span.task-status
             = issue.task_status
 
+
         .pull-right.issue-updated-at
           %span updated #{time_ago_with_tooltip(issue.updated_at, placement: 'bottom', html_class: 'issue_update_ago')}
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index a5fbe9d6128..11b7aaec704 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -29,22 +29,7 @@
           %li
             = link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: "Assigned to :name")
 
-        - upvotes, downvotes = merge_request.upvotes, merge_request.downvotes
-        - if upvotes > 0
-          %li
-            = icon('thumbs-up')
-            = upvotes
-
-        - if downvotes > 0
-          %li
-            = icon('thumbs-down')
-            = downvotes
-
-        - note_count = merge_request.related_notes.user.count
-        %li
-          = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
-            = icon('comments')
-            = note_count
+        = render 'shared/issuable_meta_data', issuable: merge_request
 
     .merge-request-info
       #{issuable_reference(merge_request)} &middot;
diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml
new file mode 100644
index 00000000000..1264e524d86
--- /dev/null
+++ b/app/views/shared/_issuable_meta_data.html.haml
@@ -0,0 +1,19 @@
+- note_count         = @issuable_meta_data[issuable.id].notes_count
+- issue_votes        = @issuable_meta_data[issuable.id]
+- upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
+- issuable_url       = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')
+
+- if upvotes > 0
+  %li
+    = icon('thumbs-up')
+    = upvotes
+
+- if downvotes > 0
+  %li
+    = icon('thumbs-down')
+    = downvotes
+
+%li
+  = link_to issuable_url, class: ('no-comments' if note_count.zero?) do
+    = icon('comments')
+    = note_count
diff --git a/changelogs/unreleased/issue_25900.yml b/changelogs/unreleased/issue_25900.yml
new file mode 100644
index 00000000000..b4b72b8a20c
--- /dev/null
+++ b/changelogs/unreleased/issue_25900.yml
@@ -0,0 +1,4 @@
+---
+title: Gather issuable metadata to avoid n+1 queries on index view
+merge_request:
+author:
diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb
new file mode 100644
index 00000000000..566d8515198
--- /dev/null
+++ b/spec/controllers/dashboard_controller_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe DashboardController do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  before do
+    project.team << [user, :master]
+    sign_in(user)
+  end
+
+  describe 'GET issues' do
+    it_behaves_like 'issuables list meta-data', :issue, :issues
+  end
+
+  describe 'GET merge requests' do
+    it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests
+  end
+end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 4b89381eb96..e576bf9ef79 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -24,6 +24,8 @@ describe Projects::IssuesController do
         project.team << [user, :developer]
       end
 
+      it_behaves_like "issuables list meta-data", :issue
+
       it "returns index" do
         get :index, namespace_id: project.namespace.path, project_id: project.path
 
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 63780802cfa..f43575a2927 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -5,7 +5,7 @@ describe Projects::MergeRequestsController do
 
   let(:project) { create(:project) }
   let(:user)    { create(:user) }
-  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
+  let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
   let(:merge_request_with_conflicts) do
     create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr|
       mr.mark_as_unmergeable
@@ -154,6 +154,8 @@ describe Projects::MergeRequestsController do
           state: 'opened', page: page.to_param
     end
 
+    it_behaves_like "issuables list meta-data", :merge_request
+
     context 'when page param' do
       let(:last_page) { project.merge_requests.page().total_pages }
       let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb
new file mode 100644
index 00000000000..e31bc40adc3
--- /dev/null
+++ b/spec/features/issuables/issuable_list_spec.rb
@@ -0,0 +1,57 @@
+require 'rails_helper'
+
+describe 'issuable list', feature: true do
+  let(:project) { create(:empty_project) }
+  let(:user)    { create(:user) }
+
+  issuable_types = [:issue, :merge_request]
+
+  before do
+    project.add_user(user, :developer)
+    login_as(user)
+    issuable_types.each { |type| create_issuables(type) }
+  end
+
+  issuable_types.each do |issuable_type|
+    it "avoids N+1 database queries for #{issuable_type.to_s.humanize.pluralize}" do
+      control_count = ActiveRecord::QueryRecorder.new { visit_issuable_list(issuable_type) }.count
+
+      create_issuables(issuable_type)
+
+      expect { visit_issuable_list(issuable_type) }.not_to exceed_query_limit(control_count)
+    end
+
+    it "counts upvotes, downvotes and notes count for each #{issuable_type.to_s.humanize}" do
+      visit_issuable_list(issuable_type)
+
+      expect(first('.fa-thumbs-up').find(:xpath, '..')).to have_content(1)
+      expect(first('.fa-thumbs-down').find(:xpath, '..')).to have_content(1)
+      expect(first('.fa-comments').find(:xpath, '..')).to have_content(2)
+    end
+  end
+
+  def visit_issuable_list(issuable_type)
+    if issuable_type == :issue
+      visit namespace_project_issues_path(project.namespace, project)
+    else
+      visit namespace_project_merge_requests_path(project.namespace, project)
+    end
+  end
+
+  def create_issuables(issuable_type)
+    3.times do
+      if issuable_type == :issue
+        issuable = create(:issue, project: project, author: user)
+      else
+        issuable = create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: FFaker::Name.name)
+      end
+
+      2.times do
+        create(:note_on_issue, noteable: issuable, project: project, note: 'Test note')
+      end
+
+      create(:award_emoji, :downvote, awardable: issuable)
+      create(:award_emoji, :upvote, awardable: issuable)
+    end
+  end
+end
diff --git a/spec/support/issuables_list_metadata_shared_examples.rb b/spec/support/issuables_list_metadata_shared_examples.rb
new file mode 100644
index 00000000000..dac94dfc31e
--- /dev/null
+++ b/spec/support/issuables_list_metadata_shared_examples.rb
@@ -0,0 +1,35 @@
+shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
+  before do
+    @issuable_ids = []
+
+    2.times do
+      if issuable_type == :issue
+        issuable = create(issuable_type, project: project)
+      else
+        issuable = create(issuable_type, title: FFaker::Lorem.sentence, source_project: project, source_branch: FFaker::Name.name)
+      end
+
+      @issuable_ids << issuable.id
+
+      issuable.id.times       { create(:note, noteable: issuable, project: issuable.project) }
+      (issuable.id + 1).times { create(:award_emoji, :downvote, awardable: issuable) }
+      (issuable.id + 2).times { create(:award_emoji, :upvote, awardable: issuable) }
+    end
+  end
+
+  it "creates indexed meta-data object for issuable notes and votes count" do
+    if action
+      get action
+    else
+      get :index, namespace_id: project.namespace.path, project_id: project.path
+    end
+
+    meta_data = assigns(:issuable_meta_data)
+
+    @issuable_ids.each do |id|
+      expect(meta_data[id].notes_count).to eq(id)
+      expect(meta_data[id].downvotes).to eq(id + 1)
+      expect(meta_data[id].upvotes).to eq(id + 2)
+    end
+  end
+end