Commit 0b14b654 authored by Felipe Artur's avatar Felipe Artur

Gather issuable metadata to avoid n+ queries on index view

parent c4fd6ff4
......@@ -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
......
......@@ -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 }
......
......@@ -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
......
......@@ -23,8 +23,11 @@ class Projects::IssuesController < Projects::ApplicationController
respond_to :html
def index
@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
......
......@@ -36,8 +36,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
def index
@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
......
......@@ -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
......
......@@ -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
......
......@@ -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?
......
......@@ -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;
......
......@@ -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;
......
- 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
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
......
......@@ -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) }
......
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