Commit 7d4d9365 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'issue_25900_2' into 'master'

Gather issuable  metadata to avoid n+ queries on index view

Closes #25900

See merge request !9006
parents 6a1b3ef7 0b14b654
...@@ -9,6 +9,28 @@ module IssuableCollections ...@@ -9,6 +9,28 @@ module IssuableCollections
private 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 def issues_collection
issues_finder.execute.preload(:project, :author, :assignee, :labels, :milestone, project: :namespace) issues_finder.execute.preload(:project, :author, :assignee, :labels, :milestone, project: :namespace)
end end
......
...@@ -9,6 +9,9 @@ module IssuesAction ...@@ -9,6 +9,9 @@ module IssuesAction
.non_archived .non_archived
.page(params[:page]) .page(params[:page])
@collection_type = "Issue"
@issuable_meta_data = issuable_meta_data(@issues)
respond_to do |format| respond_to do |format|
format.html format.html
format.atom { render layout: false } format.atom { render layout: false }
......
...@@ -7,6 +7,9 @@ module MergeRequestsAction ...@@ -7,6 +7,9 @@ module MergeRequestsAction
@merge_requests = merge_requests_collection @merge_requests = merge_requests_collection
.page(params[:page]) .page(params[:page])
@collection_type = "MergeRequest"
@issuable_meta_data = issuable_meta_data(@merge_requests)
end end
private private
......
...@@ -23,8 +23,11 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -23,8 +23,11 @@ class Projects::IssuesController < Projects::ApplicationController
respond_to :html respond_to :html
def index def index
@issues = issues_collection @collection_type = "Issue"
@issues = @issues.page(params[:page]) @issues = issues_collection
@issues = @issues.page(params[:page])
@issuable_meta_data = issuable_meta_data(@issues)
if @issues.out_of_range? && @issues.total_pages != 0 if @issues.out_of_range? && @issues.total_pages != 0
return redirect_to url_for(params.merge(page: @issues.total_pages)) return redirect_to url_for(params.merge(page: @issues.total_pages))
end end
......
...@@ -36,8 +36,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -36,8 +36,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts] before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
def index def index
@merge_requests = merge_requests_collection @collection_type = "MergeRequest"
@merge_requests = @merge_requests.page(params[:page]) @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 if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
return redirect_to url_for(params.merge(page: @merge_requests.total_pages)) return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
end end
......
...@@ -16,6 +16,14 @@ class AwardEmoji < ActiveRecord::Base ...@@ -16,6 +16,14 @@ class AwardEmoji < ActiveRecord::Base
scope :downvotes, -> { where(name: DOWNVOTE_NAME) } scope :downvotes, -> { where(name: DOWNVOTE_NAME) }
scope :upvotes, -> { where(name: UPVOTE_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? def downvote?
self.name == DOWNVOTE_NAME self.name == DOWNVOTE_NAME
end end
......
...@@ -15,6 +15,11 @@ module Issuable ...@@ -15,6 +15,11 @@ module Issuable
include Taskable include Taskable
include TimeTrackable 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 included do
cache_markdown_field :title, pipeline: :single_line cache_markdown_field :title, pipeline: :single_line
cache_markdown_field :description cache_markdown_field :description
......
...@@ -108,6 +108,12 @@ class Note < ActiveRecord::Base ...@@ -108,6 +108,12 @@ class Note < ActiveRecord::Base
Discussion.for_diff_notes(active_notes). Discussion.for_diff_notes(active_notes).
map { |d| [d.line_code, d] }.to_h map { |d| [d.line_code, d] }.to_h
end 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 end
def cross_reference? def cross_reference?
......
...@@ -17,22 +17,7 @@ ...@@ -17,22 +17,7 @@
%li %li
= link_to_member(@project, issue.assignee, name: false, title: "Assigned to :name") = link_to_member(@project, issue.assignee, name: false, title: "Assigned to :name")
- upvotes, downvotes = issue.upvotes, issue.downvotes = render 'shared/issuable_meta_data', issuable: issue
- 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
.issue-info .issue-info
#{issuable_reference(issue)} &middot; #{issuable_reference(issue)} &middot;
......
...@@ -29,22 +29,7 @@ ...@@ -29,22 +29,7 @@
%li %li
= link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: "Assigned to :name") = link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: "Assigned to :name")
- upvotes, downvotes = merge_request.upvotes, merge_request.downvotes = render 'shared/issuable_meta_data', issuable: merge_request
- 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
.merge-request-info .merge-request-info
#{issuable_reference(merge_request)} &middot; #{issuable_reference(merge_request)} &middot;
......
- 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
---
title: Gather issuable metadata to avoid n+1 queries on index view
merge_request:
author:
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
...@@ -24,6 +24,8 @@ describe Projects::IssuesController do ...@@ -24,6 +24,8 @@ describe Projects::IssuesController do
project.team << [user, :developer] project.team << [user, :developer]
end end
it_behaves_like "issuables list meta-data", :issue
it "returns index" do it "returns index" do
get :index, namespace_id: project.namespace.path, project_id: project.path get :index, namespace_id: project.namespace.path, project_id: project.path
......
...@@ -147,6 +147,8 @@ describe Projects::MergeRequestsController do ...@@ -147,6 +147,8 @@ describe Projects::MergeRequestsController do
end end
describe 'GET index' do describe 'GET index' do
let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
def get_merge_requests(page = nil) def get_merge_requests(page = nil)
get :index, get :index,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
...@@ -154,6 +156,8 @@ describe Projects::MergeRequestsController do ...@@ -154,6 +156,8 @@ describe Projects::MergeRequestsController do
state: 'opened', page: page.to_param state: 'opened', page: page.to_param
end end
it_behaves_like "issuables list meta-data", :merge_request
context 'when page param' do context 'when page param' do
let(:last_page) { project.merge_requests.page().total_pages } let(:last_page) { project.merge_requests.page().total_pages }
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) }
......
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
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
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