summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/concerns/issuable_collections.rb22
-rw-r--r--app/controllers/concerns/issues_action.rb3
-rw-r--r--app/controllers/concerns/merge_requests_action.rb3
-rw-r--r--app/controllers/projects/issues_controller.rb7
-rw-r--r--app/controllers/projects/merge_requests_controller.rb7
-rw-r--r--app/models/award_emoji.rb8
-rw-r--r--app/models/concerns/issuable.rb5
-rw-r--r--app/models/note.rb6
-rw-r--r--app/views/projects/issues/_issue.html.haml17
-rw-r--r--app/views/projects/merge_requests/_merge_request.html.haml17
-rw-r--r--app/views/shared/_issuable_meta_data.html.haml19
-rw-r--r--changelogs/unreleased/issue_25900.yml4
-rw-r--r--spec/controllers/dashboard_controller_spec.rb19
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb2
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb4
-rw-r--r--spec/features/issuables/issuable_list_spec.rb57
-rw-r--r--spec/support/issuables_list_metadata_shared_examples.rb35
17 files changed, 199 insertions, 36 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..6937ad3bdd9 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(*) as 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..029fe667a45 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(*) as 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..0e3902c066a 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;
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..bfd134e406e 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -147,6 +147,8 @@ describe Projects::MergeRequestsController do
end
describe 'GET index' do
+ let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
+
def get_merge_requests(page = nil)
get :index,
namespace_id: project.namespace.to_param,
@@ -154,6 +156,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
View Lorry Controller Status